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] Adding API for parallel block to task_arena to warm-up/retain/release worker threads #1522

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pavelkumbrasev
Copy link
Contributor

@pavelkumbrasev pavelkumbrasev commented Oct 1, 2024

Adding API for parallel block to task_arena to warm-up/retain/release worker threads

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

```cpp
class task_arena {
void indicate_start_of_parallel_block(bool do_warmup = false);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about

void retain_threads();
void release_threads();

*_parallel_block is misleading since even your example shows serial parts of the region.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, I think retain_threads and release_threads provides unnecessary guaranteed like the threads will be actually retained.
Should it be something more relaxed?
Perhaps, make_sticky and make_unsticky suites better because we can set definition of sticky.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do think "sticky" could people think of thread-to-core affinity? Since constraints are used for affinity, I'm thinking the likelihood of confusion is low and so I'm ok with make_sticky and make_unsticky.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should not all this be about work rather than threads? Threads are the execution resources, that should not be exposed to the user, should they? I mean that is the original idea of the TBB library. Therefore, I suggest something like expect_[more/less_]parallel_work or assume_[more/less_]parallelism as a good level of a loose terminology what library should tend to "think" about user's code when this API is 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.

TBB exposes some level of "thread logic" with observers. I'm not sure whether this API should expose this logic too.
If we want to extend these functions with additional guarantees such as "warm-up" or "leave earlier" perhaps we could not ignore threads completely.

namespace this_task_arena {
void indicate_start_of_parallel_block(bool do_warmup = false);
void indicate_end_of_parallel_block(bool disable_default_block_time = false);
void disable_default_block_time();
Copy link
Contributor

Choose a reason for hiding this comment

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

The end-user doesn't know what the default block time is, and it will be platform dependent. The first set of functions indicate of region of interest, these default_block_time functions change a property on the task_arena that is not tied to a region. That makes me think it is better as a constraint. Are there known cases where this needs to be disabled then reenabled dynamically?

If the first two functions became something like retain_threads and release_threads, what would these be named? What about set_sleep_policy( sleep_quickly | sleep_slowly ) or something like 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.

Perhaps, it is a good idea to move it constraints. If you need different guarantees just use different arenas the same is applicable to priorities.
If we include the property as part of the constraints which in turn represent HW Resources perhaps the name should represent how these resources will be used, like greedy or something.

@akukanov what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

A couple questions:

  • How would users think about "greedy" relative to per-arena priorities? It might imply some kind of priority between a greedy normal arena and non-greedy normal arena, even though that wouldn't be the case.
  • In suggesting sleep_quickly and sleep_slowly, which I admit are not great names, I was trying to find something that indicated more about the wastefulness of holding onto resources once you have them and while there's nothing better to do with them in contrast to a greediness in acquiring resources, perhaps from some other competing arenas. I think this is the key point of disable|enable_default_block_time -- while it is a form of greediness, it is more about the amount of wastefulness tolerated to reduce startup latency on the next parallel algorithm.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I am thinking about this, thoughts about being nice/responsive to the demand from other arenas appear in my head. But I am not sure how to better combine these two sets of API as they kind of mutually exclusive to each other. Consider, expect more parallel work to appear but be responsive to resources demand from other arenas. Actually, this counterintuition applies to the proposed design as well. Perhaps, we need to express that mutual exclusiveness somehow in the API.

Copy link
Contributor Author

@pavelkumbrasev pavelkumbrasev Oct 7, 2024

Choose a reason for hiding this comment

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

expect more parallel work to appear but be responsive to resources demand from other arenas.

What part of the proposal is stating this? (Expects both properties simultaneously)

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 not stating explicitly, but sort of implying the question What will it mean if I invoke indicate_start_of_parallel_block and call disable_default_block_time right after that?

@vossmjp vossmjp changed the title Adding API for parallel block to task_arena to warm-up/retain/release worker threads [RFC} Adding API for parallel block to task_arena to warm-up/retain/release worker threads Oct 3, 2024
@vossmjp vossmjp changed the title [RFC} Adding API for parallel block to task_arena to warm-up/retain/release worker threads [RFC] Adding API for parallel block to task_arena to warm-up/retain/release worker threads Oct 3, 2024
Copy link
Contributor

@aleksei-fedotov aleksei-fedotov left a comment

Choose a reason for hiding this comment

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

Overall, it looks as too certain about the things that will or will not happen when the new API is utilized. I think that the explanation should be written in a more vague terms using the more of "may", "might", etc. words. Essentially, conveying the idea that all this is up to the implementation and serve as a hint rather than a concrete behavior.

What do others think?

rfcs/proposed/parallel_block_for_task_arena/README.md Outdated Show resolved Hide resolved
rfcs/proposed/parallel_block_for_task_arena/README.md Outdated Show resolved Hide resolved
rfcs/proposed/parallel_block_for_task_arena/README.md Outdated Show resolved Hide resolved
rfcs/proposed/parallel_block_for_task_arena/README.md Outdated Show resolved Hide resolved
rfcs/proposed/parallel_block_for_task_arena/README.md Outdated Show resolved Hide resolved
rfcs/proposed/parallel_block_for_task_arena/README.md Outdated Show resolved Hide resolved
rfcs/proposed/parallel_block_for_task_arena/README.md Outdated Show resolved Hide resolved
rfcs/proposed/parallel_block_for_task_arena/README.md Outdated Show resolved Hide resolved

```cpp
class task_arena {
void indicate_start_of_parallel_block(bool do_warmup = false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not all this be about work rather than threads? Threads are the execution resources, that should not be exposed to the user, should they? I mean that is the original idea of the TBB library. Therefore, I suggest something like expect_[more/less_]parallel_work or assume_[more/less_]parallelism as a good level of a loose terminology what library should tend to "think" about user's code when this API is used.

namespace this_task_arena {
void indicate_start_of_parallel_block(bool do_warmup = false);
void indicate_end_of_parallel_block(bool disable_default_block_time = false);
void disable_default_block_time();
Copy link
Contributor

Choose a reason for hiding this comment

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

When I am thinking about this, thoughts about being nice/responsive to the demand from other arenas appear in my head. But I am not sure how to better combine these two sets of API as they kind of mutually exclusive to each other. Consider, expect more parallel work to appear but be responsive to resources demand from other arenas. Actually, this counterintuition applies to the proposed design as well. Perhaps, we need to express that mutual exclusiveness somehow in the API.

@pavelkumbrasev
Copy link
Contributor Author

Overall, it looks as too certain about the things that will or will not happen when the new API is utilized. I think that the explanation should be written in a more vague terms using the more of "may", "might", etc. words. Essentially, conveying the idea that all this is up to the implementation and serve as a hint rather than a concrete behavior.

What do others think?

I tried to indicate that this set of APIs is a hint to the scheduler. But if you believe that we can relax this guarantees even more I think we should do this.

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

Successfully merging this pull request may close these issues.

3 participants