Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
[RFC] Adding API for parallel block to task_arena to warm-up/retain/release worker threads #1522
Changes from 1 commit
215a17d
74bf599
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
andrelease_threads
provides unnecessary guaranteed like the threads will be actually retained.Should it be something more relaxed?
Perhaps,
make_sticky
andmake_unsticky
suites better because we can set definition ofsticky
.There was a problem hiding this comment.
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
andmake_unsticky
.There was a problem hiding this comment.
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
orassume_[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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
andrelease_threads
, what would these be named? What aboutset_sleep_policy( sleep_quickly | sleep_slowly )
or something like that.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple questions:
sleep_quickly
andsleep_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 ofdisable|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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What part of the proposal is stating this? (Expects both properties simultaneously)
There was a problem hiding this comment.
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 calldisable_default_block_time
right after that?