Repository navigation
perf(cpu-ep): inline intermediate dimension storage (-2 allocations/node) - #1422
Closed
justinchuby wants to merge 3 commits into
Closed
justinchuby wants to merge 3 commits into
justinchuby wants to merge 3 commits into
Conversation
Every routed node built two fresh vectors for each buffer-backed output -- a shape clone and a strides vector -- and dropped them when the intermediate died. The bytes were already pooled; only the metadata was thrown away. Pool the metadata alongside it. A dead intermediate now retires through one entry point that reclaims all three, so a caller cannot reclaim the bytes and silently drop the rest, which is exactly what every death site did before. Reused pairs come back empty rather than resized. That is the property that makes stale dimensions impossible rather than merely unlikely: there is no element to read before the caller writes one, and a shorter shape cannot inherit a longer one's trailing strides. Measured on the 8-element depth-100 chain, latest main: 8.17 -> 6.19 allocations per node, with the unattributed bucket falling 4.09 -> 2.11 -- the two allocations this removes were the ones sitting in the routed loop between the named phases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The pool in the previous commit removes two allocations per node, which the counters confirm (8.17 -> 6.19). It does not make anything faster: paired against its own parent it measured 1.440 vs 1.424, inside a noise floor of about 2%. Priced the mechanism instead of guessing. Injecting two extra allocations per node into the same loop moved the ratio 1.440 -> 1.548, and eight moved it to 1.925 -- linear, and far outside noise. So the allocations the pool removes should have been worth roughly -0.10 ratio, and none of it arrived. The bookkeeping was eating the saving: two thread-local RefCell round trips per node in place of two mallocs. Inline storage gets the same allocation count with no bookkeeping at all. Rank is unbounded in the ONNX spec, so this is not a fixed-capacity type -- it spills to the heap past four dimensions and stays correct there. Four covers NCHW and everything under it, and keeps the struct small enough that moving it once per node does not cost back what the allocation saved. It is also simply less machinery: no thread-local, no reset semantics, no window in which a reused buffer holds a previous tenant's dimensions. Stale metadata stops being a property that has to be tested for and becomes one that cannot be expressed. Measured: 6.19 allocations per node, identical to the pool. Timing unchanged within noise, which is the honest result for both mechanisms -- see the issue ledger for what that says about allocation count as a lever. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review found the stride test was comparing the function to itself: the reference it checked against is a thin wrapper over the function under test, so it proved determinism and nothing else. A shared row-major/column-major mistake would have passed it. Anchored to the IR crate's implementation instead, and confirmed by mutation that a column-major recurrence now fails it. Also pins the invariant the inline storage made load-bearing. An input view is transmuted to 'static, and shape/stride data now lives inside the IntermediateBuf rather than in a separately allocated Vec, so growing the intermediates vector while those views are live would be a use-after-move rather than the benign edit it used to be. The vector is sized once and only ever index-assigned; that is now written down where someone would go to change it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Independent adversarial review (Opus 4.8, read-only, seven named attack surfaces): APPROVE WITH NITS — no correctness defect. It specifically cleared the highest-risk aspect, that shape/stride data now moves with Both nits were real and are fixed in 7da61a7:
|
🔴 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
Part of #1077. ## What `is_dense` collected the non-trivial `(stride, size)` extents into a `Vec` on every call. It runs once per operand per node on the CPU plugin dispatch path, so a rank-1 tensor of eight floats paid a `malloc`/`free` pair to have four numbers inspected. Rank <= 8 covers every tensor ONNX produces in practice, so those extents now live on the stack. Higher ranks keep the heap path rather than impose a limit the signature does not have. **Both paths hand the same slice to the same routine** (`dense_extents`), so there is exactly one implementation of the predicate and the two cannot drift. ## Why this function `perf record` sampling of the 100-node 8-element `Relu` chain, our EP side alone (`NXRT_MM_BENCH_SIDE=ours`), 120k iterations so the steady state dominates rather than model construction: | % self time | symbol | |---|---| | 23.55 | `compute_execute::{closure#0}` | | 23.16 | `malloc` / `_int_free` / `cfree` | | **5.25** | **`is_dense` + the `Vec` it built** | | 2.97 | `WorkspacePlanCache::lookup` | | 2.61 | `__memcmp_avx2_movbe` | | ~3 | the elementwise arithmetic the graph exists to do | `is_dense` cost more than the kernel work it was gating. ## Measurement Paired A/B, same host and session, three interleaved repetitions, 100-node 8-element `Relu` chain, one thread on both sides, CPU fallback disabled, `assigned == executed` enforced by the suite: | | ours p50 | ratio p50 | |---|---|---| | control | 0.0324 / 0.0330 / 0.0329 ms | 1.367 / 1.375 / 1.368 | | **with fix** | **0.0307 / 0.0307 / 0.0309 ms** | **1.261 / 1.299 / 1.316** | **-6.4%** on our side. The two sets are non-overlapping across every repetition and the effect is well clear of this host's ~4% wall-clock noise floor. Per-node slope (depth 100 minus depth 1, divided by 99) moves from 0.307 to 0.285 us/node against ORT's 0.223. ## Correctness `is_dense` had **no tests in its own crate** -- the only coverage was seven assertions in a consumer crate. The new differential test runs the pre-optimisation implementation verbatim as an oracle over ranks 0..=10, straddling the inline bound in both directions, across contiguous, permuted, zero-sized, negative-strided and length-mismatched cases. It also asserts the corpus actually reached the dense arm, so it cannot silently degenerate into "everything was rejected and both agreed". Mutation-verified -- the test fails for each of: | mutation | result | |---|---| | inline array not truncated to the filled length | **FAILS** | | `d > 1` filter dropped on the inline path | **FAILS** | | stride not taken as absolute | **FAILS** | | `INLINE_RANK` lowered to 2 | passes -- correctly, since the array size and the branch bound derive from the same constant, so this is a performance-only change and not a defect | `cargo test -p onnx-runtime-ir` 69 passed; `cargo test -p onnx-runtime-ep-cpu --lib` 1447 passed; `plugin_ort_e2e` 55 passed including `every_assigned_node_is_also_executed_by_this_ep`; clippy clean. ## Also in this PR: `is_contiguous` (a null result, kept as a simplification) `is_contiguous` had the identical shape of defect -- it built the entire row-major stride vector on the heap and compared slices against it. It now accumulates the expected stride while walking backwards, with no allocation and a short-circuit on the first mismatch. **This is not claimed as a performance win.** On the dispatch benchmark it is a null result, because our elementwise fast path gates on `is_dense` and reaches `is_contiguous` barely at all -- `compute_contiguous_strides` is 0.04% of sampled self time. Paired A/B, three repetitions: | | ours p50, 100 nodes | |---|---| | `is_dense` fix only | 0.0307 / 0.0307 / 0.0309 ms | | plus `is_contiguous` | 0.0310 / 0.0311 / 0.0306 ms | Indistinguishable. It is kept because it *removes* an allocation and a line of indirection from a predicate used across the dtype-conversion, elementwise and host-ABI paths this benchmark does not exercise -- negative complexity, unlike #1422, which was closed unmerged for being a null result that *added* representation complexity. Short-circuiting also means the walk cannot overflow where the materialising version could, since that one built every stride before comparing any of them. Differential test against the previous implementation over ranks 0..=6, with exactly-contiguous, one-axis-disturbed and arbitrary strides plus ragged lengths, asserting the corpus actually reached the accepting arm. Mutation-verified against three corruptions: product advancing on the wrong index, walking forwards, and dropping the length check. ## Review follow-ups (applied) Independent adversarial review returned APPROVE WITH NITS, no correctness defects, and confirmed by diff that the differential oracle is a faithful verbatim copy of the pre-change implementation. Two nits were real and are fixed: - The `checked_dense > 100` guard was satisfied four times over by rank-0 shapes, which return `true` from the empty-extents early exit without reaching the sort-and-product logic the guard names. It now counts only cases with a dimension above 1; the corpus supplies 836 of them. - Added a deterministic test for a rank of exactly `INLINE_RANK` with every dimension non-trivial -- the single input where the filled length equals the whole array, so the truncation mutation cannot be caught. It was reached once by chance in the random corpus and should not depend on the seed. It also covers dense-but-not-row-major. Mutation testing additionally showed the `pairs[0].0 != 1` early rejection is **redundant** -- the loop's first iteration makes the same comparison. It is preserved verbatim from the original and now labelled so a reader does not take it for load-bearing. ## Stacking Based on `squad/resch-plan-cache` (#1430). Auto-merge deliberately **off** while the base is a non-`main` branch. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 19, 2026
…de (#1472) Removes one heap allocation and one `Drain` drop per **routed node, per `Run`**. ## What The routed node loop collected `TensorMut`s for every ORT-bound output into `ort_out_views`, then immediately `drain(..)`ed it to feed the map that builds `all_output_views`. The intermediate vector was never read as a vector — every element went straight through to the next collect. `main` already removed exactly this shape of code on the **single-node** path. This does the same on the routed path, which is where it is paid *per node* rather than once per `Run`. ## Why it is safe `ort_outputs.push(..)` and `slot_kinds.push(RoutedSlotKind::Ort)` happen together in the one `NodeOutputSink::Ort` arm, so there is exactly one `Ort` slot kind per ORT output. The lazy iterator therefore yields precisely the same views, in the same order, and is exhausted identically. `Buffer` and `Absent` slots are untouched. `TensorMut` construction has no side effects, so not building views for outputs that are never consumed is unobservable — and by the lockstep argument there are none. The mutable borrow of `ort_outputs` now *starts later* (at the block rather than before the `new_bufs` loop) and ends at the same place, so this is strictly less restrictive to the borrow checker, not more. ## Measurement — this is a null result, and is labelled as one Paired interleaved A/B, `grid_relu_100_tiny` (100-node 8-element `Relu` chain), 800 iters, 100 warmup, 4 threads, pinned to cores 9/11/13/15 verified ≥99% idle, 3 reps: | | ratio vs ORT CPU | |---|---| | before | 1.193 / 1.190 / 1.180 | | after | 1.207 / 1.192 / 1.056\* | \* third rep contaminated (both sides inflated ~1.75×, host interference). Medians: **1.193 → 1.192**. Indistinguishable against a ~4% wall-clock noise floor. `perf record` self time attributes ~2% to the collect and its drain (`Vec<TensorMut>` collect 1.22%, `Drain<TensorMut>::drop` 0.79%), which this grid cannot resolve. This is the **third** independent confirmation of the finding recorded in #1077 §5e: removing allocations one or two at a time does not move this number. **So it is not offered as a performance fix.** It is offered as a simplification: it deletes a binding and a drain, removes an allocation the profile does show, and makes the routed path agree with the single-node path. That is what separates it from #1422, which was closed unmerged because it bought an equally null result at the cost of *added* representation complexity. Recorded in #1077 as a null result and **not counted toward the parity target**. ## Validation - `onnx-runtime-ep-plugin` lib: 261 passed - `plugin_ort_e2e`: 55 passed, including `every_assigned_node_is_also_executed_by_this_ep` (assigned == executed holds) - clippy `--all-targets` clean on the touched crate - No ORT CPU fallback, no runtime/default MLAS Refs #1077. Stacked on #1433 — auto-merge deliberately **off** while the base is an unprotected branch. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 19, 2026
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.
Closed: controlled null result
This change is not being merged. It is recorded here because the measurement is worth more than the code.
What it did
Removed the two heap allocations each routed node made for its output metadata — a shape clone and a strides vector — by storing both inline in
IntermediateBuf. Verified with the deterministic counters from #1409 on the 8-element depth-100 chain:The
unattributedbucket fell 4.09 → 2.11: the removed allocations were exactly the ones in the routed loop between the named phases.The controlled null result
No measurable performance benefit. Paired against its own parent on the same base, latest
main:The distributions overlap completely. The noise floor across repeated runs of identical code was ~4%, wider than any effect present.
This was built twice — first as a thread-local pool, then as inline storage with no bookkeeping at all — precisely to rule out the mechanism being at fault. Both hit 6.19/node; neither moved the ratio.
Why the prediction was wrong
The mechanism had been priced by injecting known allocations into the same node loop:
Linear, ~13–15 ns each, far outside noise — which predicted about −0.10 ratio for the two removed. None of it arrived.
The injected allocations are not equivalent to the real ones. The real shape/stride allocations are the same size on every iteration of a tight loop and land in glibc's tcache; the injected ones are held live behind a
black_boxthat inhibits exactly the reuse being measured. Repeated same-size allocations on this path are close to free. The counters were right; the inference drawn from them was wrong.Disposition
Closed unmerged. It removes real allocations, but it buys no measured time and adds a second representation for tensor dimensions, and that complexity is not worth carrying on a null result. Nothing is stacked on it, so the rest of the dispatch stack is unaffected.
The finding is recorded in #1077 §5e and in its "negative results — do not retry" list, along with the direct consequence: converting
infer_shapesto caller-supplied buffers is retired as a performance action. It is ~720 lines across 13 match arms to remove 2 allocations per node now measured to be worth approximately nothing.Review record retained in the comment below: independent adversarial review found no correctness defect, and its two nits were fixed — one of which was a genuine defect in my own test (a self-referential oracle that a row-major/column-major bug would have passed).
Part of #1077.