Add implementation details information for task group dynamic dependencies

[kboyarinov]

Description

Add RFC describing the implementation of task_group dynamic dependencies.
RFC can be found here: #1664
Implementation:

• Part 1 (task_completion_handle): Implementation of task_group dynamic dependencies - part 1 - task_completion_handle #1682
• Part 2 (set_task_order): Implementation for task_group dynamic dependencies - part 2 - set_task_order #1685
• Part 3 (transfer_this_task_completion_to): Implementation for task_group dynamic dependencies - part 3 - get_current_task entry point and transfer_this_task_completion_to #1727

The document describes the complete implementation

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

[aleksei-fedotov]

Technically, the task in submitted state can also be a recipient of successors as long as its predecessors are not yet completed (e.g., not submitted). However, as far as I understand, we don't have use cases for such scenario at the moment. Or is there another reason for that restriction?
Supposing we will need to add such possibility in the future. How will it look like?

[kboyarinov]

I see this extension as having two additional overloads for make_edge:

class task_group {
public:
    static void make_edge(task_handle& pred, task_tracker& succ);
    static void make_edge(task_tracker& pred, task_tracker& succ);
};

We will also need to strictly define the limitations for succ. Something like, if succ tracks the task in the state, other than created or submitted, or the state is concurrently changing while creating the edge, the behavior is undefined.
But I agree, we need to have a use-case for this first.

[aleksei-fedotov]

The class task has 48 bytes in reserve. If we re-use something from there, will it help simplifying the implementation or, perhaps, support other desired use cases (if any)?

[kboyarinov]

I don't think it can simplify the implementation and I don't see how the place where the additional pointer is placed can affect the covered use-cases.
Potential usage of these 48 bytes can be discussed. The only doubt I have is that we will reuse part of them only for the task_handle_tasks. What if other types of tasks need to use this space as well?

[akukanov]

In theory, we might "reserve" a void* pointer in the layout of task, which could be used differently in derived classes.

[aleksei-fedotov]

What will task_tracker track, if corresponding task_handle has been submitted by the time this constructor is called? task_handle becomes empty once it is run(), right? But previously there was an idea to be able to make predecessors from submitted tasks. If task_tracker tracks nothing, then specifying it as a predecessor could potentially result in a data race since the task it is supposed to track may not be finished yet. Thus, this task as a predecessor and specified successor may be running simultaneously. Can you please point out where I am wrong?

[kboyarinov]

I think that move semantics for task_handle protects us from such a races in some manner. What should the user expect if it concurrently copies and moves the object? The state of the copy is undefined, since copying can happen before the move or after the move. So, such actions are kind of undefined behavior since neither a valid or an empty copy cannot be guaranteed.

Even if we guarantee that the tracker would be in one of two possible states after the constructor (which is not the case for the current implementation, since th may become empty while calling th.get_dynamic_state), extra emptiness check would be required on the user side, since providing an empty tracker (or handle) to make_edge is specified as UB.

[akukanov]

That raises the question: should we maybe consider construction from an empty task handle as undefined behavior? In other words, is not it always a mistake when a tracker is constructed from an empty handle? After all, if you want an empty tracker, there is the default constructor.

[kboyarinov]

I agree, that we can treat the construction from an empty task_handle as an error.

[aleksei-fedotov]

I would not say adding new successors does not make any sense. It may have a sense to run these successors immediately (if they do not have other incomplete predecessors), since its predecessor (i.e. the associated task) is completed. This is identical to the situation where completed predecessors can be added with no effect.

[kboyarinov]

Agree, I will improve the wording

[kboyarinov]

Done

[aleksei-fedotov]

In this case also m_new_dynamic_state should point to the receiving state.

I wonder if having only m_successors_list_head is enough. Instead of having "dead" state for this scenario, let it point to the corresponding list in the task_dynamic_state in the task where the successors have been moved. Will it allow not introducing m_new_dynamic_state?

[kboyarinov]

Yes, I agree that we can make m_successors_list_head to point to the new list instead of "dead' state. But we still need some flag to identify that transfer happened, because in two following cases, the state would be the same:

• The task with successors that did not run transfer_successors_to - m_successors_list_head points to the last added successor. The task completion should traverse the list and notify the successors.
• The task that did the transfer - m_successors_list_head also point to the successor, but task completion should not cause signaling and releasing the successors. It is now a responsibility of the recipient task.

Also, I we will need to handle the lifetime issue somehow - current dynamic state should hold a reference to the recipient's dynamic state and release it in the destructor.

[akukanov]

Remember also that we will have transfer "chains": the task (B) that received successors from a task (A) then transfers those to another task (C). In this case, how would the dynamic state of (A) change to point to the successor list of (C)?

[aleksei-fedotov]

Although the included code snippet definitely helps, a sequence diagram describing the algorithm will make the algorithm explanation clearer, from my personal standpoint. Since this is, perhaps, the main algorithm of the whole feature, I would consider adding sequence diagrams for such important pieces in the future.

