Add reference implementation for parallel_phase feature

[isaevil]

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]

@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.

[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.

[aleksei-fedotov]

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.

[dnmokhov]

LGTM

[akukanov]

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

[vossmjp]

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.

[isaevil]

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.

[akukanov]

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".

[vossmjp]

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

[isaevil]

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

[vossmjp]

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

[isaevil]

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.

[vossmjp]

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

[isaevil]

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

[aleksei-fedotov]

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();

[isaevil]

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

[aleksei-fedotov]

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();

[akukanov]

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.

[aleksei-fedotov]

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.

[akukanov]

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.

[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.

[akukanov]

It's better to use UINTPTR_MAX instead of this cast of -1, here and in other places.

[isaevil]

Done.

[akukanov]

In reset, you only need to drop the one-time flag, correct? Since now it does not intersect with other states, it should be enough to just fetch_and(~ONE_TIME_FAST_LEAVE).

[aleksei-fedotov]

I think the desire is to drop the one-time flag only if a new work is submitted in the empty arena. So, the flag cannot be blindly dropped.

[isaevil]

Well, at first glance it seems like @akukanov suggestion shouldn't contradict the desire. Since unregister_parallel_phase sets this bit only on the last parallel phase completion, when new work submission happens the state will be either still ONE_TIME_FAST_LEAVE, or already include some amount of parallel phase references, which means that this bit will be already dropped by some of the first parallel phases.

[akukanov]

Right, if the arena is not empty, then the flag should have been dropped already, and fetch_and would just do nothing. We can still leave the load and the check if it gives a proven performance advantage, but certainly compare_exchange_strong is not needed.

[isaevil]

Replaced CAS by fetch_and.

[akukanov]

I think it can be simplified as fetch_add(PARALLEL_PHASE) followed by fetch_and(~ONE_TIME_FAST_LEAVE) in case the bit is set in the returned value of fetch_add.

[isaevil]

Seems reasonable to me. Once we have added the reference to parallel phase it should be safe to drop the bit and, just like in case with reset_if_needed method function, trying to drop already dropped flag should do no harm.

[isaevil]

Done.