Repository navigation
fix(cpu-ep): size the persistent decode pool by physical cores, not half the cpuset - #1794
Merged
Merged
Conversation
…alf the cpuset
`default_persistent_threads` returned `available / 2`, documented as mapping "to
roughly the physical-core count on SMT hosts". That equivalence only holds when
the process can see every logical CPU. It breaks exactly where operators are
most deliberate about placement: `taskset -c 0,2,...,30` on a 16-core/32-thread
host leaves `available == 16`, all of them already distinct cores, and halving
builds an 8-worker pool on 16 reserved cores.
Verified on this host with the decode harness:
taskset -c 0,2,...,30 before: requested=8 realized=8
after: requested=16 realized=16
and unchanged everywhere the proxy was already right:
unpinned (32 cpus) before: 16 after: 16
taskset -c 0-7 before: 4 after: 4
Take the real physical-core count from `core_topology::allowed_physical_cores`,
which already intersects the detected topology with `sched_getaffinity`, and
fall back to the halving proxy when the topology is undiscoverable. `None` means
unknown and must never mean one, so the result is clamped into `1..=available`
and a nonsensical `Some(0)` from the topology layer is treated as unknown.
The knob is silent about this, which is what let it survive: the EP prints
`decode_width requested=8 realized=8 as_requested`, and 8 really was what the
resolver asked for -- the defect is upstream of the report, in what "default"
resolved to.
Closes #1781.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1794 +/- ##
==========================================
+ Coverage 80.27% 80.92% +0.65%
==========================================
Files 411 411
Lines 200113 200310 +197
Branches 200113 200310 +197
==========================================
+ Hits 160641 162104 +1463
+ Misses 34004 32744 -1260
+ Partials 5468 5462 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…-SMT gap Review of this PR found six doc sites still describing the persistent decode pool as "about half the logical CPUs", which after this change is only the fallback taken when the core topology is undiscoverable. Two of them (`DISPATCHER_RESERVED_CPUS`, `numa_pools`) additionally justified themselves *from* the halving rule, so they no longer argued for what the code does. Also record the evidence gap the review identified rather than leaving it implicit: both measurements behind the old rule -- the "~half the logical CPUs" plateau on a 2-socket Xeon 8480C and 1.4 tok/s at 96 workers against 28.7 at 48 -- were taken on SMT hosts, where half the logical CPUs *is* the physical-core count. Neither distinguishes the two rules, so neither supports this change on a non-SMT host, where the default moves from half the machine to all of it. What makes that safe is not the sizing rule but the consuming path: `reserve_single_group_headroom` turns a request for n on n cores into n-1 spawned workers plus the inline dispatcher's shard, so the fully-subscribed spinning layout behind the 1.4 tok/s cliff is unreachable through this default. Confirmed by direct thread inspection on a 16-core host pinned one CPU per core: 15 `onnx-genai-spmd` threads on 15 leader CPUs, sixteenth core free. Rename `persistent_decode_thread_default_is_half_the_logical_cpus` to say `_when_topology_is_unknown`, which is the path it actually exercises, and drop the `(1, Some(1))` assertion -- review showed it survives every mutation of the function and so falsifies nothing. No behaviour change. Doc warning count unchanged from main (71).
justinchuby
marked this pull request as ready for review
August 23, 2026 02:02
justinchuby
enabled auto-merge (squash)
August 23, 2026 02:03
This was referenced Aug 23, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 23, 2026
…d a knob that no longer exists (#1822) Follow-up to #1173, correcting two defects I shipped in it and repairing the rule they undermined. Docs, one ledger string, one new test, one new script. No production kernel or routing change. ## 1. The ledger named a route gate that had already been deleted `PLAN[MatMulF32].shape_gate` said the native `SimdX86` route "gates M=1 on `ONNX_GENAI_CPU_MM_SIMD_M1_GEMV` (default off, #1116)". #1183 shipped that GEMV on by default and removed the probe. `git merge-base --is-ancestor 5417d04 bdb4599` confirms it landed **before** #1173 merged — so the ledger was wrong the day it landed. Today `sgemm_simd` calls `sgemm_simd_variant(a, b, c, m, k, n, true)` unconditionally and `use_m1_gemv` is a plain parameter that only the in-process A/B harness passes as `false`. No environment variable reaches that route. `docs/performance/CPU_MATMUL_ASSIGNMENT.md:559` already recorded the correct fact ("It is measured now, and the route is the default. There is no env probe on the dispatch any more"). Two files in the same directory disagreed and nothing compared them. **Now guarded.** `ledger_prose_only_names_environment_variables_that_still_exist` requires every `NXRT_*` / `ONNX_GENAI_*` token in the ledger's prose to still exist as a string literal in the crate's sources. It cannot check that the description is *right*, only that the knob is *real* — which is the half that goes stale silently. Mutation-verified, not just observed green: ``` matmul_f32: ledger prose names environment variable `ONNX_GENAI_CPU_MM_SIMD_M1_GEMV`, but no source file in this crate contains the literal "ONNX_GENAI_CPU_MM_SIMD_M1_GEMV". ``` ## 2. The doc published a toggle A/B that could not have been run #1173 carried a table captioned **"same binary, same session, toggle the only difference"**, reporting `decode 1×2048×2048` at 0.146 with `ONNX_GENAI_CPU_MM_SIMD_M1_GEMV` off against 0.337 with it on, and called turning it on "the obvious next slice". Nothing reads that variable. Setting it measures the same route twice; it cannot produce two different columns. The table is withdrawn and the retraction kept in the text rather than quietly deleted. This is the failure mode the document's own graduation rule warns about — **an arm that was not on the route it was labelled with** — committed by the document that wrote the rule. It survived review because a plausible number in a well-formed table is not self-evidently unmeasured. Readers are pointed at `bench_f32_gemm_ab`, which holds the route as a function parameter and carries the M≥2 rows as a built-in control. ## 3. The gap table is re-measured and the ≥5% rule is repaired The old table was one unguarded invocation per row at an unstated width, taken before the decode-placement corrections (#1729, #1794, #1811) — i.e. when the decode pool put 16 workers on 8 physical cores. New harness: `scripts/bench_native_vs_mlas_width.py`. Arms interleaved rep by rep so host drift lands on both equally; per-rep `os.wait4` CPU-efficiency guard adapted from #1809; six reps per arm; two widths. Raw verdicts, spreads and discards are all reported rather than summarised away. **Three findings, all about method rather than kernels.** | | narrow (6 cores, 1 L3) | wide (32 logical CPUs) | |---|---|---| | `matmul_f32 16×512×512` | 1.581, spread 41% | 0.866, spread 134% | | `matmul_f32 decode 1×2048×2048` | 1.117, spread 21% | 0.934, spread 13% | - **Two cases change verdict on width alone.** Same binary, same half-hour, only the CPU mask differs. `x86_sgemm` parallelises over column strips and MLAS declines to parallelise some shapes, so interleaving the two *routes* inside one process does not protect the ratio — it changes both at once. - **`16×512×512` disagrees with itself on both arms**, alternating `keep-mlas` / `native-graduates` from a byte-identical binary. **One more run of the old table could have graduated a route on this row.** - **The narrow arm is more trustworthy despite having fewer cores** — spreads 4–42% against 5–134%, and it lost no reps to the guard. Isolation beat parallelism. **Softmax now decomposes cleanly**, because no vendored MLAS kernel has changed since #1173 (the only `mlas-sys` edits are the additive straggler handshake in `work_stealing_pool.rs`, #828/#1714, which adds waiting). At matched width the MLAS control arm is stationary to within 4% while native improved **1.24–1.27×** — matching #1416's claim for the row kernel. The f32 GEMM rows get no such attribution and now say so explicitly: their control moved **2.0× the wrong way**, so only the current ratio at a stated width is defensible. **The rule gains what it lacked**: spread must be smaller than the claimed win; reps that did not get the CPU are discarded rather than averaged; a verdict is valid only at a stated width. Under it, `decode 1×2048×2048` — the first f32 GEMM case to show a real native win — **still does not graduate**: it costs more CPU (cpu_ratio 0.875), does not hold at 32 threads, and its 21% spread exceeds its 12% win. ## The width claim is verified, not asserted #1815 landed while this was in progress and observed the neighbouring `bench_generic` harness spawning its ORT arm *outside* the affinity confinement it applied to the native arm. That hazard applies to any `taskset` claim, including mine, so I checked it instead of trusting it — sampling `Cpus_allowed_list` from `/proc/<pid>/task/*/status` 40× across a live narrow-arm run: ``` '16,20,22,26,28,30': 478 observations native_vs_mlas- 273, mlas-sys-ws-0..4 39 each, nxrt-task-0..4 2 each '0-31': 1 (the taskset process itself, before exec) ``` Both routes confined identically; no thread escaped. The rule now requires this check. ## Validation - `dispatch_ledger` **17/17**, including the new falsifier, after merging latest `main`. - `default_artifacts_are_mlas_free` **9/9** — the no-MLAS-in-defaults invariant is untouched. - `cargo clippy -p onnx-runtime-ep-cpu --lib --all-targets` clean; `cargo fmt --check` clean. - Normal merge of `origin/main` (`aee2b9d11`), no rebase, no conflicts. ## Limitations - The narrow arm is six cores on one L3 of one x86-64 host. Nothing here transfers to aarch64 or to a two-socket box. - The `activations erf 1 Mi` row shows native 13.5% slower at matched width. The nearest scatter figure is the wide arm's 8% spread, but that is a spread of *ratios* against a move in a *native time*, so the two are not strictly commensurable. Its MLAS control also moved 11%. **Flagged for pinned re-measurement, not reported as a regression.** - The wide arm was taken with ~4–5 cores of unrelated load present. That is stated in the doc rather than hidden, and it is why its spreads are wider; the guard reports which reps were discarded instead of pretending the host was quiet. - No production behaviour changes here, so there is no performance claim to make about the shipped artifact. Refs #1173, #1183, #1809, #1815, #1416. ## Independent review, and what it changed An independent adversarial review of the full diff returned **no blockers** — it confirmed the ancestry argument behind the retraction, the stationary-control premise for the softmax attribution, and that the headline case is correctly *refused* by the rule (21% spread against a 12% win). It also found seven real defects, all now fixed in `f0323f9ed`. The one that mattered most was in the new test. It only proved the variable name appeared *somewhere* in the crate, so a variable whose read site had been deleted but whose name survived in an `EnvVarGuard::set(...)` line would still have passed — which is the precise shape of the defect this PR exists to correct. The test now requires the matching line to be an `env::var(` / `env::var_os(` read or an `_ENV: &str =` binding. Verified by mutation in **both** directions: | mutation | before | after | |---|---|---| | reinsert retired `ONNX_GENAI_CPU_MM_SIMD_M1_GEMV` into ledger prose | fails ✅ | fails ✅ | | retire the two real `NXRT_CPU_GEMM_BACKEND` reads, leaving the literal only in test guards | **passes ❌** | fails ✅ | The remaining six were prose defects in the doc: a stated spread range that contradicted its own table's 82% row, "within 4%" against a table reading −4.2%, a narrow-arm ratio fused with a wide-arm attribution, a spread quoted as 7.5% that was 8% *and* compared against an incommensurable quantity, the CPU-efficiency guard oversold as "what makes this table measurable at all" (in-process interleaving is what protects the ratio; the guard catches only *differential* descheduling), and a one-directional provenance argument standing in for the direct control measurement that actually carries the softmax attribution. **Two further defects I found myself while checking the tables against each other**, neither raised by the review: - The `ratio` column is a median of per-rep ratios while the `ns/unit` columns are medians of times. Medians do not distribute over division, so every row looked internally inconsistent to anyone who tried to divide it out (`0.0684 / 0.0617 = 1.109` against a stated `1.117`). Now documented, along with why the per-rep form is the correct one to quote: it pairs each MLAS invocation with the native invocation it was interleaved against, which is the entire point of interleaving. The then→now figures are relabelled as quotients of medians. - "wider than nine of the twelve wide-arm rows" was eleven of twelve. ## Adopting #1814 `aee2b9d11` (#1814) landed on `main` while this was in review, and it closes the exact hole the review found in the guard this document recommends. A differential CPU-efficiency check cannot see contention that lands evenly on both arms; #1814's confined-set meter reads busy jiffies on the process's own `Cpus_allowed_list` and subtracts the process's own CPU, so foreign load shows up directly. The rule now points at it, and the tables here are explicitly marked as predating it and guarded by the weaker method. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 23, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 24, 2026
…sting the width label (#1937) A cross-agent placement report had the default decode pool pinning **16 workers to cpus 0–15** — two workers per physical core on a single 32 MiB L3, half the machine idle — and offered #1729 as the fix. **#1729 is merged** (`6e8c31ebd`, 2026-08-23T01:11:35Z), as is the related width-halving fix #1794 (`0652fdd2e`). Both are ancestors of `origin/main`. The report describes a pre-#1729 build. Rather than argue from the diff, this adds the categorical instrument that settles it. ## `benches/decode_placement_census.sh` Reads `Cpus_allowed_list` for every `onnx-genai-spmd` thread in three configurations. It is a `/proc` read, not a benchmark — it does not need a quiet host and no number in it is a timing. It still takes the hostlock as a courtesy, since it does spin the pool. Measured on `0a668d54b`, **reproduced identically three times**: | configuration | spawned workers | pinned CPUs | L3 spread | |---|---:|---|---| | default, no `taskset`, no env | 15 | `0,2,4,…,28` | 8 in L3#0, 7 in L3#1 | | `THREADS=16` under the even mask | 15 | `0,2,4,…,28` | 8 in L3#0, 7 in L3#1 | | `THREADS=8` under the even mask | 7 | `0,2,…,12` | 7 in L3#0 | **One worker per physical core in every case**, with the reserved dispatcher CPU (`30` at width 16, `14` at width 8) left clear — exactly what `decode_affinity::order_pin_targets` and `reserve_single_group_headroom` specify. The tell was visible in the original report without any re-measurement: its two arms reported **16** and **15** shard participants, and 15 spawned workers plus an inline dispatcher is precisely what current main builds. ## What it turned up in my own record `ONNX_GENAI_CPU_DECODE_THREADS=8` confines the process to `[0,2,4,6,8,10,12,14]` — **entirely inside one 32 MiB L3 instance** — while width 16 spans both. So `t=8 → t=16` on this host doubles cache and memory-controller reach as well as cores. That was not written down anywhere and now is, in both the benchmark record and the ledger. **It is not a confound.** `acc0_gap_matrix.ort()` defaults its pin to `native_pin(threads)`, so both arms get the same CPUs at each width, and both the 1.762x (ORT) and 1.319x (native) scaling figures post-date that fix (`4b4dacc7e`). It does make the 2.0x ideal a *conservative* reference for both arms, which leaves the finding understated rather than overstated. ## `benches/cpu_work_probe.py` A second, independent reason not to read `/usr/bin/time -v`'s `Percent of CPU` as utilisation here, beyond its being wall-derived: on an SMT host a logical CPU whose sibling is busy is granted a **full 100% share while delivering roughly half the work**, and no CPU-time instrument can see it — the scheduler really is handing over the CPU; the contention is in hardware, below its view. Only a work-completed probe distinguishes them. The permanent cpu-0 competitor reported alongside the placement claim **did not reproduce**: cpu 0 reads `cpu_share` 0.999–1.000 at 9429/9482/9489 iterations, inside the 8744–9499 band spanned by ten other CPUs, with one transient outlier that two re-probes cleared. On a host shared by several agents, "permanent" was load rather than topology. The instrument point is the durable part and is what the ledger records. ## Scope Docs and bench scripts only — no compiled code changes. `shellcheck` clean, `ruff` clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 25, 2026
…as pinned to (#2059) Closes the end-to-end hole left by #1794's fix for #1780. ## The gap #1780: the persistent decode pool sized itself as `available / 2`, documented as approximating the physical-core count on an SMT host. That approximation only holds when the process can see every logical CPU. Under `taskset -c 0,2,4,…,30` every allowed CPU is *already* a distinct physical core, so `available == 16`, halving gives 8, and the pool built **8 workers on the 16 cores the run had deliberately reserved**. #1794 fixed the resolver and added unit tests over `default_persistent_threads(available, allowed_physical_cores)`, including the pinned case `(16, Some(16)) == Some(16)`. **Those tests cannot catch the regression that matters.** They call the pure function with the correct second argument already in hand; the defect was in what reached it. Measured rather than argued — plumb `None` into the call site at `matmul_nbits.rs:4528`: ``` test result: FAILED. 1744 passed; 1 failed; 25 ignored failures: kernels::matmul_nbits::tests::a_default_width_pool_on_leader_cpus_uses_every_core_it_was_given ``` One test in 1770 catches it. All five `default_persistent_threads` unit tests pass with the defect present, because none of them builds a pool. ## What this adds A test that builds one. It: 1. restricts the child to one CPU per physical core **from inside the child**, so the mask is an observable the child reports (`allowed`/`cores`) rather than a `taskset` dependency; 2. leaves the width unset — `ONNX_GENAI_CPU_DECODE_THREADS` and `RAYON_NUM_THREADS` are explicitly `env_remove`d, since inheriting either would quietly turn this into a second explicit-width arm that always passes; 3. asserts **`workers == cores`** and a realized placement of `one-per-core`. It also asserts `allowed == cores` *first*. Without that, a failed restriction would leave the child on the full cpuset where `workers == cores` holds for the wrong reason and the real assertion would be testing nothing. ## Negative control, both directions | | result | |---|---| | defect reintroduced | fails: `built 8 workers on 16 reserved cores` | | reverted | 1745 passed, 0 failed | ## Why this shape Throughout #1780 the EP printed `decode_width requested=8 realized=8 path=spmd-pool as_requested`. That line was **honest** — 8 really was what the resolver asked for — and every guard and log agreed with it. The defect sat *upstream of the report*, in what "default" resolved to, so no amount of self-reporting could surface it. This is the same reason the existing sweep asserts what the kernel did rather than what the pool says about itself. It also matters for the benchmark record specifically: pinning one CPU per physical core is the discipline adopted to get clean numbers, so the runs most likely to be published were exactly the runs at risk. ## Scope note My own published native rows are unaffected — all three bench harnesses (`acc0_gap_matrix.py`, `acc0_w16_worker_split.py`, `acc0_w16_chunk_permutation.py`) set `ONNX_GENAI_CPU_DECODE_THREADS` explicitly on every launch, and an explicit width bypasses the default resolver. This test is to keep that from mattering. Refs #1780, #1794. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 25, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 25, 2026
#1729 (#2150) ## Why A cross-agent note asked for **every `t>=8` row published before #1729 (`6e8c31ebd`) to be re-taken**, on the grounds that the default decode pool pinned 16 workers to cpus 0–15 — 8 physical cores with both SMT siblings loaded. The premise is correct, and it is the same defect this repo found independently and filed as **#1680** (§24 of `CPU_MATMUL_ASSIGNMENT.md`). The blanket conclusion is too broad for the rows in that file, and — the point of this PR — **which rows survive is decidable from the source, without spending host time re-measuring.** ## The argument Every multi-thread timing in §23/§25 is `taskset`-pinned to the even CPUs (§24's closing note). On this host SMT siblings are adjacent pairs, so that mask is 16 CPUs that are *already* 16 distinct physical cores. **The spread half of #1729 is the identity on such a mask.** Pre-#1729 the SPMD shard builder used `allowed_cpus()` in raw ascending order (confirmed at `6e8c31ebd^`); post-#1729 the same list goes through `order_pin_targets`. Its `Spread` arm is `leaders_within(cpus)` plus the non-leader remainder — and on an all-even mask each core group contributes its single allowed member, the remainder is empty, and `leaders_within` ends `sort_unstable(); dedup()`. Same ascending list. `build_decode_pool` pins worker `i` to `cpus[i % len]` either way, so **#1729 cannot move a number taken under that pin.** **The reserve half is the identity on that mask too.** This is a correction to the first version of this PR, which claimed `t=16` went 16 workers -> 15. It does not. #1729 did not *introduce* the dispatcher reserve; it changed it from a logical-CPU rule to a physical-core rule: ``` pre (6e8c31e^): total < allowed ? total : allowed - 1 post (core budget): min(total, cores - 1).max(1) ``` When the mask holds one CPU per physical core, `allowed == cores`, and the two agree at **every** width: below saturation `min(total, allowed-1) == total` because `total <= allowed-1`; at and above saturation both give `allowed-1`. They diverge only when `allowed > cores` — a mask holding both SMT siblings, which is exactly the unpinned case #1729 was written to fix and exactly the case these benchmarks are not run in. I got this wrong in the characteristic way: I read the post-#1729 formula carefully and **assumed** the pre-#1729 one, then published a delta with only one side measured. An adversarial review accepted the wrong table; fetching `6e8c31ebd^` is what caught it. It is in the ledger under its own heading because it is the same error class the rest of this file is about. **#1794 does not apply.** It fixed `default_persistent_threads` returning `available / 2`. These sweeps set `ONNX_GENAI_CPU_DECODE_THREADS` explicitly, and an explicit count bypasses the default entirely. The rows that defect corrupted are pinned runs that left the width to the default — of which this file has none, because the pin and the explicit width were adopted together. **Disposition: no row taken under the even-CPU pin moves at any width** — both halves of #1729 are the identity there, and #1794's defect lived in a default these sweeps never used. `t=16` stays withheld for its own unrelated reason (its A/A null spans 0.969–1.295, ±30%, against 3.6% at `t=1`). The placement question and the instrument question are independent, and only the second ever disqualified that row. This is a *stronger* claim than the one asked for, so it carries a stronger obligation: it holds **because of the pin**, and says nothing about an unpinned row. Unpinned numbers remain governed by §24's standing rule and are not rehabilitated by anything here. ## The tests, and why they are the real content Both arguments were **load-bearing and untested**. `cargo test order_pin_targets` matched **zero** tests, and nothing asserted the reserve invariant either. **1. `a_one_cpu_per_core_mask_is_unchanged_by_either_placement_policy`** — a cpuset already holding one CPU per physical core is a fixed point of both placement policies. This is the guarantee *every* pinned benchmark in this repository rests on: the house rule for a clean multi-thread number is `taskset` to one CPU per core, and it is worth nothing unless the pool then pins workers to the CPUs that were reserved, **in the order reserved**. Also asserts the policies still disagree on a *full* mask, so it is a property of the mask rather than a policy that never reorders. Live: reversing the leader order inside `Spread` fails it with ``` `spread` reordered a mask that was already one CPU per core ```. **2. `the_core_reserve_matches_the_logical_reserve_on_a_one_cpu_per_core_mask`** — carries the pre-#1729 logical rule as a reference implementation and asserts agreement across masks 1..32 at every total, plus the contrasting SMT-mask case where the rules genuinely differ (`(16, 32, 16) -> 15` vs logical `16`). A future change to the reserve that breaks this would silently invalidate a file full of measurements; this makes it break in CI instead. Live: reserving two CPUs instead of one fails it at `total=3` on a 4-CPU mask. ## Also recorded `t=2` is **closed**, by two methods sharing no apparatus: **1.96x** measured here on a quiet host (20.447 vs 40.039 ms/token, 0.6% A/A null, both workers 99% busy by per-thread attribution) and **1.94x / 97% efficiency** reported independently by the runtime owner on a post-#1729 baseline. ~1% apart. The withdrawn "71% of one core" is now over-determined. One standing caveat is reinforced rather than revised: `t=1` runs `path=flat` and `t>=2` runs `path=spmd-pool`, so a `t=1` vs `t=2` comparison crosses routes as well as widths. §20 already reads that row as "vs serial" rather than "vs a one-worker pool". ## Validation - `cargo test -p onnx-runtime-ep-cpu --lib` — **1829 passed, 0 failed** (two new tests), under `scripts/hostlock.sh` - `cargo clippy -p onnx-runtime-ep-cpu --all-targets --all-features -- -D warnings` — clean - `cargo fmt --all --check` — clean No production code changed: two tests plus a documentation section. --------- 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
1 participant
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 #1781.
The defect
default_persistent_threadssizes the persistent SPMD decode pool and returnedavailable / 2. Its doc justified that as mapping "to roughly the physical-core count onSMT hosts", which is true only when the process can see every logical CPU.
It breaks precisely where an operator has been most deliberate about placement. On a
16-core/32-thread host,
taskset -c 0,2,...,30selects 16 CPUs that are already 16distinct physical cores.
availableis 16, halving gives 8, and the pool builds 8workers on 16 reserved cores. The proxy cannot distinguish a 16-CPU SMT-free set from
half of a 32-CPU SMT set, and it guesses wrong on the first.
Measured with the decode harness, reading the EP's own width line:
taskset -c 0,2,...,30requested=8 realized=8requested=16 realized=16requested=16 realized=16requested=16 realized=16taskset -c 0-7requested=4 realized=4requested=4 realized=4Only the case the proxy got wrong changes. The two where halving already agreed with the
core count are byte-identical.
Why it went unnoticed
The EP reports
decode_width requested=8 realized=8 path=spmd-pool as_requested. Thatline is true: 8 is what the resolver asked for and 8 is what it got. The defect is
upstream of the report, in what "default" resolved to, so every existing width assertion
and every log line agrees with each other and with the wrong answer.
This matters for the benchmark record specifically, because pinning one CPU per physical
core is the standard way to take a clean measurement on this host — so the runs most
likely to be published are exactly the runs that silently ran at half width.
The fix
Use
core_topology::allowed_physical_cores(), which already intersects the detectedsibling map with
sched_getaffinity(so containers,taskset, and the crate's ownbound_process_to_decode_budgetconfinement are all reflected), and keep the halvingproxy only for when the topology is undiscoverable.
Nonefrom the topology layer means unknown, never one — capping on a guess is how a32-thread host silently becomes a 1-thread host — so the result is clamped into
1..=available, and a nonsensicalSome(0)is treated as unknown rather than used tobuild a zero-worker pool.
default_persistent_threadsstays pure: the core count is a parameter, and onlyconfigured_persistent_decode_threadsconsults the host. That keeps the policy testableon a machine with any topology, including the SMT cases this host cannot produce.
Tests
persistent_decode_default_uses_physical_cores_not_half_the_cpusetcovers the defect(
(16, Some(16)) -> 16), the unpinned SMT case that must not change(
(32, Some(16)) -> 16,(96, Some(48)) -> 48), the clamp ((4, Some(9)) -> 4), thedegenerate topology answer (
(8, Some(0)) -> 4, i.e. fall back), and theunknown-topology path (
(16, None) -> 8, historical behaviour exactly).The existing
persistent_decode_thread_default_is_half_the_logical_cpusis retainedunchanged in meaning, now passing
None— it pins the fallback so a future change cannotquietly drop it.
Full crate suite: 1644 passed, 0 failed. Clippy clean.
Relationship to #1729
#1729 (merged as 6e8c31e) fixes where the workers go — one per physical core, with a
core reserved for the dispatcher. This fixes how many there are under a pinned cpuset.
They compound: without this, a one-CPU-per-core
tasksetgave #1729 only 8 workers tospread across 16 reserved cores, capping its benefit.
Where the evidence stops (raised in review)
The table above is from an SMT host, and it understates the footprint. The default also
changes on every non-SMT host, even unpinned: there
cores == available, so the defaultmoves from half the machine to all of it.
That case has the least evidence behind it. Both measurements the old rule cited — the
plateau at "~half the logical CPUs" on a 2-socket Xeon 8480C, and 1.4 tok/s at 96 workers
against 28.7 at 48 on a 96-logical-CPU host — were taken on SMT hosts, where half the
logical CPUs is the physical-core count. Neither distinguishes the two rules. No non-SMT
host was available to measure. The old rule was equally unevidenced there, so this is a
wash on rigor rather than a demonstrated improvement, and it is now recorded in the
docblock instead of left implicit.
What makes it safe rather than merely plausible is the consuming path, and this part was
verified by execution rather than argued. Under
taskset -c 0,2,...,30— 16 CPUs that are16 distinct cores, i.e. exactly the fully-subscribed shape the cliff measurement warns
about —
reserve_single_group_headroom(16, 16)returns 15, and/proc/<pid>/taskinspection during a live run shows 15 threads named
onnx-genai-spmdpinned to cpus0,2,...,28 with cpu 30 left free for the inline dispatcher. One thread per physical core,
zero oversubscription. The reserve, not the sizing rule, is what forecloses the cliff.
Two caveats on how to read
realized=N: it counts the inline dispatcher's own shard, sorealized=16is 15 spawned threads plus the dispatcher, not 16 spinning threads. And thenuma-splitopt-in path reserves one logical CPU per node rather than a core, so on anSMT host its on-node dispatcher still gets a sibling instead of a free core — tracked as
#1791, not fixed here.
What the extra workers buy, and why this is a distribution not a number
Latency, same pin (
taskset -c 0,2,...,30), llama width-default, zero gap, 400 tokens,5 reps, 2 warmups, interleaved with an A/A null arm. Both arms are built on a main that
already contains #1729, so this isolates width and nothing else.
realized=8realized=16Base-arm launches (n=8, pooling base and the null): min 4.742, median 4.86, max 7.662.
Fix-arm (n=4): min 2.439, median 3.553, max 6.107. The fix arm's median sits below the
base arm's minimum, and its worst launch beats the base arm's worst.
But do not read the row-by-row pairs as effect sizes. The A/A null disagrees with base
by up to 59% at the same rep (launch 3: base 7.662, null 4.807), because both arms are
bimodal per process launch. Any single pairing can be dominated by which mode each launch
happened to land in. The distributional shift is real; a point estimate from this data
would not be.
CPU per token is flat across arms (40–48 vs 42–50 ms). The fix converts otherwise-idle
reserved cores into throughput at roughly constant CPU cost, which is the expected shape
for a width correction rather than a scheduling one.
Note also that the bimodality persists with #1729 merged, in both arms. Correct
placement was a large part of it, but not all of it; the residual is unexplained and is
tracked with the harness in #1780 rather than here.
The claim this PR actually rests on is the deterministic one: under a one-CPU-per-physical-core
pin, the pool built 8 workers on 16 reserved cores and now builds 16. That is read from
the EP's own width line, needs no statistics, and reproduced on every launch.