Skip to content
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

[RFC] task_group_dynamic_dependencies #1469

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

vossmjp
Copy link
Contributor

@vossmjp vossmjp commented Aug 5, 2024

Description

A proposal to extend task_group:

  1. Extend semantics and useful lifetime of task_handle. We propose task_handle to represent tasks for the purpose of adding dependencies. The useful lifetime and semantics of task_handle will need to be extended to include tasks that have been submitted, are currently executing, or have been completed.
  2. Add functions to set task dependencies. In the current task_group, tasks can only be waited for as a group and there is no direct way to add any before-after relationships between individual tasks. We will discuss options for spelling.
  3. Add a function to move successors from a currently executing task to a new task. This functionality is necessary for recursively generated task graphs. This case represents a situation where it is safe to modify dependencies for an already submitted task.

Type of change

  • 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

@pavelkumbrasev
Copy link

I think this proposal is lacking the final definition of class task_handle including all the new methods.

Base automatically changed from dev/vossmjp/rfcs to master September 26, 2024 14:02
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
void add_successor(task_handle& th);
};

void transfer_successors_to(task_handle& th);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this method is related to the currently executing task, what about including this API into tbb::this_task:: namespace? By analogy with tbb::this_task_arena:: namespace.

Copy link
Contributor

@akukanov akukanov Dec 5, 2024

Choose a reason for hiding this comment

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

I would use tbb::task namespace, since it is already used for suspend (which applies to the currently running task) and resume functions.
Actually no, I think it should not be in the namespace task or this_task or just tbb, but rather it should be a static function or a member function in task_group. The reason is that, since task_group::defer is the only way to create a new non-empty task_handle, the method to transfer successors cannot be used in arbitrary tasks, only in task_group tasks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add to task_group

rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
Comment on lines 363 to 365
- Are the suggested APIs sufficient?
- Are there additional use cases that should be considered that we missed in our analysis?
- Are there other parts of the pre-oneTBB tasking API that developers have struggled to find a good alternative for?
Copy link
Contributor

Choose a reason for hiding this comment

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

These are interesting questions.

While reading this RFC, I kind of rushed proposing additional syntax sugar that besides being more user-friendly, since it seems to represent popular use cases, can save some CPU cycles. So I am posting them here for a discussion.

  1. Would it be useful to have a method that simultaneously joins (or even fuses?) the instantiation of a task and transferring of the current task successors?
    Something like:
    template <typename Func>
    task_handle transfer_successors_to(Func&& f);

For the recursive decomposition scenario, instantiating a new task within an executing task and transferring successors to that new task seems to be the main model of writing such algorithms. Although, it seems to be not saving much (only one call to the library and assign to a couple of pointers?), there is always(?) going to be such a sequence in the code. Otherwise, how else an execution of already submitted tasks can be postponed?

Shall we also consider a question of having that API instead or in addition to the one proposed?

  1. As for add_predecessor(s) and add_successor(s) I have a couple of thoughts.
    a. It seems again that it might be useful to merge instantiation of the new task handles and adding them as successors/predecessors:
        template <typename Func>
        void add_predecessor(Func&& f);
    
        template <typename Func>
        void add_successor(Func&& f);
    b. Also, I think having an API that would allow adding more than one predecessor/successor at once can be useful, since usually a number of successors/predecessors are instantiated. I only think that we don't need to limit ourselves to only two parameters as it was proposed optionally, but allow passing of an arbitrary size of task handles or even user lambdas. Of course, a pattern of having a single task producer might be viewed as a limiting one, but there actually might be the cases where tasks cannot be made available to the scheduler (i.e. spawned) until all of them are gathered together from different producers, which essentially represents a barrier in the execution pipeline. Not to mention that the spawning of a bunch of tasks all together are done faster than regular one by one spawnings. Spawning of a bunch of tasks at once was implemented in the old TBB, as far as I remember.
    So here I suggest to have something like:
        template <typename... Func>
        void add_predecessors(Func&& ...f);
    
        template <typename... Func>
        void add_successor(Func&& ...f);
    However, this also seems to ask for having the task_group::run() method to accept an arbitrary size of task handles and/or functors. So, perhaps, it is more related to another RFC/extension...

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 we should start with minimal API then extend with syntactic sugar based on use cases. I suspect this will start as an experimental feature to allow some feedback on API.

Even so, I'm also open to including these additional APIs in the initial implementation, if others think they're likely needed.

@vossmjp vossmjp force-pushed the dev/vossmjp/rfc_task_group_dynamic_dependencies branch from 579bcd8 to ff7e366 Compare December 4, 2024 23:37
@vossmjp vossmjp marked this pull request as ready for review December 4, 2024 23:40
Comment on lines 140 to 147
Given two `task_handle` objects `h1` and `h2`, some possible options
for adding `h1` as an in-dependence / predecessor of `h2` include:

- `h2.add_predecessor(h1)`
- `h2 = defer([]() { … }, h1)`
- `make_edge(h1, h2)`

We propose including the first option. Similarly, there could be
Copy link
Contributor

@akukanov akukanov Dec 5, 2024

Choose a reason for hiding this comment

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

I would prefer not to add methods to task_handle but use "external" functions, perhaps in the task_group class. This would be more consistent with the current approach (defer, run, run_and_wait) as well as with transfer_successors_to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would the signature of such a function look like? The nice part of it being in task_handle is that h2.add_predecessor(h1) is easy to understand -- h1 becomes a predecessor of h2. As part of task_group, the order becomes less clear: tg.add_predecessor(h1,h2) might be read as adding two predecessors to task_group tg.

