8000 Add implementation details information for task group dynamic dependencies by kboyarinov · Pull Request #1743 · uxlfoundation/oneTBB · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add implementation details information for task group dynamic dependencies #1743

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: dev/kboyarinov/tg-rfc-dynamic-dependencies-r2
Choose a base branch
from
Open

Add implementation details information for task group dynamic dependencies #1743

wants to merge 12 commits into from
+862 −0

Conversation

kboyarinov
Copy link
Contributor

Description

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

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

@kboyarinov kboyarinov added the RFC label May 30, 2025
@kboyarinov kboyarinov added this to the 2022.3.0 milestone May 30, 2025
dnmokhov
dnmokhov reviewed May 30, 2025
View reviewed changes
kboyarinov and others added 4 commits June 2, 2025 11:15
@kboyarinov @dnmokhov
Apply suggestions from code review
9d8283b 
Co-authored-by: Dmitri Mokhov <dmitri.n.mokhov@intel.com>
@kboyarinov
Merge remote-tracking branch 'origin/dev/kboyarinov/tg-rfc-dynamic-de…
f5794dd 
…pendencies-r2' into dev/kboyarinov/tg-dynamic-dependencies-impl-rfc
@kboyarinov
Add link to the main RFC
60dc8ad
@kboyarinov
Merge branch 'dev/kboyarinov/tg-dynamic-dependencies-impl-rfc' of htt…
6aba047 
…ps://github.com/oneapi-src/oneTBB into dev/kboyarinov/tg-dynamic-dependencies-impl-rfc
vossmjp
vossmjp reviewed Jun 2, 2025
View reviewed changes
vossmjp
vossmjp reviewed Jun 2, 2025
View reviewed changes
rfcs/proposed/task_group_dynamic_dependencies/implementation_details.md Outdated

The stored reference counter is increased when:
* `task_dynamic_state` associated with the task is created, the reference is reserved for the task object.
* `task_tracker` instance for the task is created.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is legal to have more than one task_tracker for a task, I think the added "a" implies that there's not just one task_tracker but that you increment the reference count whenever a task_tracker is create. Although maybe it should be made more clear that multiple trackers are allowed for the same task.

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 changed it to be "a" and mentioned explicitly that the instance can be a new tracker or a copy.

rfcs/proposed/task_group_dynamic_dependencies/implementation_details.md Outdated
The stored reference counter is decreased when:
* The associated task is completed, before destroying the task instance
8000 * `task_tracker` instance associated with the task is destroyed
* `task_dynamic_state` of the task that did `transfer_successors_to(new_task)` while executing is destroyed. In this case, the reference
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe its addressed later, but can two tasks transfer successors to the same new_task? If so, "the" should be "a".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with "a"

dnmokhov
dnmokhov reviewed Jun 3, 2025
View reviewed changes
vossmjp
vossmjp reviewed Jun 3, 2025
View reviewed changes
rfcs/proposed/task_group_dynamic_dependencies/implementation_details.md Outdated

`m_task` is a pointer to the task, with which the current dynamic state is associated.

`m_successors_list_head` is an atomic pointer to the head of the successor's list of the currently served task. It is also used
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

rfcs/proposed/task_group_dynamic_dependencies/implementation_details.md
The stored reference counter is increased when:
* `task_dynamic_state` associated with the task is created, the reference is reserved for the task object.
* a `task_tracker` instance for the task is created (both non-empty tracker created from `task_handle` and copy of the existing one).
* `transfer_successors_to(new_task)` is called, the dynamic state of the currently executing task reserves a reference on the
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
* F438 `transfer_successors_to(new_task)` is called, the dynamic state of the currently executing task reserves a reference on the
* `transfer_successors_to(new_task)` is called, the dynamic state of the associated task reserves a reference on the

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 would keep currently executing task here since in other places the task associated to the corresponding dynamic state was described. And here the dynamic state associated to the currently executing task (the task that called transfer_successors_to in the body).

rfcs/proposed/task_group_dynamic_dependencies/implementation_details.md Outdated
When the task `pred1` completes execution, it traverses it's successors list and decreasing the reference counter stored in the vertex
of `succ1` and `succ2`.

When `pred2` traverse it's successors list, it decreases the reference counters in `succ1` and `succ2`. Both of them are equal to `0` now and hence
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably unimportant but you are assuming that pred1 completes first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an explicit mention

kboyarinov and others added 2 commits June 4, 2025 15:56
@kboyari 10000 nov @dnmokhov @vossmjp
Apply suggestions from code review
d689c3c 
Co-authored-by: Dmitri Mokhov <dmitri.n.mokhov@intel.com>
Co-authored-by: Mike Voss <michaelj.voss@intel.com>
@kboyarinov
Add state transition examples
3b1b155
dnmokhov
dnmokhov reviewed Jun 10, 2025
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vossmjp vossmjp vossmjp left review comments

@dnmokhov dnmokhov dnmokhov left review comments

@aleksei-fedotov aleksei-fedotov Awaiting requested review from aleksei-fedotov

@akukanov akukanov Awaiting requested review from akukanov

@isaevil isaevil Awaiting requested review from isaevil

Assignees
No one assigned
Labels
RFC
Projects
None yet
Milestone
2022.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@kboyarinov @vossmjp @dnmokhov
0