From 7cdcafe9dfe040f1a77c26c04eee767ddddd39e9 Mon Sep 17 00:00:00 2001 From: Justin Chu Date: Sat, 22 Aug 2026 19:17:22 +0000 Subject: [PATCH 1/2] bench(cpu): print the realized decode width next to every t=N row `int4_decode_loop_ab` sweeps `ONNX_GENAI_CPU_DECODE_THREADS` and prints a row per width, but nothing in the row reported the width the run actually built. At least four paths silently reduce it below the request -- the pre-clamp in `resolve_persistent_decode_threads_with_override`, both headroom reservations, and the single-CPU-cpuset branch that drops decode to the flat path entirely -- and only one is even `NXRT_CALIB_DEBUG`-visible. A `t=N` row was a label, not a measurement of width N. That is not hypothetical. Before #1766 the benches never called `initialize()`, so the process was unbounded and the global Rayon pool ran full width at every budget. Measured at 9747b4971 (current main minus #1766), qwen/acc0/block32/s=1, same binary otherwise: t=1 40.298 ms/token 100% CPU t=2 37.166 ms/token 267% CPU <- 1.08x for 2.67 cores That flat bottom end is what "ONNX_GENAI_CPU_DECODE_THREADS=2 does nothing" was: not a dispatch pathology, an unbounded-topology artifact. On current main the same sweep scales 1.98x / 3.91x / 7.55x / 15.0x at t=2/4/8/16, reproducible within 2% sweeping in both directions. Reports rather than asserts. A reduced width is legitimate when the host genuinely cannot honour the request (an 8-lane budget in a 2-CPU cpuset), and aborting there would leave a constrained container unable to benchmark at all. The `WIDTH-MISMATCH` token is for the caller -- human or matrix script -- to discard or mark the row, as a contended cell is already marked UNTRUSTED. Called after the phases, never before: the pool is built lazily at first decode and `decode_width()` is deliberately non-forcing, so reading it earlier reports `path=unresolved` -- and if it were forcing it would build the pool and change the topology being measured. Scoped to `int4_decode_loop_ab`. `gqa_decode` also reads a decode width but pins the ambient Rayon pool by design, so a width line there would report `path=flat` and mislead rather than verify. Refs #1763, #1766 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../onnx-runtime-ep-cpu/benches/common/mod.rs | 39 +++++++++++++++++++ .../benches/int4_decode_loop_ab.rs | 4 ++ 2 files changed, 43 insertions(+) diff --git a/crates/onnx-runtime-ep-cpu/benches/common/mod.rs b/crates/onnx-runtime-ep-cpu/benches/common/mod.rs index 146b0120c1..56150ba448 100644 --- a/crates/onnx-runtime-ep-cpu/benches/common/mod.rs +++ b/crates/onnx-runtime-ep-cpu/benches/common/mod.rs @@ -230,3 +230,42 @@ pub fn init_decode_topology() { .initialize(&Default::default()) .expect("the CPU EP must initialize"); } + +/// Print the decode width the run actually realized, next to the width it asked +/// for. +/// +/// Call this **after** at least one decode has run. The persistent pool is built +/// lazily at first decode and [`decode_width`] is deliberately non-forcing, so +/// calling it earlier reports `path=unresolved` and no realized width -- and +/// would build the pool if it were forcing, changing the very topology the +/// benchmark is measuring. +/// +/// Why every row needs this: at least four paths silently reduce the realized +/// width below the request -- the pre-clamp in +/// `resolve_persistent_decode_threads_with_override`, both headroom +/// reservations, and the single-CPU-cpuset branch that drops decode to the flat +/// path entirely. Only one of them is even `NXRT_CALIB_DEBUG`-visible. Without +/// this line a `t=N` row is an unverified *label*: a sweep that silently pins +/// every width to the same realized value prints a flat line that reads exactly +/// like "this kernel does not scale" (#1763). +/// +/// Reports rather than asserts. A reduced width is legitimate when the host +/// genuinely cannot honour the request (an 8-lane budget in a 2-CPU cpuset), and +/// aborting there would make a constrained container unable to benchmark at all. +/// The `WIDTH-MISMATCH` token is for the caller -- human or matrix script -- to +/// discard or mark the row, the same way a contended cell is marked UNTRUSTED. +pub fn report_decode_width() { + let width = onnx_runtime_ep_cpu::decode_spmd::decode_width(); + let show = |v: Option| v.map_or("unknown".to_string(), |v| v.to_string()); + let verdict = if width.is_as_requested() { + "as_requested" + } else { + "WIDTH-MISMATCH" + }; + println!( + "decode_width requested={} realized={} path={} {verdict}", + show(width.requested), + show(width.realized), + width.path, + ); +} diff --git a/crates/onnx-runtime-ep-cpu/benches/int4_decode_loop_ab.rs b/crates/onnx-runtime-ep-cpu/benches/int4_decode_loop_ab.rs index 35275720a0..dbde911c37 100644 --- a/crates/onnx-runtime-ep-cpu/benches/int4_decode_loop_ab.rs +++ b/crates/onnx-runtime-ep-cpu/benches/int4_decode_loop_ab.rs @@ -366,4 +366,8 @@ fn main() { }; println!("{phase:>10} {ms:>12.3} {p90:>12.3} {total:>14.1} {spread:>9.1}"); } + + // After the phases, never before: the pool is built at first decode, so this + // is the earliest point the realized width exists to be read. + common::report_decode_width(); } From 0471e723b3b35cbae48e2817a7bc4f6f6d303954 Mon Sep 17 00:00:00 2001 From: Justin Chu Date: Sat, 22 Aug 2026 19:34:38 +0000 Subject: [PATCH 2/2] review: correct the reducer list in both docs and split the unresolved verdict Adversarial review caught a claim in the new doc that my own measurement in this branch had already refuted: `reserve_single_group_headroom` does not reduce the realized width. It reduces the *spawned thread* count, but it only runs in the single-group case, which is exactly where `dispatcher_owns_a_shard` (`shards.len() == 1 && shards[0].workers < requested`) is true, so `total_workers` adds the lane straight back. Measured: a 2-lane budget on a 2-CPU cpuset spawns one thread and realizes two lanes, `as_requested`. The genuine net reducers are three, not four: the pre-clamp to `available_parallelism`, `reserve_split_headroom` (NUMA-split only, and uncompensated because the dispatcher shard is single-group-only), and the single-CPU-cpuset fallback. Also corrects the same list in the shipped `DecodeWidth` doc from #1764, which had the mirror-image error: it named both headroom reservations, omitted the pre-clamp, and claimed all three report through `report_spmd_fallback`. Only the cpuset fallback does -- neither headroom function logs anything at all (call sites are :569, :1619, :1642, and neither :1749 nor :1763 is among them). Splits `WIDTH-MISMATCH` into `WIDTH-MISMATCH` (both widths known, unequal -- the row's label is wrong) and `WIDTH-UNRESOLVED` (never resolved -- no decode reached the pool). A matrix script wants to treat those differently. Verified by execution, all three verdicts: t=4 plain -> requested=4 realized=4 spmd-pool as_requested t=8 under taskset -c 0,2 -> requested=8 realized=2 spmd-pool WIDTH-MISMATCH t=1 plain -> requested=1 realized=1 flat as_requested The mismatch arm is the pre-clamp reducer caught in the act. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../onnx-runtime-ep-cpu/benches/common/mod.rs | 31 +++++++++++++------ crates/onnx-runtime-ep-cpu/src/decode_spmd.rs | 17 +++++++--- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/crates/onnx-runtime-ep-cpu/benches/common/mod.rs b/crates/onnx-runtime-ep-cpu/benches/common/mod.rs index 56150ba448..35580d37d6 100644 --- a/crates/onnx-runtime-ep-cpu/benches/common/mod.rs +++ b/crates/onnx-runtime-ep-cpu/benches/common/mod.rs @@ -240,14 +240,21 @@ pub fn init_decode_topology() { /// would build the pool if it were forcing, changing the very topology the /// benchmark is measuring. /// -/// Why every row needs this: at least four paths silently reduce the realized -/// width below the request -- the pre-clamp in -/// `resolve_persistent_decode_threads_with_override`, both headroom -/// reservations, and the single-CPU-cpuset branch that drops decode to the flat -/// path entirely. Only one of them is even `NXRT_CALIB_DEBUG`-visible. Without -/// this line a `t=N` row is an unverified *label*: a sweep that silently pins -/// every width to the same realized value prints a flat line that reads exactly -/// like "this kernel does not scale" (#1763). +/// Why every row needs this: three paths silently reduce the realized width +/// below the request -- the pre-clamp to `available_parallelism` in +/// `resolve_persistent_decode_threads_with_override`, `reserve_split_headroom` +/// on a NUMA split, and the single-CPU-cpuset branch that drops decode to the +/// flat path entirely. Only the last is even `NXRT_CALIB_DEBUG`-visible; the +/// other two log nothing. Without this line a `t=N` row is an unverified +/// *label*: a sweep that silently pins every width to the same realized value +/// prints a flat line that reads exactly like "this kernel does not scale" +/// (#1763). +/// +/// `reserve_single_group_headroom` is deliberately not in that list. It reduces +/// the *spawned thread* count, but only in the single-group case, which is +/// exactly where the dispatcher takes a shard of its own and adds the lane back. +/// A 2-lane budget on a 2-CPU cpuset spawns one thread and still realizes two +/// lanes -- verified, not assumed. /// /// Reports rather than asserts. A reduced width is legitimate when the host /// genuinely cannot honour the request (an 8-lane budget in a 2-CPU cpuset), and @@ -257,10 +264,16 @@ pub fn init_decode_topology() { pub fn report_decode_width() { let width = onnx_runtime_ep_cpu::decode_spmd::decode_width(); let show = |v: Option| v.map_or("unknown".to_string(), |v| v.to_string()); + // Three outcomes, not two: a width that was reduced is a different problem + // from a width that was never resolved, and a matrix script wants to treat + // them differently -- the first invalidates the row's label, the second says + // no decode reached the persistent pool at all. let verdict = if width.is_as_requested() { "as_requested" - } else { + } else if width.requested.is_some() && width.realized.is_some() { "WIDTH-MISMATCH" + } else { + "WIDTH-UNRESOLVED" }; println!( "decode_width requested={} realized={} path={} {verdict}", diff --git a/crates/onnx-runtime-ep-cpu/src/decode_spmd.rs b/crates/onnx-runtime-ep-cpu/src/decode_spmd.rs index 4cf90eb751..94584aab9b 100644 --- a/crates/onnx-runtime-ep-cpu/src/decode_spmd.rs +++ b/crates/onnx-runtime-ep-cpu/src/decode_spmd.rs @@ -1340,12 +1340,21 @@ pub fn decode_path_label() -> &'static str { /// served it. /// /// Three separate mechanisms can quietly hand back fewer compute lanes than -/// were requested -- [`reserve_single_group_headroom`], [`reserve_split_headroom`], -/// and the single-CPU-cpuset fallback in [`build_from_env`] -- and all three -/// report only through [`report_spmd_fallback`], which is `tracing::debug!` or -/// `NXRT_CALIB_DEBUG`-gated. In a default benchmark run they are invisible, so a +/// were requested: the pre-clamp to `available_parallelism` in +/// `resolve_persistent_decode_threads_with_override`, +/// [`reserve_split_headroom`], and the single-CPU-cpuset fallback in +/// [`build_from_env`]. Only the last reports through [`report_spmd_fallback`], +/// which is itself `tracing::debug!` or `NXRT_CALIB_DEBUG`-gated; the other two +/// log nothing at all. In a default benchmark run none of them are visible, so a /// `t=N` row in a results table is a *label* rather than a measurement of width /// `N`. This is the read that turns it back into a measurement. +/// +/// [`reserve_single_group_headroom`] is deliberately *not* in that list. It does +/// reduce the number of spawned threads, but it only runs in the single-group +/// case, which is exactly the case where `dispatcher_owns_a_shard` is true, so +/// the lane it takes is added straight back by the dispatcher's own shard. A +/// 2-lane budget on a 2-CPU cpuset spawns one thread and still reports +/// `realized = 2`, measured, because two lanes really do compute. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub struct DecodeWidth { /// Compute lanes asked for: the explicit `ONNX_GENAI_CPU_DECODE_THREADS`