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
Improve the rate of thread injection for blocking due to sync-over-async #53471
Improve the rate of thread injection for blocking due to sync-over-async #53471
Changes from all commits
55af247
726dc9e
96cc50e
f3e8a31
d3a977b
c2cb717
20526d6
984888f
56bd4d9
8fc1f8a
1fe2067
e7b9508
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.
There's a lot of policy in here, and a lot of knobs to go with it. Do you have a sense for how all of this is going to behave in the real-world, and if/how someone would utilize these knobs effectively? How did you arrive at this specific set and also the defaults employed?
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.
Some of the criteria used:
MinThreads
as a workaroundThreadsToAddWithoutDelay
to the equivalent and setMaxThreadsToAddBeforeFallback
to something higher to give some buffer for spikes that may need more threadsMaxThreadsToAddBeforeFallback
could also be set to a large value to effectively unlimit the heuristicMaxThreadsToAddBeforeFallback
threads would be created in short order to respond to even a short sync-over-async IO before the IO even completes (if there are that many work items that would block on the async work)MaxThreadsToAddBeforeFallback_ProcCountFactor
of10
came from a prior discussion where we felt that adding 10x the proc count relatively quickly may not be too badThreadsToAddWithoutDelay
andMaxThreadsToAddBeforeFallback
, and perhapsMaxDelayUntilFallbackMs
to control the thread injection latency for spikesThere 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.
Decided to remove
MaxThreadsToAddBeforeFallback
and renamedMaxDelayBeforeFallbackMs
toMaxDelayMs
in the latest commit. The max threads limit before falling back to starvation seems unnecessary, it would be unlimited for starvation anyway. Now the only time it would fall back to starvation is in low-memory situations.