Repository navigation
The ORT arm escaped the benchmark's own affinity mask (observed) - #1815
Merged
Merged
Conversation
#1811 recorded, as suspected-not-observed, that bench_generic constructs ort_session before the native session load that applies bound_process_to_decode_budget's process affinity mask -- so ORT's intra-op pool is spawned outside the confinement and the native EP's threads inside it. Confirmed by reading Cpus_allowed_list per thread from /proc/<pid>/task/*/status on live paired runs. At T=2 and T=4 every nxrt-task-N worker carries the budget mask and exactly T-1 unnamed threads read 0-31. Those are ORT's workers. At T=16 that is a native arm confined to 16 physical cores while 15 ORT workers roam all 32 logical CPUs, including the SMT siblings of the cores native cannot leave. --ort-intra-threads T equalises the thread counts, not the CPU sets. Also settles the mask's value, which #1811 explicitly declined to assert: T=2 -> [0,2], T=4 -> [0,2,4,6], T=16 -> [0,2,...,30]. scatter_across_cores picks physical-core leaders, so the second mechanism places well and the #1729 pathology is excluded on both mechanisms. Records a verified mitigation: ONNX_GENAI_CPU_DECODE_AFFINITY=off stands the auto-mask down and all threads return to 0-31, symmetric. That is a knock-on for #1792, which reports the same knob as inert on the default SPMD path -- it is not inert here, it is the only switch that disables the self-confinement. Probes are /proc reads, not timings, so contention cannot corrupt them; taken on a busy host without disturbing the owner's matrix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review of the previous commit found this revision repeating, in miniature, the pattern of the two it corrects. 1. "ORT's 15 spinning workers roam all 32 logical CPUs" was written as fact. The T=16 mask is measured, but that launch was --native-only and therefore had no ORT arm, so the census at 16 was never taken. Now marked as carried across from the T-1 rule established at T=2 and T=4. 2. "the second one places well" contradicts 26.2 of this same document, which measured this exact 0,2,...,30 spread as worse (0.133 ms vs 0.079 ms) because straddling two CCXs costs more than SMT sharing when the working set fits in one CCX's L3. Both hold: that penalty is paid by a pool through cross-CCX barrier traffic, and these rows never build one. Stated as the trade-off it is, scoped to these rows, with a pointer to 26.2 for anyone quoting it for a pool workload. 3. Reason 2's heading still led with the mechanism this section goes on to exclude. Flagged inline so the reader does not stop there. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 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.
Follow-up to #1811, which recorded a harness asymmetry as suspected, not observed. It is now observed, and it is the largest of the three.
The claim #1811 left open
bench_genericconstructsort_sessionat:691and thenInferenceSession::loadat:703— and it is that load which reachesbound_process_to_decode_budget()and applies a process CPU affinity mask. Affinity is inherited at thread creation, so ORT's intra-op pool is spawned outside the confinement and the native EP's threads inside it.Observed
Cpus_allowed_listper thread from/proc/<pid>/task/*/status, live paired runs, no outertaskset:T0-31bench_generic, 1 ×nxrt-task-0bench_generic, 3 ×nxrt-task-NEvery
nxrt-task-Nworker carries the mask. The unconfined threads are unnamed — so they show the process name — and there are exactlyT − 1of them: ORT's intra-op pool, which reuses the calling thread.They are not Rayon:
prefill_worker_namenames thosenxgn-prefill-N, and the mask is applied beforebuild_global(matmul_nbits.rs:4380vs:4396), so a Rayon worker would be both named and confined. They are not inter-op (--ort-inter-threadsdefaults to 0). And being unconfined is itself the proof of spawn order — had ORT built its pool lazily at firstRun, after the mask, these threads would carry it.At
T = 16the mask is measured but the census is not (that launch was--native-only, so it had no ORT arm). Carrying theT − 1rule across gives a native arm on 16 physical cores against ~15 ORT workers roaming all 32 logical CPUs — including the SMT siblings of the cores the native arm cannot leave. Marked in the text as inferred, not counted.This does not replace the spin-tax reason, it sharpens it, and it grows with
T— the shape §45.8 read as a native scaling failure.--ort-intra-threads Tdoes not equalise it: it equalises thread counts; the CPU sets differ by construction.Also settles the mask value #1811 declined to assert
--native-threads[0, 2][0, 2, 4, 6][0, 2, …, 30]scatter_across_corespicks physical-core leaders, so the two-workers-per-core pathology of #1729 is excluded on both mechanisms.But not "the mask places well" — an earlier draft said that, and §26.2 of this same document refutes it: that spread measured worse (0.133 ms vs 0.079 ms) for a pinned multi-worker decode pool, because straddling two CCXs costs more than SMT sharing when the working set fits in one CCX's L3. Both hold. That penalty is paid by a pool, through cross-CCX barrier traffic, and these rows never build one. Now stated as a trade-off scoped to these rows, pointing at §26.2 for anyone quoting it for a pool workload.
Verified mitigation
ONNX_GENAI_CPU_DECODE_AFFINITY=offmakesexplicit_decode_affinity_requested()true (it tests non-empty, not the value) and the auto-mask stands down;build_globalstill runs, so pool size is unchanged and only the confinement is dropped. Re-probed atT = 4: all 11 threads read0-31, symmetric. Re-measurements should set it, alongside--native-onlyor--ort-intra-threads 1.Knock-on for #1792, which reports this knob as inert on the default SPMD path: it is not inert here — it is the only switch that disables the process self-confinement. The same variable is dead in one mechanism and load-bearing in the other, which is a worse user-facing story than either finding alone. Sebastian's call whether that belongs in the issue body.
Review
Opus review confirmed the thread identification, the
T−1arithmetic against both data points, the spawn-order argument and the mitigation, and caught two places where this revision repeated — in the safe direction — the over-claiming pattern of the two revisions it corrects. Both are fixed inc195b4fcd: theT=16census is hedged, and the §26.2 contradiction is reconciled rather than left standing.Rule added
Process-wide state applied during setup makes construction order part of the experiment design. "Which arm was built first" silently decided which arm got the whole machine, and nothing in the harness expresses that as a decision — it is the order two
letbindings happen to appear in.Cost
/procreads, not timings, so contention cannot corrupt them — taken on a busy host (runnable 33) rather than queued behind it. Widest arm was one--runs 1native-only launch. No benchmark, no millisecond claimed. Documentation only.