[kboyarinov]

Done

[aleksei-fedotov]

Maybe I missed something, but it seems that the task_dynamic_state for successor task should be instantiated with two references (the first blue rectangle): one reference representing the dependency, i.e. due to make_edge call, and another one for the corresponding and still alive task_handle object. Otherwise, I'm afraid the successor task might start executing whenever the new_task completes, which might happen before the associated task_handle is submitted. Is my understanding correct?

[kboyarinov]

Your understanding is partially correct. There are two reference counters.
One handles the lifetime of task_dynamic_state (that is shown on the diagram). It holds references for task itself, trackers and other dynamic states that did the transfer.
The second reference counter is in the continuation vertex (it is hidden in the diagram, but mentioned in the highlighted sentence). It holds reference counters for each predecessor task and for the task_hande owning the successor task.
And this second reference counter is initialized with two reference, as you stated, to prevent execution of the successor task before the task_handle is submitted.

[aleksei-fedotov]

What is meant by "post-transfer" actions here? Is it about using the m_new_dynamic_state pointer after transfer_successors_to was made?

[kboyarinov]

Yes, it refers to adding new successors to the same tracker after doing the transfer. I will improve the wording.

[kboyarinov]

Done

[akukanov]

Is this class really needed? Looking at the implementation in #1685, I think it might be replaced with a pointer to reference_vertex in the dynamic state, and with pointers to dynamic states in the successor lists. Seemingly that would result in less allocations, and maybe in less redirections as well.

[kboyarinov]

If we replace continuation vertex in task_dynamic_state with a pointer to reference_vertex and assume it is dynamically allocated, we will have the same amount of allocations and will need to manage the correct allocator usage. Currently task_dynamic_state and continuation_vertex can be allocated by different threads and therefore with different small_object_allocator instances.

In theory, we can replace m_continuation_vertex with the reference counter (or a special vertex if we want to support distributed wait in the future) and exclude dynamic allocations at all.

• Reference counter equal to zero would mean no dependencies. (replaces m_continuation_vertex == nullptr)
• If run detects reference counter that is not equal to zero, it decreases it and if it is the last reference - spawns the task
• The first reserve on the ref counter would reserve additional reference for the task_handle owning the task.

[akukanov]

If we replace continuation vertex in task_dynamic_state with a pointer to reference_vertex and assume it is dynamically allocated, we will have the same amount of allocations and will need to manage the correct allocator usage.

I missed this aspect, thanks for explaining. But then, why cannot task_dynamic_state inherit reference_vertex?

In theory, we can replace m_continuation_vertex with the reference counter (or a special vertex if we want to support distributed wait in the future) and exclude dynamic allocations at all.

That's also an option, but I guess the refcounting machinery is already implemented in reference_vertex and we prefer to reuse it.
Updated: or does reference_vertex have extra details that are redundant for this case - namely, the pointer to the "parent" vertex?

[akukanov]

Is this traversal a potential performance concern, or do we not expect the lists to be big enough for real troubles?

We could in principle add a pointer to the tail of the list to the dynamic state. If extra space is a concern, it could be put into a union with m_new_dynamic_state (if the latter one does not need to be read for proper state determination).

[kboyarinov]

For all of the considered examples, the task had one or two successors to transfer. So in known cases, this should not introduce significant overhead (especially since only a single traverse is performed per transfer).

However, in the general case this could become problematic. I see two possible approaches to address it:

• Apply your proposed changes now to proactively prevent potential issues in the future use cases.
• Keep the current implementation for the preview feature and review how transfer is used in other examples. If we identify real-world scenarios where the traversal have significant cost, we can update the implementation before moving the feature from preview.

[akukanov]

I am fine to keep it as an open question.

[akukanov]

Would it make sense to add a field for runtime versioning of the class, in case we need to change it in the future? If the class size is a concern, I think some space can be taken out of m_num_references, for which 32 bits would be more than enough for any realistic number of references.

BTW, will the TBB scheduler access this class on the hot path in the presence of task dependencies? And if it will, is there risk of "false sharing" and related performance impact?

[akukanov]

~0 and -1 is the same value. Did you mean ~std::uintptr_t(0) - 1?

[kboyarinov]

Yes, tilde was missed. Fixed.

[akukanov]

It might be useful to replace the remaining code blocks with the flow diagram for transitioning task completion.

[kboyarinov]

Done

[akukanov]

As far as I remember, the TBB naming convention for getters that return a value is not to use the prefix. That's what I see in other similar entry points, e.g. current_context and execution_slot. So, this should probably be named current_task.

[kboyarinov]

It was initially named current_task, but I faced issues on Mac systems due to conflict with current_task function implemented as a macro on some versions.
I think it can be changed to something like current_task_ptr or this_task.