thread: optimize maybe_yield #2574
Merged
+8
−13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thread::maybe_yield, a static method, calls into thread_impl::maybe_yield which is a member function on the current thread context, which means we need to access the TLS etc, but this function is itself essentially static as it only calls needs_prempt(). This was probably different in the ancient past where there were thread-specific scheduling groups, etc. This method is performance sensitive as it is often called in loops, even tight loops as it is often difficult to call it less frequently in a tight loop which may have a large trip count. Therefore it is worth optimizing. So this change: - Have thread::maybe_yield just call needs_prempt() directly - Move maybe_yield and should_yield in to the header. This latter change doesn't change needed includes, so shouldn't impact compile time. The yield() function itself is still out-of-line. This provides a chance for the compile to hoist the TLS access boilerplate out of loops.
|
This came up while looking at metrics endpoint performance but decided to split it out alone since it's not really specific to metrics. |
avikivity
reviewed
Dec 11, 2024
|
|
||
| /// \brief Yield if this thread ought to call yield() now. | ||
| /// | ||
| /// Useful where a code does long running computation and does | ||
| /// not want to hog cpu for more then its share | ||
| static void maybe_yield(); | ||
| static void maybe_yield() { | ||
| if (should_yield()) [[unlikely]] { |
There was a problem hiding this comment.
Note: unlikely is unnecessary (but not harmful) since need_preempt() is annotated with __builtin_expect.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
thread::maybe_yield, a static method, calls into
thread_impl::maybe_yield which is a member function on the current thread context, which means we need to access the TLS etc, but this function is itself essentially static as it only calls needs_prempt().
This was probably different in the ancient past where there were thread-specific scheduling groups, etc.
This method is performance sensitive as it is often called in loops, even tight loops as it is often difficult to call it less frequently in a tight loop which may have a large trip count. Therefore it is worth optimizing.
So this change:
This latter change doesn't change needed includes, so shouldn't impact compile time. The yield() function itself is still out-of-line. This provides a chance for the compile to hoist the TLS access boilerplate out of loops.
I also add an [[unlikely]] annotation in the direction of "not yielding".