[release/11.0] Increase spinning/polling aggressiveness in the thread pool in low-saturation scenarios - #132908
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @VSadov |
32fd95b to
4560f33
Compare
JulieLeeMSFT
added
Servicing-approved
There was a problem hiding this comment.
🟡 Changes recommended
The new default threshold computation narrows an int processor-count-based value to short without clamping, which can overflow on large core-count machines and destabilize the heuristic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adjusts ThreadPool worker parking/spinning heuristics and semaphore behavior to be less aggressive in low-saturation/bursty scenarios, aiming to avoid latency and throughput regressions caused by parking workers too readily after spurious dispatches.
Changes:
- Add a configurable threshold to only park immediately after spurious dispatches when enough other workers are still processing work.
- Increase the default LIFO semaphore spin limit and ensure remaining signals eventually wake additional waiters.
- Reduce exponential backoff maximum per-iteration spin wait (tighter cap) and update the associated comment.
File summaries
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs | Adds spurious-dispatch no-spin threshold logic and threads-throughput heuristic tweak. |
| src/libraries/System.Private.CoreLib/src/System/Threading/LowLevelLifoSemaphore.cs | Increases default spin count and ensures additional signals can trigger further waiter wakes. |
| src/libraries/System.Private.CoreLib/src/System/Threading/Backoff.cs | Lowers max exponential backoff bits to reduce per-iteration spin duration cap. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
…turation scenarios (#132765) We made some changes in the threadpool to reduce spinning. In particular to reduce fruitless spinning - when a worker thread scanned through the work queue and found no work whatsoever. We would park such thread as a matter of throttling pointless scanning. The change helped in high saturation scenarios as reducing spurious scans reduces waste and lets other threads do useful work. Unfortunately, in some low-saturation scenarios those spurious scans were load bearing. In such scenarios some redundancy in terms of spurious scans must be tolerated to provide good latency. If we park workers too aggressively when we do not have many workers in the first place we will need to rely on waking them up to serve incoming requests. In a bursty case this could be a noticeable regression. In bursty ping-pong kind of scenario, if this happens on both the app and the client ends, the result could be amplified further. Here we are tuning the heuristic that parks threads after spurious scans to be enabled only when we have more than 2/3 of the proc count workers. There could be better ways to make use of this signal selectively and we should explore further. This is a simple enough change that we can do for net11. The change also increases allowed spin time and lowers the max delay between polls to cap the impact on latency from longer spin, if such happens. (it makes sense to have per iteration cap lower than the total, we had it the other way) --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
93b40ca to
802d9d1
Compare
|
All CI failures on this PR are known, pre-existing flaky socket tests — none are related to the thread pool change. Failing tests (all
All fail with Tracking issue: #131990 — "TcpReceiveSendGetsCanceledByDispose timed out" (open, Note: the Related/duplicate reports of the same failure: #124079, #84364, #56580. |
|
/ba-g unrelated known issues. |
Backport of #132765 to release/11.0
/cc @VSadov
Customer Impact
Too aggressive parking of threadpool threads in response to spurious wake ups may result in having too few active workers in bursty low saturation scenarios and cause massive regressions.
Some tests like Websockets show nearly 4x less RPS compared to net10.
Regression
Introduced in #128606
The change resultied in improvements in high saturation throughput-sensitive scenarios. It was later discovered that in low staturation latency-sensitive scenarios it resulted in regressions.
Testing
The fix was validated with various benchmarks both high-saturation and low, on both x64 and arm.
Risk
Low.
This is an intentionally small change. The new parking policy will only be enabled when the threadpool has more than 2/3 proc count workers.