Repository navigation
perf(cpu-ep): stop QLinearMatMul re-allocating its buffers on every call - #1133
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1133 +/- ##
========================================
Coverage 79.94% 79.95%
========================================
Files 368 368
Lines 160794 160899 +105
Branches 160794 160899 +105
========================================
+ Hits 128551 128640 +89
- Misses 27524 27541 +17
+ Partials 4719 4718 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
At K=N=2048, M=128 the kernel was 2.07x ORT with a 16-thread pool but at parity on one thread, so the deficit was never arithmetic. Splitting the call showed MLAS's fused GEMM+requantize scaling 7.2x (8.17ms -> 1.13ms) while everything our wrapper did around it grew from 145us to 576us -- 34% of the call -- as threads were added. The cause is per-call buffer churn, which is cheap on one thread and expensive on sixteen. Every call allocated `A` (m*k), an `i32` accumulator (m*n*4) and a staging copy of the result (m*n), zero-filled them, and freed them. Freeing a fresh multi-page mapping forces a TLB shootdown to every core the process runs on, and MLAS first-touches the accumulator from every worker, so the cost rises with the pool size rather than staying a fixed overhead. Three buffers, three fixes, none of which changes an output byte: * `A` is borrowed when the view is already dense instead of being copied. Only the sign-flip route rewrites `A`, and that still takes a private copy, because writing through to the caller's input would be a bug. * the result is requantized straight into a contiguous output tensor. A strided or non-host output still stages and scatters exactly as before. * the `i32` accumulator is parked on a thread-local between calls, bounded at 32 MiB so an outsized shape is released rather than retained. It is thread-local rather than shared because `execute` takes `&self`, and a lock would serialise the calls this is meant to speed up. Measured on a 32-vCPU EPYC 9V74, K=N=2048 M=128, both pools pinned: | threads | before | after | | --- | --- | --- | | 1 | 8.31 ms | 8.29 ms | | 4 | 2.57 ms | 2.35 ms | | 16 | 1.70 ms | 1.17 ms | Wrapper cost at 16 threads falls from 576us to 54us. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…appened The review pointed out that the `flip_a` special case -- taking an owned copy of `A` before the sign flip -- is redundant: the flip goes through `Cow::to_mut`, which copies a borrowed operand before writing to it, so the caller's input is safe either way. Removing the special case was confirmed by experiment to leave the test passing, which means the case was buying nothing and costing a `to_dense_bytes` -- the zeroed allocation this PR set out to avoid -- on the signed-A-by-unsigned-B route. Dropping it leaves that route paying a plain `to_vec` at `to_mut`, and every other route borrowing. The test that guards this could pass vacuously: if `dense_bytes` ever stopped borrowing, the caller's `A` would trivially survive and the assertion would prove nothing. It now also asserts a borrow counter, and forcing `dense_bytes` to always copy makes it fail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ef58b51 to
8572826
Compare
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
|
Decomposition is right and the gates pass on my box ( But the retention bound is stated per-call and the buffer is retained per-thread.
and
The doc comment reasons entirely in single-buffer terms -- "32 MiB covers every accumulator up to an 8M-element result" -- and never multiplies by the pool width. But
The one bound in the PR is the one number that is not the process's exposure, and it is off by the pool width -- which is exactly the variable this PR is about, since the whole finding is that per-call buffer cost scales with thread count. The fix's ceiling scales with the same variable as the bug's cost. This is the third instance of one shape, and I want to name it because it keeps costing us a round:
Each time the arithmetic in the comment was right about one copy and silent about N. Under-reporting is worse than reporting zero: zero is obviously blind and gets caught at review, whereas a plausible-looking 32 MiB passes admission and then overruns. What I need before merging
Note that The speed work stands on its own and I expect it to merge once the ceiling is honest. |
The weight-cache guard failed the test it exists to pass. #1133 parked a QLinearMatMul i32 accumulator in a thread_local! RefCell<Vec<i32>> bounded by a per-buffer 32 MiB constant, while the buffer is retained on every worker thread for the life of the process: 640 MiB on a 20-thread box, 1 GiB on a 32-vCPU one, 4 GiB on a 128-vCPU one. Run against that PR's 459 added lines the existing regex matched zero, because it only knew OnceLock/OnceCell/LazyLock. That is the guard committing, one level up, the defect it was written to catch: a check that is green but structurally incapable of failing on the case that motivated it. The new job does not merely also-match thread_local. It asks the question the first job does not -- what multiplies this buffer -- because the underlying error has now cost four rounds: #1051 reported 247 MB against 592 MB measured, #1100's ratio test drove a single instantiation so it could not observe the x2, and #1133 bounded one copy of an N-per-thread buffer. Each comment was correct about one copy and silent about N. Under-reporting is worse than reporting zero: zero is obviously blind and gets caught at review, whereas a plausible 32 MiB passes admission and then overruns. The error text also requires that any test for such a buffer drive it from more than one thread, since a single-instantiation test cannot observe an xN multiplier -- which is exactly how #1100 shipped with the factor unmeasured. Falsified against real history rather than assumed to work: #1133 (must fail) matches=1 -> flagged #1143 (must pass) matches=0 -> passes #1142 (must pass) matches=0 -> passes The pattern is rare in this tree (3 occurrences), so the false-positive cost is low, and the 'per-thread-bound-reviewed' label records a deliberate judgement rather than blocking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
#1140) ## What Our `f16` `MatMul` kept both operands in 16-bit storage and ran the portable **blocked half GEMM**. That saves bandwidth while the operands dominate, but the blocked kernel has no tuned microkernel, so once `M` grows enough for the GEMM to become compute-bound it loses badly. **ORT does not do this.** Measured on this host at `M=128, K=N=2048`, ORT spends **14.27 ms on f16** and **14.39 ms on f32** — the same number, because it widens `f16` to `f32` and reuses the same tuned SGEMM. We spent **28.2 ms**. This declines the blocked half GEMM for `f16` above a measured crossover and lets every caller fall through to the widened-`f32` SGEMM they already have. All four `try_matmul_half` call sites already had that fall-through; none needed a new path. ## Result — plugin-level A/B vs real ORT `bench_matmul_f16_m128`, `M=128, K=N=2048`, **non-constant B**, pinned to 16 physical cores, 5 interleaved rounds of before/after/ORT **in one process**, p50 ms. Ratios are **ours/ORT**, so >1 means we lose. Ranges span **three independent measurements**, one by the reviewer on a separate build: | threads | before | after | gain | |---|---|---|---| | 1 | **1.97x–1.99x slower** | **1.05x–1.08x** | **1.85x–1.87x faster** | | 16 | **3.27x–3.29x slower** | **2.03x–2.28x** | **1.44x–1.63x faster** | `M=1` decode cannot reach the gate (`1 < 16`) and measured unchanged: **0.356 ms after vs 0.353 ms before** at T=1, inside noise (ORT 0.670, so decode remains a **0.53x win** for us). ### What is *not* fixed **T=16 is still ~2x off ORT, and this PR does not claim that range.** The residual is the per-call widen of a non-constant `B` (`4·K·N` = 16 MiB here): serial in our `to_dense_f32_widen`, parallel in ORT. Evidence — our f16 costs ~2.2 ms more than our own f32 at T=16 but only ~1.6 ms more at T=1, i.e. the conversion gets *worse* with threads (fresh-mapping page faults, the mechanism #1133 documented). **That range stays on the open work list.** Constant/initializer `B` — what real LLM weights are — was already handled by `try_packed_half_prefill` and is untouched. This PR fixes the **dynamic-B** hole. ## How the thresholds were set Both constants come from `bench_f16_half_vs_widen` (added here), which times the two *actual* routes. Pinned, median of 5. Ratio is `half/widen`, so >1 means widening wins. **`HALF_WIDEN_MIN_M = 16`** — M sweep at `K=N=2048`, two independent runs: | M | T=1 | T=16 | |---|---|---| | 2 | 0.67x, 0.61x | 0.93x, 0.83x | | 8 | 1.01x, 1.05x | 1.26x, 1.38x | | **16** | **1.33x, 1.30x** | **2.14x, 1.85x** | | 32 | 1.56x, 1.63x | 3.14x, 3.32x | | 128 | 1.90x, 2.00x | 3.47x, 3.30x | `M=16` is the first row that wins repeatably at **both** thread counts. **`M=8` is a tie at T=1 (1.01x/1.05x) and is deliberately left unclaimed** despite its T=16 win. **`HALF_WIDEN_MIN_WEIGHT = 256` elements** — weight sweep at the minimum claimed `M=16`: | K×N | elements | T=1 | T=16 | |---|---|---|---| | 8×8 | 64 | 0.88x (half wins) | 275x | | **16×16** | **256** | **1.16x** | **229x** | | 32×32 | 1024 | 1.21x | 35.9x | | 128×128 | 16384 | 1.48x | 16.3x | | 256×256 | 65536 | 1.52x | 7.8x | The huge T=16 ratios are **not** a widening win — they are the blocked half GEMM forking a parallel region to multiply an 8×8 matrix (0.27 ms!). Widening sidesteps that; fixing the half path's own small-work threshold is separate and not attempted here. ## Scope — what deliberately does *not* change - **`bf16`** keeps the blocked kernel: its crossover was never measured, and widening bf16 is a different (shift-only) operation, so the f16 measurement does not transfer. - **Non-MLAS backends** keep today's behaviour. - **`M < 16`**, including all decode, is bit-for-bit unchanged. - Nothing is ever handed back to the ORT CPU EP — declining here falls through to a local widened path, never to an error or an unsupported op. ## Tests Value-only tests cannot detect this gate being mis-wired, so the tests assert **which route ran** via a thread-local counter. Every fault injected — by me and independently by the reviewer — is caught: | injected fault | caught | |---|---| | gate disabled (`&& false`) | ✅ | | gate always true | ✅ | | off-by-one (`>` for `>=`) | ✅ | | inverted (`<` for `>=`) | ✅ | | `bf16` wrongly included | ✅ | | weight clause removed | ✅ | | counter increment deleted | ✅ | | oracle perturbed 1% | ✅ | Plus `f16_widened_route_matches_an_f64_oracle_across_the_crossover` at `M = 15, 16, 19` with odd `K=521, N=517` for tails. The route test also asserts `auto_detect() == Mlas`, so the gate cannot silently become dead code. ## Review fixes (all from the independent review) - **MINOR-1** — the oracle tolerance was `2e-3·√K` = **0.0456** against a measured max error of **1.8e-6**: ~25,000x too loose, so the route assertion was carrying the test. Measured the real error (5.4e-7 / 1.2e-6 / 1.8e-6 at M=15/16/19) and set a flat **1e-4**. Verified as a falsifier: a 1% oracle perturbation now fails, and previously did not. - **MINOR-2 / NIT-1** — `HALF_WIDEN_MIN_WEIGHT` was an unmeasured guess of `256*1024`, and its doc called `K·N` a byte count when it is an element count. Measuring it showed the guess was not merely untuned but **wrong in the expensive direction**: it excluded exactly the range where the half path is at its worst (7x–275x slower at T=16). Lowered to the measured **256 elements**. - The pre-existing half-dispatch test asserted the half GEMM is *always* selected; one of its shapes (17×130×11) now crosses the gate, so it is gate-aware. Its determinism and tolerance checks are route-independent and still cover both routes. ### Second-round review nits (also fixed) - The gate-aware assertion **duplicated** the threshold values rather than referencing them, so a retune would silently desync test from kernel. It now derives its expectation from the constants. That alone would make the expectation follow *any* retune — which is exactly why the reviewer's "change the constant" injections were not caught: nothing pinned the **values**. The route test now asserts both constants equal the numbers their doc tables were measured at, and points a future editor at the benchmark. Added three shapes sitting exactly on and one step below each threshold. Both previously-missed faults are now caught: | injected fault | before | now | |---|---|---| | `HALF_WIDEN_MIN_M` 16→17 | ❌ missed | ✅ *M threshold moved off its measurement* | | `HALF_WIDEN_MIN_WEIGHT` 256→512 | ❌ missed | ✅ *weight threshold moved off its measurement* | - The two three-digit ratios in the weight table are **noise-dominated in magnitude** — an independent run put them at 47x and 28x, not 275x and 229x. The direction is robust across runs; the magnitude is not, and the doc now says so. The attribution was confirmed in source: `gemm_impl` splits with `par_chunks_mut` whenever `threads > 1` with **no** small-work guard, so the production half path really does fork for trivial problems (this was independently verified by the reviewer, not just asserted). ## Also Corrects the `CpuBackend::Mlas` doc, which claimed MLAS was "opt-in (not auto-selected)". `auto_detect` has returned it by default on x86-64 for some time; read literally, that comment implies this gate never fires. ## Gate fmt clean; clippy clean **both** feature configs (only the pre-existing `needless_return` at `matmul_nbits.rs:639`); `-p onnx-runtime-ep-cpu --lib` debug **and** release, with mlas (1335 pass) and without (1306 pass); real-ORT `plugin_ort_e2e` **51 pass**. Both the `no-mlas` build break and its dead-code warnings were found and fixed by running the second feature config locally. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Second failure of this guard in one day, and this time the hole was the path filter rather than the regex. PR #1132 adds crates/onnx-runtime-ep-cpu/src/dispatch_ledger.rs containing static LOG: OnceLock<Mutex<Vec<Observation>>> = OnceLock::new(); an unbounded process-lifetime Vec that grows one entry per dispatch decision whenever NXRT_CPU_DISPATCH_LEDGER=1. Both jobs reported success against that PR's 3511 added lines because both looked only at src/kernels/** and the file sits one directory up. The existing regex would have matched the line on sight. The question the guard asks -- what multiplies this buffer, and who accounted for it -- was never specific to kernels; only the original grep was. Falsified before landing: scope src/kernels (before) #1132 -> 0 matches (green, wrongly) scope src (after) #1132 -> 1 match, exactly the ledger line merged 11043a0 8d4401d c62798b faf489a fdeaf77 1f675d2 under the wider scope -> 0 new matches beyond the one #1133 already produced OnceLock<Instant> and OnceLock<()>, also added by #1132, are correctly not matched: the patterns require an owned growable payload. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
|
Follow-up on the memory bound (commit c5b3e19, pushed to Measured RSS multiplier (retention, exact atomic counter; 12 MiB accumulator, m=768/k=64/n=4096):
The xN multiplier is observable: retention grows 12 -> 192 MiB (x16) with the per-thread-only bound; the process budget caps it at 120 MiB. ( New bound + arithmetic: Governor wiring (#1056): parking now routes through a declinable Falsification (the test can actually fail): Gates: Speed: parking stays admitted-by-default, so the standalone fast path is unchanged (adds one relaxed CAS per park/take). |
…er-thread (follow-up to #1133) (#1151) Follow-up to #1133 (merged as c62798b with the per-thread bound unchanged). Closes #1147 -- I do not restate its arithmetic table; numbers below are measured against it. ## Measured exposure (this box, 20 logical CPUs) Harness `examples/qlinear_accumulator_rss.rs` parks the accumulator on every worker of a rayon pool, polls `WorkingSet64` **by PID** at 150 ms, and reads the governor's own byte ledger (`qlinear_accumulator_live_bytes()`). 12 MiB per-thread buffer (m=768,k=64,n=4096) to keep the demo tractable; the ledger scales exactly with `per_thread x threads`. | arm | retained (ledger) | idle WS | peak WS | | --- | --- | --- | --- | | T=1, unbounded (pre-fix) | 12 MiB | 17 MiB | 18 MiB | | **T=16, unbounded (pre-fix)** | **192 MiB (x16)** | **199 MiB** | 234 MiB | | T=1, bounded (fix) | 12 MiB | 17 MiB | 19 MiB | | **T=16, bounded (fix)** | **120 MiB (capped)** | **128 MiB** | 229 MiB | **The multiplier is real and observable.** Retention climbs 12 -> 192 MiB (x16) with the per-thread-only bound; **idle/steady-state RSS tracks it 17 -> 199 MiB** (the extra ~182 MiB is 15 further threads x 12 MiB). The fix caps it: idle RSS 199 -> **128 MiB** at T=16. **Honest caveat, stated plainly:** *peak* WS under active decode is ~230 MiB in **both** arms -- it is transient-dominated (every thread allocates its accumulator *during* `execute` regardless of parking), so peak-under-load does not isolate the retention. The exposure the fix removes is the **steady-state** buffer held between calls (idle column), and on a full 32 MiB per-thread buffer this box's ceiling is 640 MiB, not the 12 MiB demo -- exactly #1147's figure. `PeakWorkingSet64` reads 0 after exit (the by-name/post-exit pitfall the profiling skill warns of); the by-PID poll above is the valid figure. ## New bound + arithmetic `min(128 MiB, 32 MiB x threads)` -- flat 128 MiB once >=4 threads park a full buffer, and it does **not** grow with vCPUs: 20-vCPU 640->128, 32-vCPU 1 GiB->128, 128-vCPU 4 GiB->128. Per-thread 32 MiB cap kept, so an oversized single buffer is still released (surviving-case behaviour, intact). The doc comment now states the product with the arithmetic, not a per-buffer figure. ## Governor wiring (#1056) - Accumulator parks through a declinable `GovernedAccumulatorBudget` (sibling of `GovernedWeightCache`: no `Default`, verdict-required, `live_bytes()` reports the sum actually parked across all threads). Declined -> parks nothing, recomputes per call, byte-identical. - `packed_b` (the `OnceLock<Option<(QgemmPackKey, QgemmPackedB)>>` at line 154, ungoverned since ac394fd) is now gated: process gate + mlas-side live-byte accounting (`mlas_sys`) + graph predictor; declined -> dense path, byte-identical. - `load.rs` folds both predictors into `resident_f32_cache_bytes` and admits/declines with the f32 weight-cache verdict. ## Falsifiable multi-thread test `the_parked_accumulator_is_bounded_process_wide_not_per_thread` drives **8 threads** via `pool.broadcast` (a single-instantiation test cannot observe an xN multiplier -- that is how #1100 shipped) and asserts `live <= 128 MiB` **and** `live >= 2x buffer` (non-vacuity). **Falsified on this branch:** reverting the fix (disable the process-cap check in `try_park`) -> test **FAILS at 262,144,000 bytes** (8 x 31.25 MiB) over the 128 MiB cap. Restored -> **passes**. ## CI guard (070c098 / 73c3df0) Both `weight-cache-guard.yml` jobs are **green without weakening the regex**: this PR *bounds* the existing `thread_local!` buffer rather than adding a new one, so the `RefCell<Vec>` line is unchanged (not an added line) and `packed_b` is governed via the mlas ledger without a new `OnceLock<...>` declaration. I did not touch the guard's patterns. (The guard cannot retroactively flag main's existing line -- that gap is why this follow-up is filed against main rather than #1133.) ## Gates (this branch, rebased on origin/main 266a6fe) - `cargo test -p onnx-runtime-ep-cpu --lib` = **1321 passed / 0 failed** - `cargo test -p onnx-runtime-ep-cpu --features mlas --lib` = **1351 passed / 0 failed** - `cargo clippy -p onnx-runtime-ep-cpu --all-targets -- -D warnings` = **clean** - `cargo check -p onnx-genai-engine` = clean ## Speed (no regression) Parking stays **admitted-by-default**, so the standalone fast path is unchanged -- one relaxed atomic CAS added per park/take. `qlinear_phase_report` (release+mlas) still exercises the reuse path; `products-alloc` (the cost parking eliminates) is ~12 ms at m=512. Absolute wall deltas on this contended box are within run-to-run spread (the timing-vs-contention caveat in the profiling skill), so no measurable regression. Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
…indings" (#1931) ## What Both jobs in `weight-cache-guard.yml` currently report **"no findings" when their `git diff` fails**. One line each; no regex, path-filter or label semantics change. ## The defect Both jobs end the match pipeline with `|| true`. That is genuinely required — `grep` exits 1 when nothing matches, and nothing matching is the success case. But `|| true` cannot distinguish *"the matcher found nothing"* from *"the producer never ran"*. `git diff` with an unresolvable `BASE_SHA` exits 128, `set -o pipefail` faithfully reports it, and `|| true` discards it. Verified, not reasoned about: ``` $ BASE_SHA=deadbeefdeadbeef bash <the ungoverned-weight-cache run block> No new ungoverned long-lived buffers. exit=0 ``` A green check, on a guard that saw nothing. ## Why this file specifically This is the **fourth** instance of one shape here, and the first in the plumbing rather than the pattern. The header already records three: - a regex knowing only `OnceLock`/`OnceCell`/`LazyLock`, which matched **zero** of #1133's 459 added lines — the very PR that motivated it; - a path filter scoped to `src/kernels/**`, which missed #1132's `src/dispatch_ledger.rs` one directory up; - the per-instance/per-thread arithmetic error underneath both. Every one surfaced as **zero findings, green check**. Each time zero meant *"I could not see"*, not *"there is nothing there"*. A guard whose silence is indistinguishable from its success has no negative control — which is precisely what this file exists to warn about, applied to itself. ## The fix, and why it is safe Resolve the diff on its own and let its status stand. The property that makes this safe: ``` no differences -> 0 differences -> 0 bad object -> 128 ``` `git diff` is non-zero **only** on error, so "nothing matched" and "nothing ran" separate cleanly. The guard flags exactly the lines it flagged before; the only new failure mode is the one that was previously invisible. I deliberately did **not** add an "the diff must be non-empty" control, though the `paths:` filter would seem to justify it. `pull_request.base.sha` is the base branch tip, so `git diff BASE HEAD` is two-dot while GitHub's changed-files filter is three-dot — they diverge when the base already contains the head's content, and that control would fire on a legitimate PR. Noting it as considered-and-rejected rather than missed. ## Verification Both jobs, four regimes, by extracting the **actual `run:` blocks from the YAML** rather than retyping an approximation: | regime | expected | result | |---|---|---| | (a) broken producer (bogus `BASE_SHA`) | fail | **1** — was `0`, the defect | | (b) healthy diff, nothing matching | pass | `0`, *"No new … buffers."* | | (c) real match (`OnceLock<Vec<u8>>` / `thread_local RefCell<Vec<u8>>`) | fail | `1`, message intact | | (d) correct review label | pass | `0`, bypass intact | On (d): my first run reported a false regression on the second job because I passed `weight-cache-reviewed` — the *first* job's label. The second uses `per-thread-bound-reviewed`. The test was wrong, not the code; checked before reporting. ## Provenance Found while checking a claim from @gaff-1 that a stated failure mechanism (`QEMU_LD_PREFIX`, exit 0 vs 255) was wrong and that *"anyone who writes 'check for the error string, because the code can't be trusted' ships a weaker check"*. That specific concern does not apply to this repo — qemu appears in no workflow here. But the underlying question does, so I swept every `run:` step for pipelines whose producer's failure could be swallowed. Five candidates; `diff-guard.yml` is safe (`set -euo pipefail`, no `|| true`); these two were not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
What this is
QLinearMatMulwas 2.27x slower than ORT atK=N=2048, M=128with a 16-thread pool, but 1.01x — parity — on one thread (plugin-level, interleaved, both sides pinned; 8.281 ms ours vs 8.183 ms ORT). A deficit that only appears when threads are added is not arithmetic, so this started as a decomposition rather than a tuning exercise.Hardware for every number below: 32-vCPU / 16-core EPYC 9V74 (Zen4, AVX-512),
--release --features mlas, both thread pools pinned to the same budget (ONNX_GENAI_MLAS_THREADPOOL_THREADS+RAYON_NUM_THREADSfor us, intra-op for ORT). Ratios are written explicitly as ours/ORT, so greater than 1.0 means we are slower.Where the time was going
The in-tree
qlinear_phase_reportwas extended to time the fused MLAS primitive on its own, sowhole - fusedisolates everything our wrapper does around it. AtK=N=2048, M=128:MLAS scales 7.2x across that range. Our wrapper gets 4x more expensive as threads are added, which is the entire deficit.
The thread pool was ruled out first rather than assumed innocent. An empty
parallel_fordispatch measures 0.03 us at T=1, 2.6 us at T=8 and 4.7 us at T=16, and a 100 us tile costs 103.5 us at T=16 — a few microseconds, not a few hundred. It is also insensitive to a 2 ms idle gap, so parking and wake latency are not involved either.Bisecting the wrapper by making a direct MLAS call allocate the same buffers the kernel allocates found the mechanism:
Acopy (256 KiB)i32accumulator (1 MiB)Per-call buffer churn is cheap on one thread and expensive on sixteen. A fresh multi-page mapping has to be page-faulted in, and MLAS first-touches the accumulator from every worker, so the fault count follows the pool size. Freeing that mapping then forces a TLB shootdown IPI to every core the process is running on. Neither cost exists at T=1, which is exactly why the kernel looked healthy there.
The change
Three buffers, three fixes, none of which changes an output byte:
Ais borrowed when the view is already dense (dense_bytesreturns aCow), instead of always being copied throughto_dense_bytes— which allocates a zeroedVecand then overwrites all of it, so the copy path was also paying for a pointless memset. A strided or non-host-accessible input still copies exactly as before. The one route that rewritesA(the sign flip on signed-A x unsigned-B) goes throughCow::to_mut, which copies a borrowed operand before touching it, so the caller's input is never written through.requantize_rowsnow takesdestination: &mut [u8](exact length, with a length-mismatch guard) rather than appending into a&mut Vec<u8>. A strided or non-host output still stages into aVecand scatters, via a smallOutputSink::{Direct, Staged}enum whoseregion(base, len)hands out one batch's slice.i32accumulator is parked on a thread-local between calls, bounded at 32 MiB so an outsized shape is released rather than retained forever. It is thread-local rather than shared becauseexecutetakes&self; a lock would serialise precisely the concurrent calls this is meant to speed up. An early?return simply drops the buffer, which costs the next call one allocation and cannot affect a result.This follows an existing in-repo convention rather than inventing one:
matmul_nbits.rshas had the samedirect_result/owned_result+contiguous_host_sliceshape for a while.QLinearMatMulwas the outlier.Result
Kernel level (in-process, our side only — there is no ORT number at this level, so no ratio is quoted here),
u8,K=N=2048, M=128:Wrapper cost at 16 threads: 576 us -> 54 us.
Plugin level vs real ORT — this is the apples-to-apples comparison, and the only place a ours/ORT ratio is defensible. Three cdylibs (this branch /
main/ plain ORT) measured in 11 interleaved rounds of 41 iterations after 3 warmups, T=16 on both sides, medians of the per-round p50s:K=N=2048u8, M=128 (5 rounds)u8, M=1 (11 rounds)u8, M=128 at T=1 (3 rounds)So the prefill shape goes from 2.27x to 1.43x ours/ORT at 16 threads, and decode is unchanged to three digits — its buffers are a few KiB, so buffer churn was never its problem.
The T=1 row is the control that makes the whole diagnosis falsifiable: at one thread we were already at parity with ORT (1.01x) on the exact shape where we were 2.27x slower at sixteen. A kernel that is level at T=1 and 2.27x behind at T=16 is not losing on arithmetic, and the fix had to be — and was — in what scales with the thread count. That row is also tight enough to trust: p50-to-p90 spread under 1.2% on all three sides, versus 3-4x at T=16.
This is a real gain and it is still not a win. We remain ~1.4x slower than ORT at M=128 and ~1.6x slower at M=1. This PR does not claim otherwise, no dispatch decision rests on it, and both shapes stay on the open list.
Honest note on the tail
Per-round p90s (medians across rounds,
u8M=128): ours before 3.88 ms, ours after 3.29 ms, ORT 0.83 ms. So p90 improves but our dispersion stays much wider than ORT's, whose p90 sits almost on its p50. This box is shared with other agents' builds and each side's rounds run in separate processes at different moments, so I am not claiming the tail gap is a property of the kernel — but I am not hiding it either. It is unexplained and stays on the open list.The ORT p50 itself moved between 1.11 ms (rounds 1-2) and 0.82 ms (rounds 3-5) on the same box, which is why every ratio above is taken from interleaved rounds and never from numbers measured in different sittings.
What is not improved
bench_qlinear_u8_m1(decode) stays at ~1.63x ours/ORT. Unchanged, still open.bench_qlinear_i8_m1was measured in an earlier session at ~0.065 ms ours vs 0.54-0.97 ms ORT — 8-15x in our favour. That number is not re-measured in this PR's runs and nothing here depends on it; it is repeated only to say this change does not touch that path.Behaviour change worth disclosing
On the new direct path, an error raised part-way through a multi-batch call can leave the output tensor partially written, where previously the output was untouched until the whole result had been staged. This is within ORT's kernel contract (an output is undefined when a kernel returns non-OK) and it is what
matmul_nbits.rsalready does, but it is a real difference and is called out here rather than buried.Tests
Three new falsifiers, each of which fails if the corresponding optimisation is wrong rather than merely slow. Each was checked by deliberately breaking the thing it guards:
a_contiguous_output_is_written_in_place_and_a_strided_one_is_staged— asserts the route actually taken via thread-local counters. Forcing the direct path off makes it fail, so a silent regression to always-staging is caught.the_sign_flip_route_never_writes_through_to_the_callers_input— asserts the caller'sAis byte-identical after a flip-route call, and that the call really borrowedA. Without the second assertion the test would pass vacuously the momentdense_bytesstopped borrowing; forcingdense_bytesto always copy now makes it fail.a_batched_call_lands_every_batch_at_its_own_offset_in_place— catches an off-by-one in thebatch * m * nregion base. Forcing the base to0makes it fail in both feature configurations.The route counters are
thread_local!Cells, not global atomics; the harness runs tests in parallel and a global counter made the assertions flaky.Review
Independently reviewed by an Opus reviewer, which ran the build, both test configurations, the real-ORT suite, and four falsification experiments of its own. Verdict APPROVE, no MAJOR findings. Both MINOR findings are fixed in the second commit:
flip_aspecial case (taking an owned copy before the flip) was redundant —Cow::to_mutalready copies a borrowed operand — and it re-introduced the veryto_dense_byteszeroed allocation this PR removes. It is gone, and the test that guards the flip now also asserts the borrow happened.Verification run
cargo fmt --all --check— cleancargo clippy --all-targets -p onnx-runtime-ep-cpu, with and without--features mlas— no new warnings (the pre-existingneedless_returnatmatmul_nbits.rs:639is untouched here and is being fixed separately)cargo test -p onnx-runtime-ep-cpu --lib— debug and release, with--features mlas(1335 pass) and without (1307 pass), 0 failuresNXRT_REQUIRE_ORT_TESTS=1 cargo test -p onnx-runtime-ep-cpu-plugin --test plugin_ort_e2e --features mlas --release— 43 pass, 0 fail, against real ONNX Runtime