Repository navigation
perf(ep-cpu): bound the worker spin phase's sched_yield rate - #2151
Merged
Merged
Conversation
The spin loop's yield phase yielded on every iteration, so how often a worker entered the kernel was set by how fast the loop went round rather than by any policy. This gives it one: at most one `sched_yield` per 10us of window, with a `spin_loop` in between. What #2075 measured, and what it did not ---------------------------------------- Regressing steady-window kernel seconds on the `spin_yields` counter (#2117) across nine arms spanning a 94x range of yield counts: sys ~ yields + parks R^2 0.9971 1.044 us/yield 76.5 us/park intercept +17.8 ms The coefficients recover both syscall costs -- an uncontended `sched_yield` and a futex round trip -- from a fit handed only counter totals and a kernel-time total, and the intercept is ~0. Replicated at 1.061 us/yield, 1.6% apart. So the yield phase is essentially all of the kernel time, and the control (`sys ~ user_s`) fits both worse and impossibly, at a -0.43 s intercept. **It does not follow that removing the yields saves CPU, and it does not.** A yield costs ~1.05us of kernel time and advances the wall clock ~1.05us; a `spin_loop` advances it ~0.3ns at ~0.3ns of user time. Per unit of waiting both hold the core at ~100%. Measured, order-cancelled, against a same-binary null: sys share falls from 20-31% to 3.6-5.9% while cpu-per-wall-second moves -0.02% to -0.85%, every one of which is smaller than the null arms' own -0.71% and -0.43%. This relabels kernel time as user time. Latency does not resolve in either direction in any arm. So the case for it is the bounded rate itself and the 5-12x drop in syscall traffic the process imposes on the host's run queues -- not a CPU saving, and the constant's doc says so in those words. Safety, measured rather than argued ----------------------------------- A spin does not offer the core to a neighbour and a yield does; every solo matrix is blind to that difference. With a CPU-bound co-tenant beside the pool, the co-tenant completes 1.000, 1.000 and 1.002 of its baseline work at 8, 16 and 32 competing threads, against a same-binary null of 1.000 +/- 0.001. Two properties keep the escape valve intact, both tested: the phase's first iteration always yields, so a core is never held longer before being offered; and the limit self-disables under contention, because a yield that blocks longer than the interval leaves the next one due immediately. #1825's guarantee is untouched. The deadline is still evaluated on every pass through the phase -- the clock read it already required is now the same one that gates the yield, so this adds no `Instant::now()` -- and `the_spin_window_deadline_is_evaluated_on_every_yield_not_on_a_stride` still passes, which is also the test that demonstrates the self-disable. Tests ----- `the_yield_phase_rate_limits_its_yields_against_the_wall_clock` states its ceiling absolutely (64) rather than as `MAX_SPIN / YIELD_MIN_INTERVAL`: a derived ceiling rises to meet a one-nanosecond interval and asserts only that division works. It refuses to pass vacuously if the window expired before the phase was entered, and it asserts the structural lower bound -- the first iteration yields -- rather than a statistical one, which load could push below. `the_spin_phase_yield_counter_counts_exactly_the_yields_the_window_performed` took its expected value from `spins - SPIN_LOOP_BUDGET + 1`, which this change necessarily breaks. It now compares against the count of actual `spin_yield` calls: a counter test that recomputes the loop's policy asserts the policy rather than the counter. `YIELD_COUNT` is bumped unconditionally for that, having previously only existed when a test injected a yield cost. Mutation-proved both directions: removing the limit fails the ceiling at 221 yields, and delaying the phase's first yield fails all three of the new lower bound, the counter identity and #1825's deadline test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
marked this pull request as ready for review
August 25, 2026 20:25
This was referenced Aug 25, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2151 +/- ##
===========================================
+ Coverage 72.01% 80.37% +8.36%
===========================================
Files 12 430 +418
Lines 5227 213120 +207893
Branches 5227 213120 +207893
===========================================
+ Hits 3764 171303 +167539
- Misses 1330 36022 +34692
- Partials 133 5795 +5662
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This was referenced Aug 25, 2026
This file contains hidden or 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
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.
Closes item 2 of #2075.
The headline is a negative result, and it is in the constant's doc comment
This does not reduce CPU. It moves it from the
syscolumn to theusercolumn. Asched_yieldcosts ~1.05 µs of kernel time and advances the wall clock ~1.05 µs; aspin_loopadvances it ~0.3 ns at ~0.3 ns of user time. Per unit of waiting, both hold the core at ~100%. Replacing one with the other therefore relabels the accounting and does not give the core back.Measured on a 7×7 order-cancelled, per-repetition-paired matrix (
bench_decode_gap, gaps 20 µs–3 ms × t=2/4/8/16):Every cpu-per-wall delta is smaller than the two same-binary null arms' own drift (−0.71% and −0.43%). Only t=16 sits outside at −6.6% ± 0.4 (~2.4σ) — suggestive, not established, and not claimed.
Latency is unresolvable at this sample size: p50 medians looked 7–15% better under co-tenancy, but paired per-repetition analysis gives ratio means 1.005 / 1.016 / 0.960 with sd up to 0.515. No latency claim is made in either direction.
Co-tenant harm is ruled out precisely. A dependency-free CPU-bound competitor at 8/16/32 threads shows throughput 1.000, 1.000, 1.002 against a null of 1.000 ± 0.001.
So what is it worth?
The yield rate today is set by how fast the loop happens to run, not by policy — nothing bounds it. This makes it a bounded, tested quantity: ≤64 yields per
MAX_SPINwindow (YIELD_MIN_INTERVAL= 10 µs =MIN_SPIN/2), which is 5–12× less run-queue traffic imposed on the host, with proven-zero harm to co-tenants.Whether that reduction helps other tenants via run-queue lock contention is plausible and unmeasured, so it is deliberately not claimed.
The attribution behind it (this part is solid)
Before changing anything I checked #2075's premise with a regression rather than an anecdote. Nine arms spanning a 94× range of yield counts:
The fit was given only counter totals, and it recovers both syscall costs — ~1.05 µs for an uncontended
sched_yieldand ~68 µs for a futex round-trip — from data that contained neither. Replication agrees to 1.6%.The control fails:
sys ~ user_sfits worse (R²=0.86) and impossibly, with a −0.43 s intercept. Sys share spans 2.4%→31.6% across arms, so kernel time is not a fixed fraction of CPU. Yields explain 77–101% of kernel time in every arm above 50CLK_TCKticks (arms below that are 10 ms-quantised and are not quoted).So #2075's attribution is correct. The payoff simply is not there.
Implementation
Phase 2 of
spin_for_dispatchbecomes:start.elapsed()read is the one fix(ep-cpu): bound the decode pool's readiness barrier and release its workers on failure #1825 already mandates per phase-2 iteration; it now gates both the deadline and the yield. No added clock read.next_yieldis computed from the pre-yieldelapsed, which makes the limit self-disable under contention: an 11 ms yield returns with the next one already due, so a genuinely contended box behaves exactly as before.Caught/Shutdownlatency strictly improves: the epoch is re-read every ~25 ns instead of being invisible for up to 11 ms inside a blocking yield.Independent Opus review — one real find, taken
[RISK] I had claimed "the phase's first iteration always yields", so the tests asserted
performed > 0. But the deadline check now precedes the yield, and phase 1's last stride check is atspins == 4032— iterations 4033–4095 run with no clock read. On a loaded box the window can therefore already be expired at phase-2 entry, giving zero yields withspins >= SPIN_LOOP_BUDGET. The assertion could hard-fail on correct code.The reviewer offered relaxing the assertion, or yielding before honouring an expired deadline. I rejected the latter as actively wrong (it spends a syscall to leave late) and rejected merely relaxing as losing the test's teeth. Instead the assertion is now exact, using a discriminator that needs no clock:
Proved by construction, not by argument: with
CLOCK_CHECK_STRIDEmutated to1<<20and the window to 1 ns, the run reaches phase 2 with the deadline already blown and a probe confirmsspins=4096, performed=0— the reviewer's exact scenario, which the old assertion would have failed and the new three-wayiffhandles.Seven further items were traced and cleared:
spinsu32 wrap (~107 s of looping vs a 500 µs window cap, and a wrap self-corrects within ≤64 iterations),next_yieldoverflow, #1825's guarantee and its test's continued discriminating power, self-disable under contention, theYIELD_COUNTchange, and release-build cost on thespin_window <= 130 µspath.Tests
the_yield_phase_rate_limits_its_yields_against_the_wall_clock— absolute ceiling of 64 yields per window, with a non-vacuity gate.the_spin_phase_yield_counter_counts_exactly_the_yields_the_window_performed— the three-way exactiffabove.YIELD_COUNT's increment moved out ofspin_yield's injection branch so it is unconditional ground truth (still#[cfg(test)], zero release cost).Mutation-proved: removing the limit → 221 yields against the 64 ceiling → fails. Delaying the first yield → fails all three of the new lower bound, the counter identity, and the pre-existing #1825 deadline test.
Where the loss actually is
The pool burns ~15 cores at a 300 µs gap and the yield has nothing to do with it. That is spin-window policy —
MAX_SPIN/MIN_SPINadaptation — which I am filing separately with the cpu-per-wall framing this matrix established.Validation
cargo test -p onnx-runtime-ep-cpu --lib1828 passed / 0 failed;--features mlas1854 / 0;clippy --all-targets --all-features -D warningsclean;fmt --checkclean.Scope corrections to #2075 (restated)
There is one yield site, not two —
:429/:520was my misreading. AndDISPATCHER_YIELD_STRIDEdid not regress anything; it landed in #1201, the same commit that introduced the pool.