Repository navigation
docs(cpu): withdraw the "decode width 2 is inert" claim -- it is 1.96x - #1740
Merged
Merged
Conversation
Two merged documents recorded that `ONNX_GENAI_CPU_DECODE_THREADS=2` produced timings identical to `=1` and concluded the second worker was parked rather than computing -- "a dispatch/wakeup defect in the persistent decode pool at small worker counts, not a measurement artifact". It is a measurement artifact. Controlled re-measurement on a quiet host (one process per launch, interleaved arms, per-rep load guard on the instantaneous runnable count, over-guard cells discarded) gives 20.447 ms/token at `=2` against 40.039 at `=1` -- a 1.96x speedup against a 0.6% A/A null. Scaling is near-linear to t=8 (7.52x) and knees into t=16 (9.26x) at the memory-bandwidth plateau the pool default is sized for. The parked-worker reading is falsified directly rather than only by timing: per-thread attribution over a steady window shows both `=2` workers 99% busy with 62 voluntary context switches in six seconds. A parked worker shows the inverse -- near-zero CPU and high voluntary ctxsw. The original figure came from `/usr/bin/time`'s `Percent of CPU`, which is `(user+sys)/wall` and therefore wall-derived: under contention it degrades exactly like the wall time it was being used to corroborate, so it was not the independent confirmation it appeared to be. The clearest signal that this was harness-side was arithmetic -- 23.529 vs 23.527 ms/token agree to four significant figures, and contention is random. The protocol point survives and is strengthened: verify a knob structurally (`w` in must give `w` SPMD threads out -- categorical, so valid even on a loaded host) rather than by comparing timings. Records the two configurations where the knob genuinely is vacuous: in-process width sweeps (the pool is a process-wide `OnceLock`, built once at first decode) and `THREADS=N` on an exactly-N-CPU cpuset, where `reserve_single_group_headroom(N, N) == N-1` leaves one worker and the `total_workers <= 1` serial short-circuit then runs everything on the dispatcher with no diagnostic. Also documents that t=1 is not "the pool with one worker" but "serial on the dispatcher", so it is a different code path from every other width and comparisons against it should say so. Docs only; no shipped code changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review caught that the t=4 cell quoted its minimum (10.442) while its two reps were 10.442 and 15.283 -- a 46.4% spread, bimodal in the way this host is known to be per process launch. The other rows were shown with reproductions and an A/A null; t=4 was not, so a reader would reasonably read the whole curve as equally solid. Adds per-cell rep counts and spreads, marks t=4 provisional, and withdraws the 'near-linear to t=8' phrasing, which leaned on the unmeasured point. The headline is unaffected: t=2 at 1.96x is outside the 0.6% A/A null and is backed independently by the 99%-busy per-thread attribution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
marked this pull request as ready for review
August 22, 2026 10:34
This was referenced Aug 22, 2026
Closed
justinchuby
added a commit
that referenced
this pull request
Aug 22, 2026
) Closes #1746. `ONNX_GENAI_CPU_DECODE_THREADS=N` builds **N-1 compute lanes** in production, for every N. At `N=2` that is one lane, which trips the `total_workers <= 1` serial short-circuit in `dispatch_output_rows` — so the whole persistent pool degenerates to serial dispatch on the engine thread and the knob is indistinguishable from `=1`. ## Root cause: two correct pieces composing badly Neither half is wrong on its own, which is why unit tests on either half could not see this. 1. `provider.rs:340` — `EpFactory::initialize()`, the earliest per-session hook, calls `bound_process_to_decode_budget()`. With an explicit budget it confines the process to **exactly N CPUs**, so that "a user who caps cores disturbs at most N CPUs". Deliberate and documented. 2. Later, on first decode, `node_shards(N)` reads `allowed_cpus()` — now exactly N — and `reserve_single_group_headroom(N, N)` returns **N-1**, keeping one CPU free for the inline dispatcher. Also deliberate and measured: N spinning workers pinned across all N allowed CPUs starve the dispatcher and collapse throughput **20-60x** (1.47 tok/s at 32 workers on `taskset -c 0-31` vs ~29 tok/s once one CPU is spare). `reserve_single_group_headroom`'s docstring notes it only fires for "a user who sets `ONNX_GENAI_CPU_DECODE_THREADS=N` on an exactly-N-CPU cpuset". **We build that cpuset ourselves in step 1**, so it is not a corner case — it is the guaranteed outcome of every explicit budget. Measured through the production sequence, under an outer `taskset -c 0,2,...,30`: ``` onnx-genai: CPU decode budget 2 confined the process to 2 CPUs [0, 2] PROD budget=2 allowed_before=Some(16) allowed_after=Some(2) spmd_threads=1 PROD budget=4 allowed_before=Some(16) allowed_after=Some(4) spmd_threads=3 PROD budget=8 allowed_before=Some(16) allowed_after=Some(8) spmd_threads=7 ``` `allowed_before=16 -> allowed_after=N` is the whole mechanism. The dispatcher does not compute — it publishes and spins in `SharedState::wait`. So the reserved CPU is not merely unallocated, it is **burned spinning**. ## Fix Keep the reservation exactly as measured, and let the dispatcher compute the shard it was holding a CPU for. At budget N: **N-1 pinned worker threads** (unchanged — the starvation cliff stays fixed) plus the dispatcher computing on the reserved CPU = **N compute lanes on N CPUs**, no thread oversubscribed. Strictly better than N-1 lanes plus a spinning CPU. | budget | pinned threads | compute lanes (before) | compute lanes (after) | |---:|---:|---:|---:| | 2 | 1 | **1** (serial short-circuit) | **2** | | 4 | 3 | 3 | **4** | | 8 | 7 | 7 | **8** | | 16 | 15 | 15 | **16** | Mechanically: `publish` counts down `node_thread_counts` (spawned threads only — the dispatcher's shard has no thread to wait for), the dispatcher runs the remaining shard inline, then `wait()`s. Partitioning already produces `total_workers` shards, so only the pending-count source and the shard-to-thread mapping change. `dispatch_inline` (the re-entrant fallback) already loops over all shards and stays correct. **Scope.** Single-group layouts only, and only when the reservation actually fired. A group that already had headroom is untouched, or the pool would be one lane *wider* than the budget allows. On a NUMA split the dispatcher's node is not known at build time, so handing it a shard could pull that shard's weights across sockets; those layouts keep the previous behaviour. ## `catch_unwind` is load-bearing, not defensive The published `Job` holds a raw pointer to a closure borrowed off the **dispatcher's own stack frame**, and the workers read through it until the barrier drains. Now that the dispatcher also computes, a panic in its shard would unwind that frame while workers are still reading it — a use-after-free, not merely a hang. So: catch, complete the barrier, then `resume_unwind`. Miri agrees. Removing the `catch_unwind`: ``` error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `onnx-genai-spmd` and (2) retag write of type `{closure@decode_spmd.rs}` on thread `decode_spmd::te` at alloc1106927 ``` `decode_spmd`'s panic-safety test is added to the Miri lane, with the "verified both ways" note the workflow already uses. ## Falsifiers Every test was checked to fail without the fix. | # | Mutation | Result | |---|---|---| | F1 | Dispatcher never takes a shard | 3 unit tests fail (`...restores_the_requested_width`, `...fans_out_across_the_dispatcher_and_one_worker`, `...covers_its_rows_exactly_once`) | | F2 | Remove `catch_unwind` | panic test fails natively; **Miri reports UB** (above) | | F3 | `publish` the shard counts instead of thread counts | barrier never drains — hangs (exit 124) | | F4 | Dispatcher never takes a shard | end-to-end subprocess test fails: `ONNX_GENAI_CPU_DECODE_THREADS=2 must buy 2 compute lanes, got 1 (1 pinned threads on 2 allowed CPUs)` | The end-to-end test spawns **one subprocess per budget** — not stylistic. Both halves latch (`PROCESS_BUDGET_BOUND` and the pool are `OnceLock`s) and the child mutates process-wide CPU affinity, which would poison the test runner. It skips budgets above `available_parallelism()` and skips NUMA-split layouts, so it is meaningful on a 2-vCPU runner and inert where it cannot apply. ## No performance claim This PR claims a **width restoration**, proven categorically by thread and lane counts, not a speedup. The host is shared and has been above loadavg 60; per the standing protocol I am not quoting a throughput number I could not measure under control. The arithmetic ceiling is 2x at `N=2` and 1.33x at `N=4`, but realised speedup depends on scaling efficiency and is not asserted here. ## Corrects the record on #1740 The merged width-scaling benchmark lists this exact mechanism as vacuity case 2, but classifies it as *"benchmarking inside a small container hits this"*. **#1740's measurements are correct and nothing in them is retracted.** `int4_decode_loop_ab` never calls `EpFactory::initialize()` — it goes straight to `with_decode_pool_scope` — so the confinement never runs there, the process keeps its 16 `taskset` CPUs, `reserve_single_group_headroom(2, 16) = 2`, and the bench genuinely gets two busy workers. Its 1.96x and its per-thread attribution stand. That is also exactly why its non-vacuity check passed (`w` in gave `w` out) while production lost a worker. The finding is the **divergence**: the decode bench does not reproduce the production thread topology, so a width sweep run through it is structurally unable to observe this class of defect. One correction to that table: its **`t=16` row is a 15-worker measurement**, since `reserve_single_group_headroom(16, 16) = 15` fires even without the production confinement. The `1/2/4/8` checks could not catch it because the reservation only triggers at exactly full subscription. This understates the plateau rather than overstating it, so the conclusion drawn from it is unaffected. Addendum written into `docs/benchmarks/2026-08-22-decode-width-scaling.md` rather than left in a PR comment. I also withdrew my own first reading of this: I initially attributed Roy's `=1`/`=2` bench timings to this defect. That attribution was wrong — he was on the bench binary — and is retracted in #1746 and on #1722. ## Validation - `cargo test -p onnx-runtime-ep-cpu --lib` — **1616 passed, 0 failed** - `cargo test --workspace` (excluding `onnx-genai-bench`) — **6303 passed, 0 failed** - `cargo fmt --check`; `cargo clippy --all-targets -D warnings` — clean - Feature configs: `--features mlas` (incl. the `Steal` path, which keeps its previous width), `--no-default-features` — clean - Cross: `aarch64-unknown-linux-gnu` clippy `-D warnings` — clean - All **seven** Rust-quality lint scripts — pass - Miri: `decode_spmd::tests::a_panic_in_the_dispatcher` — ok in ~23s; reports UB with F2 applied - Latest `origin/main` merged in before this run --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 23, 2026
…ation, and two sites this PR itself missed Adversarial review of the first commit found six defects. All six addressed. The ironic one first: this PR criticised #1740 for missing a site, and then missed two of its own. Both asserted the very error bar being withdrawn. * `int4_decode_loop_ab.rs:55` -- the bench header still advertised "16 -> 3.32 ms/token (1.77x, +-0.7% over three interleaved repetitions)". * `2026-08-21-int4-acc4-ntile-design.md` §4 and §"noise floor" -- both still said t=8 and t=16 "stayed within +-0.7% throughout; only the all-vCPU configuration is unstable", the direct opposite of this PR's Result 3. "All-vCPU" was the wrong boundary: the resource that runs out is physical cores, and w=16 already consumes all 16 of them. The mechanism for width 1 was wrong, in the source material and in my own earlier accounts. It is *not* the `total_workers <= 1` short-circuit in `dispatch_output_rows`, and there is no spawned worker idling at 0% busy: at width 1 the budget confines the process to a single CPU and `build_from_env` declines to construct the pool at all, because one CPU leaves no core for the inline dispatcher beside a spinning worker. The crate's own test says so -- "the smallest budget that builds a pool is 2". `path=flat` comes from that build-time fallback. The "vs serial" conclusion is unchanged. This also reframes Result 1 honestly: the budget confines the process to `w` CPUs, so t=2 has twice the hardware of t=1 and ~2x is the *expected* result, not a discovery. The finding is negative -- the recorded curve reported no speedup where the ordinary one was. Dropped the claim that acc0 and acc4 1.96x are "independent"; they share a harness and a serial baseline. The SMT attribution was over-read. Pearson r = 0.91 rests entirely on two leverage points; Spearman is 0.54 on n = 6, not significant. Launch 8 is slow at 15.7% sibling occupancy while launch 2 is the fastest of all at 16.1%, and the two modes' sibling ranges overlap, so no threshold separates them. Odd-CPU occupancy is also a proxy for whole-socket load, and shared L3 / memory bandwidth contention is not excluded. Now stated as: bimodality confirmed, external-load involvement likely, SMT unproven. The withdrawal is also re-scoped. The build moved ~1.8x between the two measurements (t=8: 5.87 -> 3.31), so the old +-0.7% cannot be re-checked and the claim that those three reps "landed in the fast mode" is not supportable -- the old t=16 sits near today's w=8. Withdrawn as unverifiable, not disproven, and the t=32 bimodality parallel is marked suggestive rather than established (three points cannot carry it, and the levels do not line up). Finally, the "13-14 CPU-s/wall-s in both modes" figure is no longer asserted without data: the rusage numbers are now tabulated, ten launches split by mode, showing identical CPU-seconds across a 1.8x wall-time gap. That is the evidence for a CPU-time guard being blind to this class of interference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 23, 2026
…stion" was wrong, not open (#1837) ## What #1740 retracted the `ONNX_GENAI_CPU_DECODE_THREADS=2` "silently does nothing" claim in three locations. It missed a fourth — **§20 of `CPU_MATMUL_ASSIGNMENT.md`** — and that was the worst of the four, because the other three stated `t=1 ≡ t=2` as an *observation* while §20 promoted it to: > (t=1≡t=2 is **unexplained and recorded as an open question**, not an explanation.) An open research question is the version most likely to stop the next person re-measuring, and it would currently cause them to reject a true 1.96x as a measurement error. I corrected it **by measuring, not by cross-referencing** — I didn't want to assert my table had the same cause as the acc0 one without showing it. ## Result 1 — t=1 ≡ t=2 is false acc4 / llama / block 32, one process per cell, 384-token steady window, pinned to 16 physical cores: | width | path | ms/token | tok/s | vs t=1 | reps | spread | |---:|---|---:|---:|---:|---:|---:| | 1 | `flat` | 14.300 | 69.9 | 1.00x | 5 | 2.25% | | 2 | `spmd-pool` | **7.278** | 137.4 | **1.96x** | 5 | 1.73% | | 4 | `spmd-pool` | 3.784 | 264.3 | 3.78x | 5 | 2.60% | | 2b *(A/A null)* | `spmd-pool` | 7.278 | 137.4 | — | 5 | 1.29% | **A/A null: 0.00%.** **The 1.96x is not itself remarkable and isn't sold as if it were.** The budget confines the process to `w` CPUs (`CPU decode budget 2 confined the process to 2 CPUs [0, 2]`), so t=2 has twice the hardware and ~2x is the *expected* result. That is the point: **the recorded curve reported no speedup where the ordinary one was.** The finding is negative. ## Result 2 — t=1 doesn't build a pool at all (and I had the mechanism wrong) At width 1 the budget confines the process to a **single CPU**, and `build_from_env` then *declines* to construct the pool — one CPU leaves no core for the inline dispatcher beside a spinning worker, so it would starve itself. The crate's own test records it: *"the smallest budget that builds a pool is 2."* So it is **not** the `total_workers <= 1` short-circuit in `dispatch_output_rows`, and there is **no spawned worker idling at 0% busy** — nothing is spawned. My first draft said both, following the existing accounts; review caught it and I verified the source. Conclusion unchanged and it's the part that matters: **t=1 is a different code path, so ratios against it mean "vs the serial flat path".** ## Result 3 — the ±0.7% error bar is withdrawn as *unverifiable* Six independent launches, alternating widths, sampling `/proc/stat` per-CPU: | width | n | min | median | max | spread | |---:|---:|---:|---:|---:|---:| | 8 | 6 | 3.195 | 3.307 | 3.509 | **9.8%** | | 16 | 6 | 1.476 | 3.210 | 9.064 | **514%** | `w=8` is stable across launches; `w=16` spans a factor of six. That alone retires the `±0.7%`. **I am deliberately *not* claiming the cause.** The obvious reading is SMT contention (`w=16` holds all 16 physical cores) and Pearson is 0.91 — but that rests entirely on two leverage points, and **Spearman is 0.54 on n=6**, not significant. There's a direct counterexample: launch 8 is **slow (4.820 ms) at 15.7%** sibling occupancy while launch 2 is the **fastest (1.476 ms) at 16.1%**. The modes' sibling ranges overlap, so no threshold separates them. Odd-CPU busy% is also a proxy for whole-socket load — shared L3 / memory-bandwidth contention isn't excluded. **Bimodality confirmed; external-load involvement likely; SMT unproven.** **Nor am I claiming the old reps were secretly bimodal.** The build moved ~1.8x between the measurements (t=8: 5.87 → 3.31), and the old t=16 (~3.3) actually sits near *today's w=8*. The old interval can't be re-checked. It is withdrawn as **unverifiable, not disproven** — and `8→16` is left unquoted rather than given an interval the host can't carry. ## The finding with the longest reach: a CPU-time guard is blind to this Rusage `(utime+stime)/wall` across ten `w=16` launches, split by mode: | mode | ms/token | CPU-s per wall-s | |---|---:|---:| | fast | 1.884, 1.888, 1.901, 1.897, 1.897, 1.893 | 13.21–14.74 (mean 14.4) | | slow | 3.392, 3.374, 3.408, 3.375 | 13.82–14.45 (mean 14.1) | **Identical CPU-seconds across a 1.8x wall-time gap.** The affected thread is never descheduled — it stays runnable and burns its full CPU-second, just retiring fewer instructions per cycle. So rusage efficiency, voluntary-ctxsw counts, and `Percent of CPU` are all blind to it. A "99% busy, not parked" certification does **not** mean a run is trustworthy. `realized=`/`path=` wouldn't have caught it either — necessary, not sufficient. ## Also ruled out - **Not a config failure** — every launch reported `realized=16 path=spmd-pool as_requested`. - **Not a bad pin** — verified `thread_siblings_list` is `0-1`, `2-3`, … so the even-CPU pin really is 16 distinct physical cores. On a host where siblings are `(0,16),(1,17),…` the same pin would silently be 8 cores and every number here would be wrong. ## Changes - `docs/performance/CPU_MATMUL_ASSIGNMENT.md` §20 — stale curve replaced; `8→16` left unquoted. - `crates/onnx-runtime-ep-cpu/benches/int4_decode_loop_ab.rs` — bench header still advertised the withdrawn `1.77x, ±0.7%`. - `docs/benchmarks/2026-08-21-int4-acc4-ntile-design.md` — dated corrections that **preserve** the original observations; §4 and the noise-floor item said t≤16 was stable within ±0.7%, the direct opposite of Result 3. - `docs/benchmarks/2026-08-21-int4-acc4-execution-regime.md` — footnote on a table row that uses `1.77x` as a roofline comparator (its conclusion is unaffected). - `docs/benchmarks/2026-08-23-acc4-decode-width-remeasurement.md` — new full record. ## Review Adversarial review found **six** defects in the first commit, including — with some irony for a PR about a correction that missed a spot — **two sites this PR itself missed**, both asserting the very error bar being withdrawn. All six fixed in `26b4aa0e0`; details in that commit message. ## Not in scope `CPU_MATMUL_ASSIGNMENT.md:1893` (acc0 `56.307 vs 30.632 = 1.84x`) also carries no width label. Not re-measured, so not labelling it on inference. ## Validation `cargo fmt --all --check` clean; `cargo clippy -p onnx-runtime-ep-cpu --benches --all-features` clean. The Rust change is a doc comment, but it does take the PR off the docs-only CI path, so full required checks apply. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
What
Withdraws the claim that
ONNX_GENAI_CPU_DECODE_THREADS=2is an inert knob and that its identity with=1was "a dispatch/wakeup defect in the persistent decode pool at small worker counts, not a measurement artifact".It is a measurement artifact.
=2is 1.96x faster than=1.Evidence
qwen / acc0 / block 32 / sessions 1, pinned
taskset -c 0,2,...,30, mainc4987923c, pure native CPU EP. One process per launch, arms interleaved round-robin, per-rep load guard on the instantaneous runnable count, over-guard cells discarded (7 discarded, not silently kept).=1A/A null (both arms width 2): 0.6%.
=2reproduced at 20.447 / 20.575 / 20.562 / 20.636 across three independent windows including a contended one.The t=4 cell is not measured and is marked provisional — its two reps were 10.442 and 15.283 (46.4% spread), so 3.83x is a waypoint awaiting reps, not a result. Nothing below leans on it. The measured shape is a solid 1.96x at t=2, solid t=8 and t=16 points, and a knee into t=16 (1.23x for the last doubling) consistent with the memory-bandwidth plateau the pool default is sized for. Whether the t=1→t=8 segment is linear depends on the unmeasured t=4 point and is not claimed.
The parked-worker reading is falsified directly, not just by timing
Per-thread attribution from
/proc/<pid>/task/*/, deltas over a steady window only:At
=2both workers are 99% busy with 62 voluntary context switches in six seconds. A parked worker shows the inverse — near-zero CPU and high voluntary ctxsw.Why the original reading was convincing but not sound
Percent of CPUfrom/usr/bin/timeis(user+sys)/wall— wall-derived. Under contention it degrades exactly like the wall time it was being used to corroborate, so it never was an independent confirmation. Theusercolumn is contention-robust and was flat across widths, which is consistent with correct work division.The decisive clue was arithmetic: the original pair, 23.529 vs 23.527 ms/token, agree to four significant figures. Contention is random and does not do that. Two configurations reading the same number to 0.008% are the same configuration.
What survives
The protocol point, strengthened — verify a knob structurally, not by timing:
win must givewout (verified 1→1, 2→2, 4→4, 8→8). Categorical, so valid on a loaded host, and it would have caught this in thirty seconds.Two configurations where the knob is genuinely vacuous are now recorded:
static POOLS: OnceLock<Option<SpmdDecodePools>>builds the pool once per process at first decode, so a harness looping widths in-process reports the first width forever. (acc0_gap_matrix.pyis clean: it shells out per cell.)THREADS=Non an exactly-N-CPU cpuset —reserve_single_group_headroom(N, N) == N-1leaves one worker atN=2, which then hits thetotal_workers <= 1serial short-circuit: everything runs on the dispatcher while a pinned worker sits parked, with no diagnostic.Also records that t=1 is not "the pool with one worker" but "serial on the dispatcher" (
total_workers <= 1short-circuit), so it is a different code path from every other width.Why it matters
Left standing,
CPU_MATMUL_ASSIGNMENT.md:1810("any t=2 row is a duplicate t=1 row") instructs the next reader never to measure t=2, and would cause a true 1.96x to be rejected as a measurement error.Notes
sysrise at t>=4 from the same handoff is not withdrawn: it is real, it is theworker_waityield ramp, and it stays open with me.