Repository navigation
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. |
dotnet-policy-service
Bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @VSadov |
|
|
||
| internal void EnsureWorkerRequested() | ||
| { | ||
| if (ThreadPoolWorkQueueThreadLocals.threadLocals == null) |
There was a problem hiding this comment.
Here we are not using if (!Thread.CurrentThread.IsThreadPoolThread) because that would miss some external producers like the IO completion pollers.
There was a problem hiding this comment.
🔵 Needs a closer look
The timing race needs correction, and the shared scheduling heuristic requires human validation of burst latency and multi-producer contention.
1 open issue
What changed in this PR
Reduces empty-dispatch spinning in the portable thread pool to address runtime-async performance regressions while preserving responsiveness to external bursts.
Changes:
- Tracks external worker requests, including coalesced requests.
- Skips spinning after empty dispatches when external activity is no longer recent.
Builds, tests, and benchmarks were not run.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.WorkerThread.cs | Uses external-request age to decide whether to skip spinning. |
| src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.cs | Records the latest external-request timestamp. |
Review effort: Balanced
|
This may need quite a bit of testing. |
|
Sorry, closed by accident. (The page moved and a wrong button was clicked) |
Agreed. I have the benchmark numbers in the linked issue comments, and it doesn't seem to regress the workload the prior PR was addressing. But I think it still worth to run the tests in your end as I don't have the exact environment and workload. |
| // which can introduce another burst after the queues drain. For work produced entirely by | ||
| // workers, park empty dispatches promptly instead of competing with the productive workers. | ||
| return spurious && (numProcessingWork > SpuriousDispatchNoSpinThreshold || | ||
| (Environment.TickCount - Volatile.Read(ref threadPoolInstance._separated.lastExternalWorkItemTimeMs)) |
There was a problem hiding this comment.
Assuming there won't be a case where no external producer updates lastExternalWorkItemTimeMs for more than Int32.MaxValue milliseconds.
|
113 complete A/B pairs across 41 comparisons, including the 12 additional valid pairs. Changes are PR/main paired geometric means. Higher throughput is better; lower CPU/op and p99 are better. S = original server-only cohort; B = both server and client use main/main versus PR/PR; R = separate server-only retry cohort. S managed clients stay on stock .NET 11; HTTP load generation uses unchanged native
Largest movementsAt least one of throughput, server CPU/op, or p99 moved by 5% or more. Sorted by the largest absolute point estimate, not by confidence.
Remaining resultsAll three primary point estimates moved by less than 5%.
N/A means unusable latency percentiles, not measured zero latency. CPU/op is measured-window server CPU per request/message, not peak CPU. Percentile changes compare per-run p99 values; they are not pooled percentiles. |
|
On crank benchmarks the change worked better than I expected. Thread pool has conflicting requirements on whether to use more aggressive parking/spinning policy that improves latencies or more frugal regime that avoids waste. Whether requests are entering threadpool externally of from workers appears to be an interesting signal to consider. There are some cases where latencies got much worse. Sometimes leading to higher throughput, sometimes not. It is interesting to find why - maybe the signal is still useful and just the response needs to be tuned a bit. |
|
It seems that the extra clock read after an empty dispatch was adding unnecessary overhead, so I updated the implementation to reuse the timestamp already recorded for dispatch progress instead of calling Here are the new benchmark results against the same workloads. Runtime async is on Windows x64, and the network workloads are on Linux x64. Runtime async:
HTTP and WebSocket results, shown as without PR → with PR (change). Low = 4 connections, high = 100 connections; WebSocket uses 1,000-byte echo with compression disabled. S = server updated, B = both server and client updated. The longer runs use a 15-second warmup, with 30 seconds measured for WebSocket and 45 seconds for JSON.
SignalR broadcast diagnostic (100 connections, backpressure restored; overloaded, so this is not a reliable comparison of the original workload):
It seems that with this approach the runtime async suspension cases take 66–79% less time, while the four synchronous/completed-await controls stay within 5%. The initial WebSocket-low throughput and CPU regression did not repeat in the two longer runs, where both metrics stayed within 5%. Latency remains variable. @VSadov may I ask for another test run on your side so that we can know whether the previous regressions have been resolved or not? |
There were some lab capacity issues. it is taking longer than expected. |
We are wasting a lot of time on spinning after empty thread-pool dispatches, which causes major performance regressions on runtime async resumption. This issue also regressed our workload where we are using runtime async to build the job scheduler of a computing system, where each job can run very short.
Instead of always spinning after empty thread-pool dispatches, we can track recent external worker requests (including coalesced requests), and when a dispatch finds no work, park without spinning if the last external request was over
DispatchQuantumMsago. We can reuse the timestamp already recorded for dispatch progress to avoid reading the clock again after an empty dispatch.This reduces unnecessary polling for worker-generated workloads while keeping workers available for external bursts.
Resolves #135738