Skip to content

Conversation

kboyarinov
Copy link
Contributor

@kboyarinov kboyarinov commented Oct 1, 2025

Description

Add a comprehensive description of proposed changes

Fixes # - issue number(s) if exists

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add a respective label(s) to PR if you have permissions

  • bug fix - change that fixes an issue
  • new feature - change that adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and some bug fixes
  • not needed

Documentation

  • updated in # - add PR number
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Notify the following users

List users with @ to send notifications

Other information

@kboyarinov kboyarinov marked this pull request as draft October 1, 2025 15:33
@kboyarinov kboyarinov marked this pull request as ready for review October 3, 2025 14:16
@vossmjp
Copy link
Contributor

vossmjp commented Oct 3, 2025

FYI, you accidentally checked in the try_put_and_wait binary.


The |full_name| implementation extends the requirements for user-provided function object from
`tbb::task_group specification <https://oneapi-spec.uxlfoundation.org/specifications/oneapi/latest/elements/onetbb/source/task_scheduler/task_group/task_group_cls>`_
to allow them to return a ``task_handle`` object.
Copy link
Contributor

@vossmjp vossmjp Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that task_group specification is silent on the return type of the user-provided function object. So "extends the requirements" doesn't seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand the idea of named requirement and the return type is that if the specification says nothing about the return type means that any type can be returned, but will be ignored. So "extending" the named requirement means that the implementation will react if the handle is returned.

void run(task_handle&& handle);
Schedules the task object associated with ``handle`` for execution, provided its dependencies are satisfied.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Schedules the task object associated with ``handle`` for execution, provided its dependencies are satisfied.
Schedules the task object associated with ``handle`` for execution when its dependencies are satisfied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "when" suggests that run will schedule the task when the dependencies are satisfies. It can be interpreted as waiting for the dependencies to be satisfied and then schedule the task.
Or you believe that together with the following note the behavior is obvious?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the ", provided" version could be read as though the "run" function does nothing if the task object dependencies are not satisfied at the time of the call to run. But of course calling run does have an effect. It make the task available for dependency tracking, and ultimately execution once its dependencies are satisfied.

Comment on lines +20 to +23
`Task Bypassing <../tbb_userguide/Task_Scheduler_Bypass.html>`_ allows developers to reduce task scheduling overhead by providing a hint about
which task should be executed next.

Execution of the deferred task owned by a returned ``task_handle`` is not guaranteed to occur immediately, nor to be performed by the same thread.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to say something about the returned task being effectively scheduled for execution, so it must not be explicitly submitted. This is also important because of the difference with task dependencies, where a task must be explicitly submitted (or bypassed).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a note at the end of description.

Comment on lines 58 to 59
template <typename RandomAccessIterator, typename Function>
void parallel_for(RandomAccessIterator begin, RandomAccessIterator end, Function f) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
template <typename RandomAccessIterator, typename Function>
void parallel_for(RandomAccessIterator begin, RandomAccessIterator end, Function f) {
template <typename RandomAccessIterator, typename Function>
void for_each(RandomAccessIterator begin, RandomAccessIterator end, Function f) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to parallel_for_each since with the name for_each it is challenging to test the function because of the ambiguity with std::for_each.

tbb::task_group& tg;
}; // struct for_task

// Function accepts std::iterator_traits<RandomAccessIterator>::reference argument
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand this comment; it seems to be incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was part of the original example. Removed.

Comment on lines 243 to 247
.. code:: cpp
bool operator==(const task_completion_handle& t, std::nullptr_t) noexcept;
*Returns*: ``true`` if ``t`` does not reference any task; otherwise, ``false``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe combine with the other operator== that only differs by the order of parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 249 to 253
.. code:: cpp
bool operator!=(const task_completion_handle& t, std::nullptr_t) noexcept;
Equivalent to ``!(t == nullptr)``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 270 to 275
.. code:: cpp
~task_handle();
Destroys the ``task_handle`` object and its associated task, if any.
If the associated task is involved in a predecessor-successor relationship, the behavior is undefined.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the existing API, would it make sense to take the exact description from the spec in full, the modify and highlight the differences?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have copied the exact description from the spec and highlighted the change using admonition.
It looks like this in the rendered reference:

destructor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants