perf(ep-cpu): stop the decode wait yielding on every iteration

[justinchuby]

The defect

SharedState::worker_wait spins 4096 times on spin_loop, then calls thread::yield_now() on every subsequent iteration until the blocktime window expires. Its own doc says why:

then relax to yield_now so a busy host can schedule other work while we finish out the blocktime window

On the cpuset a decode budget confines the process to, there is nobody to hand the core to. bound_process_to_decode_budget (provider.rs:344, at EP initialize()) pins the process to exactly its budget's CPUs and caps the global Rayon pool to match. Mid-barrier, every other runnable thread is a peer in this same wait. sched_yield finds nothing eligible, returns immediately, releases nothing — and still pays a syscall and a full scheduler pass.

The politeness mechanism was occupying the cores it meant to hand back.

Measurement

int4_decode_loop_ab, width 16, production-confined, PROBE_TOKENS=1024 PROBE_REPS=2. Two binaries from the same tree differing only in worker_wait. 10 rounds of [base, fixed, base', fixed'] so host drift lands inside every contrast, with base vs base' as the A/A null. n=20 per arm.

	main	this PR	paired ratio (A/B)	A/A null	resolvable?
kernel time	2.61 cores [2.34, 3.25]	0.22 cores [0.19, 0.24]	0.088 / 0.085	1.028 [0.83, 1.11]	yes, disjoint
yields per 20ms window	14896	384	—	—	38.8x
tokens/s	279.6 [217.7, 287.3]	276.2 [252.8, 279.6]	0.991 / 0.978	1.004 [0.92, 1.16]	no
total occupied cores	15.76	15.88	—	—	no

Kernel time is disjoint and every one of the 20 paired ratios sits far outside the A/A null. Throughput overlaps, and its paired ratios poke outside the null in both directions, which is what noise looks like.

What this does and does not buy

Does not: free a core. Occupancy is unchanged (15.76 → 15.88) because the worker still holds it, now spinning in user space instead of in the kernel. A co-tenant gets the runqueue lock and the scheduler passes back, not the CPU.

Does: remove ~2.4 cores' worth of syscalls that returned without doing anything, and make sys_frac a usable signal again. At width 8 the old loop was bimodal — 0.4% kernel time and 216 tokens/s in one run, 15.7% and 130 tokens/s in the next — which made "are we thrashing" and "are we waiting" indistinguishable in my own matrices.

The larger question — whether the 500us window earns its occupancy at all — is #2071, deliberately not answered here. It needs the inter-token gap regime the window was designed for, not the tight loop this bench runs, and that harness (#1395) is not merged.

Preserving #1825

The deadline is now evaluated on a stride of 64 pure spin_loops (~640ns) rather than after every yield. That is not the stride #1825 removed: that one was a stride of yields, each costing microseconds to milliseconds of an already-starved thread. Checking every ~640ns is strictly more often in wall-clock terms than "on every yield" ever achieved, and the_blocktime_deadline_is_evaluated_on_every_yield_not_on_a_stride passes unchanged.

YIELD_CLOCK_STRIDE is kept a divisor of SPIN_LOOP_BUDGET so the yield phase gets its first clock read on the iteration it begins at.

The test, and why it is shaped this way

the_active_window_yields_on_an_interval_not_on_every_iteration counts yields over a 20ms window and asserts the phase cannot exceed one yield per 20us.

The bound is arithmetic, not empirical: a yield happens only when elapsed reaches next_yield, and next_yield advances by at least YIELD_INTERVAL each time, so the count is capped no matter how fast the loop spins. Load can only reduce it.

The ceiling is stated absolutely rather than derived from YIELD_INTERVAL. A ceiling computed from the constant under test moves with it — setting the interval to one nanosecond restores the exact defect and still satisfies a derived bound. That is the same shape as an A/B whose control is latched (#1736), and mutation M2 below is the proof it would have mattered.

slow_yield now counts unconditionally and only sleeps when injected. The test has to count yields in the uninjected regime it asserts about; injecting a sleep to make them countable would have set the interval under test.

Mutation-proved, three ways

Each kills only the new test:

mutation	observed	fails on
yield on every iteration, ignoring the interval	4884 yields	rate bound
YIELD_INTERVAL = 1ns	4837 yields	rate bound — the reason the ceiling is absolute
window shortened below the spin phase	0 yields	non-vacuity, not a silent pass on a bound nothing reached

And the main-equivalent loop (no stride, yield every iteration) measures 14896, which is the baseline quoted above.

A note on instruments

strace cannot confirm this. At ~55us per traced syscall the instrument's own cost exceeds the 50us interval it is measuring, so both arms yield on every check and the difference collapses to 17797 vs 13701 — a 1.3x that looks like a weak result and is actually the instrument setting the property. The in-process counter is the only tool here that does not. perf tracepoints are unavailable at perf_event_paranoid=1.

Validation

• cargo test -p onnx-runtime-ep-cpu --lib — 1752 passed, 0 failed, 25 ignored
• --features mlas — 1778 passed, 0 failed, 36 ignored
• cargo clippy --lib --all-targets -- -D warnings clean on both lanes; cargo fmt --check clean
• All benchmark runs took scripts/hostlock.sh for the whole window

[justinchuby]

Independent adversarial review

Reviewed by an independent Opus agent with no context from the work that produced the change, explicitly asked to find defects and to report what it cleared as well as what it found.

Verdict: SAFE TO MERGE AS-IS. No serious findings. Six minor; I have taken five (50191cb71).

Taken

1. SLOW_YIELD_US not reset before the new test's assertions, unlike the sibling directly above it. Benign today (it is already 0 on that path) but asymmetric, and a footgun if the test ever grows an injected arm. Reset added.
2. the_readiness_backstop_fires_within_its_deadline_not_a_stride_later leaks YIELD_COUNT. That test drives the other slow_yield site, and since this PR makes the helper count unconditionally, it now leaves a tally behind for whatever libtest schedules next on the thread. Harmless because both readers zero the counter on entry — but that invariant was unspoken and is exactly the class of thing that bites later. Reset added at the existing cleanup site. This is a real defect I introduced and did not spot.
3. "64 spin_loops cost ~640ns" is a single-microarchitecture number presented as general. pause is ~5ns on Skylake and ~47ns on Ice Lake and later, and spin_loop lowers to yield/isb on aarch64 — so the figure is 5x off in one direction on hardware in our own CI matrix. Reworded to the range that holds everywhere; the argument (the stride is cheap against a yield) survives at both extremes.
4. "~10 offers per window" is only true at the default blocktime, which is an env knob operators do exercise. Now says it scales linearly with ONNX_GENAI_CPU_DECODE_BLOCKTIME_US.
5. 14896/384 could be misread as CI-verified. They are out-of-band measurements taken by running the new test against each version of the loop; the test asserts a rate bound, not those numbers. Now said explicitly.

Not taken

6. Lower the non-vacuity floor from yields >= 2 to >= 1 against flake risk on a saturated runner. Declined: reaching 0 or 1 requires a stall as long as the entire 20ms window, and if that happens I would rather see it than have the bound quietly stop discriminating. Instead the reasoning is now recorded at the assertion — a red there is a re-run, not a regression in worker_wait — so a future maintainer does not misread it as a defect. 6 consecutive runs at RUST_TEST_THREADS=32 were clean, as were the reviewer's 10.

Cleared, with evidence

The review's most useful section. It suspected and then disproved: u32 wraparound corrupting spins.is_multiple_of(64) (2^32 % 64 == 0, residue class preserved); is_multiple_of/Duration::ZERO toolchain availability (1.87 and 1.53, against a 1.98.0 pin); blocktime == 0 regressing (4096 spins then immediate break — strictly better than the old code's one wasted yield); spin-forever paths (sense and shutdown are still checked every iteration); the #1825 sibling test going vacuous (it still fires: a reintroduced 64-wide stride would first re-consult the deadline after 64x10ms, with YIELD_COUNT >= 64); cross-thread YIELD_COUNT pollution (the test calls worker_wait on its own thread, and the release thread never calls slow_yield); other YIELD_COUNT readers (exactly two, both zero on entry); coarse-timer stalls; the Miri skip justification; and repo docs contradicting the new behaviour.