Copy link
Contributor

Choose a reason for hiding this comment

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

What would the signature of such a function look like?

Similar to make_edge, but perhaps with a different name: connect, set_order, set_dependency, etc., with the left-to-right ordering semantics.

Currently, a task_handle is what its name says - a handle, an object that might represent a created task or be empty. It cannot even be copied, only moved.

The proposal suggests that a task_handle should represent a task at any state, so should perhaps internally track the task state somehow - that's OK. We can even think of a method to query the task state; that would also be OK.

But making the handle also tracking task dependencies, and so serving as a part of the scheduling system, rather than just an object to be scheduled, does not sound right to me. Conceptually, I see scheduling and dependency management as functions of a task group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last year, we discuss naming quite a bit. One of the reasons to offer make_edge as an option was to be consistent with flow graph terminology and therefore the expected left-to-right ordering. Some people that worked with tasks before thought of parent-child relationships or dependency relations as a depends on b. And so something like task_group::set_dependency(a, b) could easily be read two ways. The benefit of task_handle::add_predecessor is its readibility and a task_group::make_edge(a, b) might benefit from exposure to flow::make_edge. I don't think we should consider it now, but we could also consider make_edge between a task_handle and a flow graph continue_node.

rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
@vossmjp vossmjp added the RFC label Jan 9, 2025
Copy link
Contributor

@akukanov akukanov left a comment

Choose a reason for hiding this comment

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

I support this proposal in principle, and would approve it up to but not including the proposed API changes.

The API changes seem both insufficient (see the comment about the run function semantics) and arguable (when it comes to the way to set dependencies, see the discussion in comments above). I would not approve it even for an experimental API, at least until alternative API semantics are considered and compared.

rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
Comment on lines +83 to +86
task. A created task remains created until it is submitted through
`task_group::run` or `task_group::run_and_wait`. The current
`task_group` specification treats accessing a `task_handle` after it is submitted
via one of the run functions as undefined behavior. Therefore, a
Copy link
Contributor

@akukanov akukanov Feb 1, 2025

Choose a reason for hiding this comment

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

Note that this is technically done by the run functions accepting a task_handle as an rvalue. The task group can then move-construct or move-assign from that handle, which will make it empty.

In order to "extend useful lifetime of a task handle`, the run functions should therefore treat the handle argument differently. This part is not covered by the proposal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, needs to be added.

Comment on lines +96 to +97
In that case, passing a `task_handle` to `task_group::run` or `task_group::run_and_wait` only makes
it available for dependency tracking but does not make it immediately eligible for execution.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not forget also about task_arena::enqueue. What would be its behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, run and enqueue functions are not yet well covered.

rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
rfcs/proposed/task_group_dynamic_dependencies/README.md Outdated Show resolved Hide resolved
- Should we add a function to adds more than one predecessor as single call, such as `add_predecessors`?
- Should we add functions that merge creation and definition of predecessor tasks, such as
`template <typename Func> add_predecessor(Func&& f);`.
- Are there additional use cases that should be considered that we missed in our analysis?
Copy link
Contributor

@akukanov akukanov Feb 1, 2025

Choose a reason for hiding this comment

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

I can suggest a few more potentially interesting cases:

These seem to be interesting examples of "dynamic task graphs that are not trees", as stated in the introduction. I do not suggest that we must support these patterns, but it is interesting to see if we could, and what would be needed for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, more example would be good. In particular, we need to see if the very limited ways to add and modified predecessors is sufficient. And how lifetimes of task handles will be managed.

Co-authored-by: Alexey Kukanov <[email protected]>
Co-authored-by: Konstantin Boyarinov <[email protected]>
Comment on lines +200 to +203
Where `h` is a `task_handle` to a created task, and the
`transfer_successors_to` function must be called from within a task. Calling
this function from outside a task or passing anything other than a `task_handle`
representing a task in the created state is undefined behavior.
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 add information about possible limitations (or lack of limitations) for the task states represented by task_handle to which we are transferring the successors. Does it sufficient to allow only the handles representing created tasks, or other 3 states are also allowed (i.e. transferring successors to the task_handle that was already submitted to task_group::run).

@@ -0,0 +1,378 @@
# Extend ``task_group`` for Dynamic Task Dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

I have noticed one thing that is not related to the dependencies between tasks, but relates into covering the migration from the old tasking API to another, so I decided to add it as a comment here.

Let's consider the recursive Fibonacci example rewritten using the API proposed in this RFC (the splitting stage):

long* left = new long(0);
long* right = new long(0);

tbb::task_handle fib_left = tg.defer([&tg, num, left] {
    recursive_fib(tg, num - 2, *left);
});
tbb::task_handle fib_right = tg.defer([&tg, num, right] {
    recursive_fib(tg, num - 1, *right);
});
tbb::task_handle fib_sum = tg.defer([&result, left, right] {
    result = *left + *right;
    delete left;
    delete right;
});

|
The main difference between merge sort and this example is some data that is required for executing the task (left and right - the placeholders for partial results of Fibonacci calculations on leaft).
Since the lifetime of this data should be preserved until the sum task is executed, it cannot be placed on stack of current function and needs to be allocated dynamically.
Back to old TBB, this data was placed inside of the corresponding task that provides the required lifetime guarantees.
The question is do we need to extend the task_handle API somehow to allow putting the additional data to the task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants