-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
R2 RFC for task_group dynamic dependencies #1664
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexey Kukanov <[email protected]> Co-authored-by: Konstantin Boyarinov <[email protected]>
* Remove concrete proposals from the RFC * Apply review comments
…k_group_dynamic_dependencies
…fc-dynamic-dependencies-r2
|
||
namespace this_task_arena { | ||
void enqueue(task_handle&& h); // existing overload | ||
void enqueue(task_handle& h); // new overload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current enqueue
method is defined for submission of a "fire-and-forget" task the progress for which is not tracked. The proposed semantics to allow task_handle
to track the state of the task seems a bit contradictive with "fire-and-forget" tasks. But it still may be useful to set the dependencies for such tasks.
What do you think about this semantics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the other proposed extension, enqueue
will support not only "fire-and-forget" but the more general asynchronous execution semantics as well. Therefore I do not see much contradiction here.
* Semantics for ``task_group::run``, ``task_group::run_and_wait``, ``task_arena::enqueue`` and ``this_task_arena::enqueue`` should be defined | ||
when the input ``task_handle`` is handling a task in various states. | ||
* Semantics for returning a ``task_handle`` handling a task in various states from the body of the task should be described (while using the existing | ||
preview extensions for ``task_group``). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two sub-items are not sufficient unfortunately. First and foremost, the task_handle
class needs to evolve from the current move-only type to a type that can be safely shared between multiple threads and across program scopes.
(e.g. using ``task_group::run(std::move(task_handle))``) looks misleading even if some guarantees are provided for the referred handle object. It also creates an error-prone | ||
for TBB developers - any move-construct or move-assign from the accepted handle will break the guarantee. Code analyzers? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also creates an error-prone for TBB developers
An error-prone what? And whom specifically you refer to as TBB developers?
Also I would suggest to use "move construction" and "move assignment" here, with no hyphen (the hyphen is needed to form adjectives etc., e.g. "move-assignable", "move-assigned"). This would be consistent with the C++ standard use of the terms.
(e.g. using ``task_group::run(std::move(task_handle))``) looks misleading even if some guarantees are provided for the referred handle object. It also creates an error-prone | ||
for TBB developers - any move-construct or move-assign from the accepted handle will break the guarantee. Code analyzers? | ||
|
||
To handle this, the proposal is to extend all functions that take the task handled by the ``task_handle`` with the new overload taking an non-const lvalue reference and provide the following guarantees: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid that taking a non-const lvalue reference is error-prone.
Consider one of the examples from the umbrella RFC (a bit simplified here):
tbb::task_handle add_another_task(tbb::task_group& tg, int work_id) {
tbb::task_handle new_task = tg.defer([=] { do_work(work_id); });
// ...
tg.run(new_task); // takes the handle by reference
// Return the newly created task to the caller
return new_task;
}
I hope the problem is obvious.
If we want to support this pattern, taking task_handle by reference seems a bad idea. Of course it can be "copied" internally, or the actual task pointer can be extracted and stored somewhere, but the semantics of taking a handle by lvalue reference just does not seem right.
Probably we can/should think of it in the terms of ownership. The current design supports only a single owner, and it is reflected in the move semantics for submission: the ownership is transferred to the task scheduler. We want either a shared ownership, which can be represented by copies of the task handle, or some form of weak reference/observation (see https://en.cppreference.com/w/cpp/memory/weak_ptr). The API design should conform to the chosen approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the required semantics for ownership is somewhere in the middle. It seems like we need "unique" ownership to submit the task and "weak" ownership to track the task progress and set the dependencies. And the option may be to define 2 handlers of the task:
- Current
task_handle
that owns the task and can be used to make any action on it - submit for execution or add dependencies. - New handle (let's say
task_view
) that does not own the task, but can be used to track progress or add dependencies.
task_handle
would be almost unchanged. task_view
would be copyable and any copy would be able make actions on the underlying task that is owned either by the task_handle
or the TBB scheduler.
task_view
can be constructible from task_handle
but not the opposite.
Submitting functions (task_group::run
and others) would also remain unchanged and allow only the task_handle
argument.
Functions for adding dependencies would provide overloads taking both task_handle
and task_view
where the task in any state can be taken, and only the task_handle
where only the task in created state in allowed (like the successor side of make_edge
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have come to essentially the same conclusion. Even though a new class is an API complication, it seems that trying to extend task_handle
semantics and adjust all the related APIs would complicate everything even more.
I would not use task_view
for the name, as it is not really a view to a task. Rather, it's a weak_task_handle
- though that it is also not ideal because, unlike weak_ptr
, this type should not allow "promotion" to a "strong" task handle.
Extra care must be taken while working with this method anyway since even if it returns ``false``, it may be unsafe to submit the ``task_handle`` since the state | ||
can be changed by the other thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems related to the ownership question. Maybe some form of "exclusive" ownership is needed to submit the task.
Description
Add RFC describing the semantics for concrete task_group dynamic dependencies APIs
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
Tests
Documentation
Breaks backward compatibility
Notify the following users
List users with
@
to send notificationsOther information