One follow-up it surfaced

task_runtime/pool.rs:429 and :520 have the same yield-every-iteration shape as the loop this PR fixes. Out of scope here — different pool, different lifecycle — but it is mine and I will measure it separately rather than assume the result transfers.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.21%. Comparing base (bc715c3) to head (1e2a56a).
⚠️ Report is 37 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2072      +/-   ##
==========================================
+ Coverage   80.32%   81.21%   +0.88%     
==========================================
  Files         426      429       +3     
  Lines      204769   214164    +9395     
  Branches   204769   214164    +9395     
==========================================
+ Hits       164483   173927    +9444     
+ Misses      34665    34474     -191     
- Partials     5621     5763     +142     

Flag	Coverage Δ
cli-ort-linux	72.51% <ø> (?)
cli-ort-windows	72.01% <ø> (-0.10%)	⬇️
mlas	85.93% <ø> (?)
offline	81.35% <100.00%> (+0.81%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
crates/onnx-runtime-ep-cpu/src/decode_spmd.rs	90.33% <100.00%> (+0.89%)	⬆️

... and 67 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justinchuby]

The two red Windows lanes here are not this PR's — they are the #2059 regression, and #2078 already fixes it

Run 32810586898 failed Rust (Windows ARM64) and Rust coverage (Windows x86_64). Both are the same defect, and it is not mine:

matmul_nbits.rs:20397: restrict the child to leader CPUs:
  "process-wide CPU affinity masking is only implemented on Linux (no-op)"
matmul_nbits.rs:20499: realized-width child failed (requested=default, status=exit code: 101)

restrict_self_to_leader_cpus() (added by #2059, 20646b609) calls decode_affinity::set_current_thread_affinity(...).expect(...). That function is #[cfg(not(target_os = "linux"))] -> Err(...) by design (decode_affinity.rs:531), so the .expect() panics deterministically off Linux.

This PR's diff is one file, decode_spmd.rs, and touches no affinity code, so the failing path is unreachable from my change. @pris diagnosed and fixed it in #2078 before I got here; I have added my run as third-lane corroboration there rather than duplicating the fix.

Ruling out the other candidate explicitly, because I owe an honest answer on this lane: this is not #1745. #1745 is 0xC0000005 with empty stderr and no Rust panic. This prints a panic at a fixed line on two different architectures, one with llvm-cov and one without. Deterministic, not intermittent.

Status of this PR

The other 17 checks are SUCCESS. I am not merging on that basis — Rust (Windows ARM64) is required, and "it's someone else's bug" is not a merge gate. Plan: wait for #2078 to land, rebase onto it, re-run, merge only when the lane is actually green. No --admin, no --auto.

[justinchuby]

Rebased onto 08022b66d, which includes @pris's #2078 (6db21953c). Force-pushed 29fc40af8.

Local re-validation after the rebase: cargo test -p onnx-runtime-ep-cpu --lib 1796 passed, 0 failed, 26 ignored; clippy -D warnings clean on --all-targets; cargo fmt --check clean. The mlas lane is re-running now.

Waiting on required CI. Will merge only when the Windows lanes are actually green — not on the strength of the other 17.

[justinchuby]

Rebased onto c8509042a (main with #2093 merged), so the Rust coverage (macOS arm64) lane on this PR should now go green — that failure was #2089, not this change.

Only decode_spmd.rs is touched by this PR; the rebase pulled in others' commits and nothing of mine changed. Still expecting CUDA compile (Linux x86_64) to be red via #2094, which is also not mine and not a required context.

I will merge once Fast (Linux x86_64) and Rust quality are green on the rebased head.