Add reference implementation for parallel_phase feature #1570

isaevil · 2024-11-26T16:31:50Z

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

isaevil · 2024-11-27T09:58:20Z

@akukanov @aleksei-fedotov @vossmjp Could you please take a look at the PR in terms of implementation and ABI.

P.S. Tests are still WIP.

aleksei-fedotov

Not a complete review, just a couple of starters to think about.

src/tbb/waiters.h

src/tbb/arena.h

include/oneapi/tbb/task_arena.h

aleksei-fedotov

A bunch of comments from me. Have not looked at the tests yet. Also, not finished reviewing the PHASE_* switching logic because of found issue.

include/oneapi/tbb/task_arena.h

src/tbb/arena.h

src/tbb/arena.cpp

src/tbb/arena.h

src/tbb/waiters.h

include/oneapi/tbb/task_arena.h

src/tbb/arena.cpp

src/tbb/arena.h

test/tbb/test_task_arena.cpp

aleksei-fedotov · 2024-12-15T23:35:14Z

test/tbb/test_task_arena.cpp

In addition, I think need to cover in tests the following:

The DELAYED_LEAVE is returned back once new task is submitted.

Here I think we can test that the return of a worker after one-time-fast-leave happens generally longer than the following returns made after new task(s) is/are submitted.

Nested parallel phases with combinations of DELAYED and (ONE TIME) FAST leaves.

src/tbb/waiters.h

Signed-off-by: pavelkumbrasev <[email protected]>

Signed-off-by: Isaev, Ilya <[email protected]>

…rallel_block Signed-off-by: Isaev, Ilya <[email protected]>

Signed-off-by: Isaev, Ilya <[email protected]>

…rena Signed-off-by: Isaev, Ilya <[email protected]>

dnmokhov

LGTM

akukanov

My comments below are not critical; feel free to commit as-is and address later.

include/oneapi/tbb/task_arena.h

src/tbb/arena.h

vossmjp · 2025-01-14T19:23:59Z

include/oneapi/tbb/task_arena.h

