Repository navigation
perf(cpu-ep): name the prefill/MLAS Rayon workers - #1398
Merged
Merged
Conversation
Rayon does not name `build_global` workers, and an unnamed thread's `comm` defaults to the *process* name. So the pool `bound_process_to_decode_budget` builds shows up in `ps`, `top` and `/proc/<pid>/task` as N extra copies of the host binary rather than as a pool. That made this the largest unattributed block of threads in any budgeted process, and it is not a small block: an explicit budget of N costs N global Rayon workers *in addition to* raising the decode pool to N, so a budget of 16 takes the process from 22 threads to 48 and a budget of 32 takes it to 80. Half of the increase was anonymous, which is why "roughly twice the budget, unattributed" stood open as a question for as long as it did. A flat census does not merely fail to explain those threads, it actively misattributes them to the caller. Naming them settles it, and the first census taken afterwards answers the question the attribution was blocking: in a decode-only workload all 16 prefill workers report 0.0 ms of CPU. The pool is built eagerly, is never used, and costs N threads for the process lifetime. Deliberately short names: Linux truncates `comm` to 15 bytes, so this crate's longer `onnx-genai-` convention collapses to `onnx-genai-deco` and loses the worker index. Tested for the property that matters rather than for the literal string. No behaviour change — thread names only. Making the pool *lazy* is the real fix and is not attempted here: it would require every global-Rayon entry point in the default build to route through a guard first, and missing one silently un-bounds the user's budget. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
enabled auto-merge (squash)
August 19, 2026 04:35
🔴 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
|
justinchuby
added a commit
that referenced
this pull request
Aug 19, 2026
…e it (#1407) ## The UB `nested_dispatch_slot_pressure` (which I added in #1377) reconstructs a `&mut [u64]` over the **entire** row in every task and only then narrows: ```rust let row = unsafe { std::slice::from_raw_parts_mut(base as *mut u64, NEST_INNER) }; for slot in &mut row[start..end] { *slot += 1; } ``` The *stores* are disjoint. The *retags* are not. Under Stacked Borrows the violation is the retag: each task pushes a `Unique` covering all of `NEST_INNER`, popping the previous task's tag. Narrowing before the retag fixes it, and is the shape `parallel_output_rows_repeated` already uses in production. **Diagnosis and fix are Pris's, from #1385.** This PR carries them because that one is still a draft with auto-merge off and the UB is on `main` today. If #1385 goes ready first, close this and I will rebase the Miri half onto it — the two halves are independent. ## Why it survived, which is the part worth keeping CI's Miri lane runs `-p onnx-runtime-ep-cpu --lib task_runtime::`. **Integration tests under `tests/` are never Miri-checked at all.** Fixing this one instance would have left that gap open for the next one, so this also puts the shape under the lane as a lib test. That was not sufficient either, and the first attempt is the useful part: **the new test passed under Miri with the bad retag still in it.** Miri reports `available_parallelism() == 1`, so `resolve_width` builds a one-lane pool, every fan-out returns `Backend::Serial`, and no two tasks ever run against each other. Probe under Miri: ``` pool_width=1 backend=Serial tasks=1 ``` The lane has been type-checking the unsafe blocks in this module without exercising the concurrency they exist for. The workflow comment claims it "runs real threads under Stacked Borrows" — it runs one. So I would have shipped a canary that passes for a reason unrelated to its claim, which is precisely the class Pris named in #1385. Two fixes: 1. **The test asserts it actually fanned out** (`Backend::Native`), so it fails loudly if it ever degenerates to one task instead of passing silently. 2. **A dedicated lane step with `-Zmiri-num-cpus=4`**, scoped to that single test rather than all of `task_runtime::` — multi-CPU Miri multiplies the runtime of what is already the slowest step in the lane. The targeted step costs **18s**. ## Verified in both directions | retag shape | `-Zmiri-num-cpus=4` | result | | --- | --- | --- | | whole row (**#1377 as merged**) | yes | `error: Undefined Behavior: Data race detected between (1) retag write on thread task_runtime::t and (2) retag write of type [u64] on thread nxrt-task-0` | | whole row | **no** (lane default) | **passes** | | own range (**this PR**) | yes | passes | The middle row is the finding: without the flag the falsifier does not falsify. ## Scope Test-only plus one workflow step. No production change — `for_each_chunk_mut` was always correct. 30/30 `task_runtime::` lib tests pass natively and under Miri; the repaired benchmark still runs (`1 of 30 dispatches declined`); fmt and clippy clean. Includes the one-line rustfmt repair of `onnx-runtime-ep-cuda/src/runtime.rs` that `main` is currently failing on (same as #1393/#1395/#1398 — whichever lands first makes the rest a no-op). 🤖 Generated with [GitHub Copilot CLI](https://github.com/features/copilot/cli) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1398 +/- ##
===========================================
- Coverage 82.10% 80.14% -1.97%
===========================================
Files 12 376 +364
Lines 5471 164303 +158832
Branches 5471 164303 +158832
===========================================
+ Hits 4492 131675 +127183
- Misses 780 27801 +27021
- Partials 199 4827 +4628
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Rayon does not name the workers
build_globalcreates, and an unnamed thread'scommdefaults to the process name. So the pool thatbound_process_to_decode_budgetbuilds appears inps,topand/proc/<pid>/task/*/commas N extra copies of the host binary.This is not a cosmetic gap. It was the largest unattributed block of threads in any budgeted process, and it is what kept "the process has roughly twice the budget in threads and we don't know whose they are" open as a question across several phases of the CPU scheduler campaign.
A flat thread census does not merely fail to explain these threads — it misattributes them to the caller, which is worse, because the census looks complete.
What the naming immediately revealed
Census on
gemm_nbits_qwen3_0p6b_qkv_t8,ONNX_GENAI_CPU_DECODE_THREADS=16, decode-only:Before this change the sixteen
nxgn-prefill-*rows read asbench_decode_ga— indistinguishable from the benchmark's own main thread.All sixteen report 0.0 ms of CPU. The pool is built eagerly at EP init, is never used by a decode-only workload, and holds N threads for the process lifetime.
That also quantifies the budget's real cost, which is larger than it looks:
Setting an explicit budget of N does not size one pool to N — it sizes the decode pool to N and builds a second, separate N-wide pool. Half of that was previously anonymous.
A useful negative result falls out of the same census: the ORT session contributes exactly zero threads. Whatever else is going on, our EP is not double-provisioning against ORT.
Why the names are short
Linux stores
commin 15 bytes. This crate's existingonnx-genai--prefixed convention (decode_numa.rs,decode_spmd.rs) already exceeds that and collapses toonnx-genai-deco/onnx-genai-spmd, losing the worker index — visible in the census above, where all 16 decode workers fold into one row.nxgn-prefill-15is exactly 15 bytes and survives intact, matching the short form the task runtime already uses (nxrt-task-N).The test asserts the property — fits in
comm, keeps its prefix, keeps its index, stays distinct — rather than the literal string, since the string is not the thing that has to hold.Scope
Thread names only. No behaviour change, no sizing change, no scheduling change. 1448/1448
onnx-runtime-ep-cpulib tests pass; fmt and clippy clean.Making the pool lazy is the real fix and is deliberately not attempted here. It would require every global-Rayon entry point in the default build to route through a guard before first use, and missing one silently un-bounds the user's explicit budget — a correctness regression traded for a thread-count saving. Named as a measured cost with a known shape (§41) so the decision is on the record rather than rediscovered.
Includes the one-line rustfmt repair of
onnx-runtime-ep-cuda/src/runtime.rsthat main is currently failing on (same as #1393/#1395 — whichever lands first makes the others a no-op). Without it this branch cannot be green.Follows #1395, which built the census that made this diagnosable.
🤖 Generated with GitHub Copilot CLI