@@ -95,6 +95,11 @@ TBB_EXPORT void __TBB_EXPORTED_FUNC isolate_within_arena(d1::delegate_base& d, s
 TBB_EXPORT void __TBB_EXPORTED_FUNC enqueue(d1::task&, d1::task_arena_base*);
 TBB_EXPORT void __TBB_EXPORTED_FUNC enqueue(d1::task&, d1::task_group_context&, d1::task_arena_base*);
 TBB_EXPORT void __TBB_EXPORTED_FUNC submit(d1::task&, d1::task_group_context&, arena*, std::uintptr_t);
+
+#if __TBB_PREVIEW_PARALLEL_PHASE
+TBB_EXPORT void __TBB_EXPORTED_FUNC register_parallel_phase(d1::task_arena_base*, std::uintptr_t);


Is there a reason to use "register" and "unregister" in the names for the entry points? To me "registration" does not imply the start and end of the active time in the region, but might be used to indicate that a region just exists. For example, an athlete might register for a race but that doesn't mean they've started running.

I think the main idea was to use more generic names, which do not depend on feature API. So, if API will change dramatically, entry point names would still make some sense.

There might be alternative suitable generic verbs, such as "initiate", "enter", "commence", "launch", "open" for the beginning and "complete", "conclude", "close", "finish", "exit".

From the semantical point of view (i.e. speaking of a program execution phase), I would likely use "enter/exit" or "initiate/complete"; but since it's an internal entry point, I am OK with any choice made, including "register/unregister".

Of the suggestions, I like "enter/exit".

@akukanov @vossmjp Renamed entry points to "enter/exit".

vossmjp · 2025-01-14T21:10:51Z

test/tbb/test_parallel_phase.cpp

+    auto median_automatic = utils::median(times_automatic.begin(), times_automatic.end());
+    auto median_fast = utils::median(times_fast.begin(), times_fast.end());
+
+    WARN_MESSAGE(median_automatic < median_fast,


Are all of the tests guaranteed to work (i.e. not have warnings) for hybrid systems where automatic is not delayed leave?

That is a good question. I suppose that only one test case(Parallel Phase retains workers in task_arena) would produce expected results on hybrid systems. Other tests don't make much sense on such configurations, but I think it is fine since these are warnings, not asserts.

Can we skip the warning on hybrid systems? For example, don't output warning if there is more than 1 core type.

Co-authored-by: Alexey Kukanov <[email protected]> Signed-off-by: Isaev, Ilya <[email protected]>

Signed-off-by: Isaev, Ilya <[email protected]>

isaevil · 2025-01-15T17:44:58Z

@aleksei-fedotov @akukanov @vossmjp @pavelkumbrasev @dnmokhov I have updated the RFC document: included some technical details and conditions to leave the experimental stage.

Signed-off-by: Isaev, Ilya <[email protected]>

aleksei-fedotov · 2025-01-20T15:30:51Z

src/tbb/arena.h

@@ -433,6 +528,9 @@ void arena::advertise_new_work() {
            workers_delta = 1;
        }

+#if __TBB_PREVIEW_PARALLEL_PHASE
+        my_thread_leave.reset_if_needed();


Will the following assert be useful?

Suggested change

my_thread_leave.reset_if_needed();

__TBB_ASSERT(mandatory_delta > 0 || workers_delta > 0, "Potentially delaying fast leave?");

my_thread_leave.reset_if_needed();

It is not a valid assert. In case of workerless arena and spawn of the task both of those values will be zeroes.

Okay... let it be then:

Suggested change

my_thread_leave.reset_if_needed();

__TBB_ASSERT((mandatory_delta > 0 || workers_delta > 0) ||

(is_arena_workerless() && work_type == work_spawned),

"Potentially delaying fast leave?");

my_thread_leave.reset_if_needed();

First of all, I do not readily see how this assert is related to the parallel phase.
And if you move the assertion next to the code in the lines 522-529, it seems it just duplicates almost exactly the conditions and assignments there.

Yeah, I know that it duplicates. But the stress here is on the words "almost exactly". With the assert I sort of wanted to make sure we do not fall into recently discovered issue of dropping the fast leave setting when threads are recalled from arena. I don't mind not having an assert here, since, as you said, the conditions duplicate it. However, I do think we need to tell reader about the desired behavior here, it feels like the code itself might not be good at telling that.

Another interesting consideration is to reduce cross-NUMA accesses as much as possible. If thread can base its decision whether it needs to read and/or modify global-to-arena variables on the data it has already read, then it makes sense to avoid doing more re-reads of other global-to-arena data. Of course, if we are sure that the set of "almost exactly" is empty here, then the thread already avoids unnecessary touches to the data that increases cross-NUMA traffic.

Assertions are good to enforce preconditions for a function, and in some cases of sophisticated internal logic also good to check expected invariants in certain point. Having an assertion that checks the conditions explicitly set by the code right above is a pure duplication. The part that is not an exact duplication could be checked in a proper place, of course; also reset_if_needed could internally assert on its expected preconditions.

For the NUMA part of the comment, I cannot make the connection to the assertion being discussed.

src/tbb/arena.h

include/oneapi/tbb/task_arena.h

Signed-off-by: Isaev, Ilya <[email protected]>

aleksei-fedotov

Looks good to me.

akukanov

I think that might also simplify the implementation logic.

Below is how. Note that separation of significant bits for different leave states was a necessary prerequisite to operate with ONE_TIME_FAST_LEAVE as a single bit flag.

src/tbb/arena.h

Signed-off-by: Isaev, Ilya <[email protected]>

akukanov · 2025-01-22T17:10:23Z

src/tbb/arena.h

+            __TBB_ASSERT(prev - PARALLEL_PHASE < prev,
+                         "A call to unregister without its register complement");
+            desired = prev - PARALLEL_PHASE; // Mark the end of this phase in reference counter
+            if (enable_fast_leave && /*it was the last parallel phase*/desired == DELAYED_LEAVE) {


A bit of nitpicking: the comment does not really match the condition tested. To check if that's the last parallel phase, you'd use desired < PARALLEL_PHASE. Here you also check that neither of the fast-leave flags is set.
Perhaps it can be improved like this:

Suggested change

if (enable_fast_leave && /*it was the last parallel phase*/desired == DELAYED_LEAVE) {

if (enable_fast_leave && /*it was the last parallel phase, and delayed leave is the default*/ desired == DELAYED_LEAVE) {

But also feel free to ignore :)

vossmjp

Looks good to me. At some point we need to improve tests though.

github-actions bot added enhancement tests labels Nov 26, 2024

isaevil changed the title ~~Add reference implementation of parallel_block feature~~ Add reference implementation for parallel_block feature Nov 26, 2024

isaevil requested review from aleksei-fedotov, akukanov and dnmokhov November 27, 2024 09:56

aleksei-fedotov reviewed Nov 28, 2024

View reviewed changes

src/tbb/waiters.h Outdated Show resolved Hide resolved

src/tbb/arena.h Outdated Show resolved Hide resolved

src/tbb/arena.h Outdated Show resolved Hide resolved

include/oneapi/tbb/task_arena.h Outdated Show resolved Hide resolved

akukanov reviewed Dec 2, 2024

View reviewed changes

include/oneapi/tbb/task_arena.h Outdated Show resolved Hide resolved

dnmokhov reviewed Dec 4, 2024

View reviewed changes

include/oneapi/tbb/task_arena.h Outdated Show resolved Hide resolved

isaevil changed the title ~~Add reference implementation for parallel_block feature~~ Add reference implementation for parallel_phase feature Dec 4, 2024

aleksei-fedotov reviewed Dec 6, 2024

View reviewed changes

akukanov reviewed Dec 10, 2024

View reviewed changes

include/oneapi/tbb/task_arena.h Outdated Show resolved Hide resolved

include/oneapi/tbb/task_arena.h Outdated Show resolved Hide resolved

isaevil marked this pull request as ready for review December 13, 2024 15:19

aleksei-fedotov reviewed Dec 15, 2024

View reviewed changes

isaevil requested review from vossmjp, aleksei-fedotov, akukanov, pavelkumbrasev and dnmokhov December 17, 2024 11:21

pavelkumbrasev and others added 10 commits December 17, 2024 12:28

Add initial impl of Parallel Block

5e5778e

Signed-off-by: pavelkumbrasev <[email protected]>

Add test for workers_leave

55fd835

Signed-off-by: pavelkumbrasev <[email protected]>

Fix thread_leave_manager

637a74d

Signed-off-by: pavelkumbrasev <[email protected]>

Add parallel_block API, improve tests

eb172a7

Signed-off-by: Isaev, Ilya <[email protected]>

Improve tests for parallel_block

7f08af0

Signed-off-by: Isaev, Ilya <[email protected]>

Add workers_leave::automatic, let hybric systems take advantage of pa…

f6a139a

…rallel_block Signed-off-by: Isaev, Ilya <[email protected]>

Fix Windows compilation

0a9faed

Signed-off-by: Isaev, Ilya <[email protected]>

Add win32 exports

c1802a1

Signed-off-by: Isaev, Ilya <[email protected]>

Add mac64 symbols

ecd5a9e

Signed-off-by: Isaev, Ilya <[email protected]>

Correct mask, add a clarifying comment to it

0d32f80

Signed-off-by: Isaev, Ilya <[email protected]>

Decrease the size of dummy work, don't execute tests for workerless a…

6a951bd

…rena Signed-off-by: Isaev, Ilya <[email protected]>

dnmokhov previously approved these changes Dec 20, 2024

View reviewed changes

akukanov reviewed Dec 23, 2024

View reviewed changes

src/tbb/arena.h Outdated Show resolved Hide resolved

isaevil dismissed dnmokhov’s stale review via c05d9a7 January 14, 2025 11:36

vossmjp reviewed Jan 14, 2025

View reviewed changes

isaevil and others added 2 commits January 15, 2025 15:26

Apply suggestions from code review

9d507ca

Co-authored-by: Alexey Kukanov <[email protected]> Signed-off-by: Isaev, Ilya <[email protected]>

Improve test stability

515fd33

Signed-off-by: Isaev, Ilya <[email protected]>

isaevil force-pushed the dev/pavelkumbrasev/parallel_block branch from c05d9a7 to 515fd33 Compare January 15, 2025 14:34

isaevil added 4 commits January 15, 2025 16:07

Use uintptr_t

0f233bb

Signed-off-by: Isaev, Ilya <[email protected]>

Update copyright years

15c3ea5

Signed-off-by: Isaev, Ilya <[email protected]>

Reduce testing time

f041f3a

Signed-off-by: Isaev, Ilya <[email protected]>

Move RFC to the experimental stage

fdae151

Signed-off-by: Isaev, Ilya <[email protected]>

isaevil requested review from akukanov, vossmjp, aleksei-fedotov and dnmokhov January 15, 2025 17:41

Fix one time fast leave

a178e85

Signed-off-by: Isaev, Ilya <[email protected]>

aleksei-fedotov reviewed Jan 20, 2025

View reviewed changes

kboyarinov reviewed Jan 20, 2025

View reviewed changes

src/tbb/arena.h Outdated Show resolved Hide resolved

include/oneapi/tbb/task_arena.h Show resolved Hide resolved

Fix sources format

0f1b1a5

Signed-off-by: Isaev, Ilya <[email protected]>

aleksei-fedotov previously approved these changes Jan 20, 2025

View reviewed changes

akukanov reviewed Jan 21, 2025

View reviewed changes

src/tbb/arena.h Outdated Show resolved Hide resolved

src/tbb/arena.h Show resolved Hide resolved

src/tbb/arena.h Outdated Show resolved Hide resolved

src/tbb/arena.h Outdated Show resolved Hide resolved

src/tbb/arena.h Show resolved Hide resolved

Rename entry points, simplify state machine interaction

c38eb2f

Signed-off-by: Isaev, Ilya <[email protected]>

isaevil dismissed aleksei-fedotov’s stale review via c38eb2f January 22, 2025 11:23

isaevil requested review from aleksei-fedotov and akukanov January 22, 2025 12:58

akukanov reviewed Jan 22, 2025

View reviewed changes

vossmjp approved these changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reference implementation for parallel_phase feature #1570

Add reference implementation for parallel_phase feature #1570

isaevil commented Nov 26, 2024

isaevil commented Nov 27, 2024 •

edited

Loading

aleksei-fedotov left a comment

aleksei-fedotov left a comment

aleksei-fedotov Dec 15, 2024

dnmokhov left a comment

akukanov left a comment

vossmjp Jan 14, 2025

isaevil Jan 15, 2025

akukanov Jan 21, 2025 •

edited

Loading

vossmjp Jan 22, 2025

isaevil Jan 22, 2025

vossmjp Jan 14, 2025

isaevil Jan 15, 2025 •

edited

Loading

vossmjp Jan 22, 2025

isaevil commented Jan 15, 2025

aleksei-fedotov Jan 20, 2025

isaevil Jan 20, 2025

aleksei-fedotov Jan 20, 2025

akukanov Jan 21, 2025

aleksei-fedotov Jan 21, 2025

akukanov Jan 21, 2025 •

edited

Loading

aleksei-fedotov left a comment

akukanov left a comment

akukanov Jan 22, 2025 •

edited

Loading

vossmjp left a comment

	my_thread_leave.reset_if_needed();
	__TBB_ASSERT(mandatory_delta > 0 \|\| workers_delta > 0, "Potentially delaying fast leave?");
	my_thread_leave.reset_if_needed();

-        my_thread_leave.reset_if_needed();
+        __TBB_ASSERT((mandatory_delta > 0 || workers_delta > 0) ||
+                     (is_arena_workerless() && work_type == work_spawned),
+                     "Potentially delaying fast leave?");
+        my_thread_leave.reset_if_needed();

	if (enable_fast_leave && /it was the last parallel phase/desired == DELAYED_LEAVE) {
	if (enable_fast_leave && /it was the last parallel phase, and delayed leave is the default/ desired == DELAYED_LEAVE) {

Add reference implementation for parallel_phase feature #1570

Are you sure you want to change the base?

Add reference implementation for parallel_phase feature #1570

Conversation

isaevil commented Nov 26, 2024

Description

Type of change

Tests

Documentation

Breaks backward compatibility

Notify the following users

Other information

isaevil commented Nov 27, 2024 • edited Loading

aleksei-fedotov left a comment

Choose a reason for hiding this comment

aleksei-fedotov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnmokhov left a comment

Choose a reason for hiding this comment

akukanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akukanov Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaevil Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaevil commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akukanov Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

aleksei-fedotov left a comment

Choose a reason for hiding this comment

akukanov left a comment

Choose a reason for hiding this comment

akukanov Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

vossmjp left a comment

Choose a reason for hiding this comment

isaevil commented Nov 27, 2024 •

edited

Loading

akukanov Jan 21, 2025 •

edited

Loading

isaevil Jan 15, 2025 •

edited

Loading

akukanov Jan 21, 2025 •

edited

Loading

akukanov Jan 22, 2025 •

edited

Loading