Repository navigation
Attribute every dispatch allocation to a phase across the ORT ABI - #1409
Merged
Merged
Conversation
Issue #1077 asks why our per-Run dispatch overhead is higher than ORT's. Answering that repeatedly has meant hand-editing `Instant` probes into the hot path, measuring, and tearing them out — which is slow, unreviewable, and leaves nothing behind that would catch a regression. This adds `dispatch_probe`, a counter and phase-timing module gated behind a non-default Cargo feature, and wires it through the CPU dispatch path: ORT callback entry, input metadata queries, tensor binding, output allocation, dispatch lookup, kernel invocation, and status/ABI crossing. It also tallies FFI round trips, heap allocations and `OrtStatus` creations, which are the three things actually worth removing. The counters are kept twice. The thread-local copy is precise, and is what makes exact-count assertions possible without a neighbouring test bleeding into the measurement. The global mirror exists because ORT picks the thread that runs `Compute`, and the e2e harness reading these numbers back over the C ABI is not necessarily on it; a thread-local-only probe would report zero there and read as "we made no FFI calls" rather than "you are on the wrong thread". Without the feature the whole thing is a `Drop`-less zero-sized guard and a set of `#[inline(always)]` empty functions, so production builds pay nothing. `probe_is_compiled_out_in_production` asserts that directly rather than trusting it. Timing is gated a second time, on `ONNX_GENAI_PROFILE_DISPATCH=1`, because a test asserting call counts does not want two `Instant::now()` calls added to every phase it measures. The counts themselves are pinned by a hand-built `OrtApi` in `kernel_ctx`, which answers just enough of the C API to run `read_inputs` without a live ONNX Runtime. Reading one input costs eight round trips and five allocations today; three inputs cost 1 + 3x7 and 1 + 3x4, so the model is linear rather than a single data point. Any change that adds a round trip or an allocation to the per-Run path now has to come here and edit a number in a diff a reviewer can see. Two of the five allocations are already visibly waste: the `format!("input {i}")` label is built eagerly on every input even when validation succeeds, and `compute_execute` formats a staging log line unconditionally before the function that early-returns when logging is off. Those are removed in a follow-up, which this change exists to measure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`probe_is_compiled_out_in_production` compared `snapshot()` after some counting against `snapshot()` before it. Both are calls to the same constant function in a disabled build, so the assertion held no matter what the probe did — verified by mutation: a version whose disabled `count_n` recorded into a static and whose `snapshot` returned a non-zero `Counters` passed the test unchanged. Compare against `Counters::default()` instead, which is an absolute claim rather than a self-consistency one, and fails against that mutation. Also assert `!needs_drop::<PhaseGuard>()`, so a `Drop` impl added to the production guard — the one way it could regain a cost while staying zero-sized — is caught. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… one Review of #1387 found the module's central claim was false. It documented `OrtFfiCall` as "how many ORT FFI calls a Run made" and called it the headline number for #1077, but only `read_inputs` and `allocate_output` were instrumented. On the happy path of a single-node Run, `device_mem_info`, `ort_input_mem_info`, `mem_info_is_device`, `alloc_scratch`, `host_pool::install` and `CreateStatus` all reach into ORT and none of them were counted. A probe that under-counts while presenting itself as exact is worse than no probe: it invites the reader to conclude the dispatch path is cheaper than it is, which is precisely the conclusion #1077 exists to prevent. Every leaf call through an `OrtApi` function pointer in this crate is now counted. Rather than assert that in a comment, `ffi_coverage` enforces it: it scans the source of each file that reaches into `OrtApi`, extracts the API members it names — ORT's C API is CamelCase and our own fields are snake_case, so `.CamelCase` isolates them cleanly — and fails if that set or the instrumentation count moves. Adding an uninstrumented call now fails with the member's name in the message. Verified by mutation: adding a stray `GetTensorShapeElementCount` reference failed with exactly that. A second test pins that the scan finds real members, so the guard cannot degrade into a heuristic that matches nothing and passes forever. `DispatchAlloc` gets the opposite treatment, because hand-placed counts genuinely cannot be exhaustive: `Vec::new()` does not allocate, `Vec::with_capacity(0)` does not allocate, and whether a `collect` allocates once or twice is a property of `size_hint` rather than of anything visible at the call site. It is now documented as a lower bound that is exact only for `read_inputs`, and callers who need the true whole-Run figure get `CountingAllocator`, a `GlobalAlloc` wrapper they can install in a test or benchmark. This crate deliberately does not install it — a library that defines a `#[global_allocator]` takes that choice away from every dependent. Also from review: - A node with no inputs was charged one allocation for a `Vec::with_capacity(0)` that never happens. Now conditional, and pinned by a test. - The phase timings are documented as what they are: non-exhaustive on the success path, and nesting on the error path, where `StatusCrossing` opens inside whichever phase was live because a guard closes at scope exit rather than at the early return. `DispatchLookup` is entered twice per node and reports the total. Summing them does not yield Run time in either direction, and a reader who assumed it did would be misled. - `probe_is_compiled_out_in_production` cannot prove `count` has no side effect — a disabled build has no storage to observe one — so it no longer implies it does. What it can prove, it does: the guard is a ZST and does not need dropping. Validation: clippy clean and `--lib` green in both feature configurations (253 without, 260 with), and 55/55 `plugin_ort_e2e` against real ORT. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stacked on #1387 (base `squad/resch-dispatch-probe`). The instrumentation there exists to make changes like this one provable; this is the first of them. ## Why Two heap allocations sat on the per-`Run` dispatch path purely to serve diagnostics that are switched off. **1. `staging_log(&format!(…))`.** `staging_log` returns immediately when the transfer trace is disabled — but the argument is evaluated first. Every one of these builds a `String` and formats each interpolated value on the way to a function that drops it on the floor. One of them is in `compute_execute`, before any real work happens: ```rust staging_log(&format!( "[plugin/staging #982] compute_execute enter: entries={} routing={} device_staging={}", exported.entries.len(), exported.routing.is_some(), exported.device_staging.is_some() )); ``` So every dispatch in every production run has been paying for a debug line nobody asked for. **2. `format!("input {i}")` in `read_inputs`.** Built for every input on every `Run`, to label an error that is almost never raised. ## What A `staging_log!` macro that checks `staging_trace_enabled()` *before* formatting, so a disabled trace costs one relaxed `OnceLock` load and nothing else. Thirteen call sites convert. `validate_dims` now takes `impl Display` instead of `&str`, so the caller passes `format_args!("input {i}")` — which borrows its arguments and formats only if a message is actually produced. Same treatment for `transfer.rs`'s `CopyTensors[{i}]`. ## Evidence The pinned counts in `dispatch_cost` (added by #1387) move: | | before | after | |---|---|---| | allocations, 1 input | 5 | **4** | | allocations, 3 inputs | 1 + 3×4 | **1 + 3×3** | That is the instrumentation earning its keep. The improvement is a number changing in a test — reproducible on any machine, by anyone, at any time — rather than a claim resting on a benchmark. That matters here in particular: the box is currently shared with other work and sitting at ~74% idle against my ≥93% gate, so a wall-clock A/B taken right now would be noise. I will post interleaved p50/p90 once the machine quiesces, but the correctness of the change does not depend on it. ## Correctness - **The trace still works.** Ran the e2e suite with `ONNX_GENAI_PLUGIN_TRANSFER_TRACE=1` and confirmed the `compute_execute enter` line still appears. A macro that silently disabled the trace would otherwise be indistinguishable from one that made it cheap. - **No side-effecting expression moved inside a now-conditional macro.** I checked every converted site; the only mutation in the neighbourhood (`to_stage.push(p)`) is outside the macro call. - **Emitted text is unchanged** — the format strings and arguments are copied verbatim, only the gate moved. - clippy clean and `--lib` green in both feature configurations (253 / 260), and **55/55 `plugin_ort_e2e`** against real ORT. Relates to #1077. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…uted-loop fixes (2.31 -> 1.71 at depth 100) (#1397) > **Note on scope.** Enabling auto-merge across the stack caused each stacked PR to merge into its *parent feature branch* (those branches are unprotected; only `main` requires checks), and GitHub retargeted the children. No `main` protection was bypassed and no work was lost, but this PR now carries the whole dispatch-overhead stack below #1387, not just the memory-info change: > > - #1394 — the #1077 benchmark grid > - **this PR** — `device_mem_info`, 7 → 4 FFI calls per `Run` > - #1401 — routed-loop instrumentation, the O(depth²) retirement scan, lazy absent strides, and the 8-element dispatch-isolating cases > > Each was independently reviewed before it merged; the sections below cover the memory-info change, and #1394/#1401 retain their own descriptions and review records. #1387 (probe + lazy formatting) remains a separate PR to `main`. ## Stop asking ORT three times where the inputs live Part of #1077. Stacked on #1394 (which measured the cost this removes). ### The finding Every `Compute` call resolves where a routed subgraph's intermediate tensors should be allocated. On a host EP — and the CPU plugin is always a host EP — that resolution cost **7 FFI calls** for a single-input node to arrive at the answer "host memory". Three of them were asking ORT for something already in hand. `device_mem_info` scans the inputs looking for a device-resident one: 1. per input: `GetValue` → `GetTensorMemoryInfo` → `mem_info_is_device` 2. on a host EP it never finds one, so the scan runs to completion 3. it then falls through to "use input 0's memory info" — and **fetches input 0 again**, two more calls for a value it had already looked at and discarded 4. the caller then asked `mem_info_is_device` on the result — a **third** query for a fact the function had just established internally and thrown away ### The change The scan remembers input 0's memory info as it walks, and returns `(mem_info, is_device)` so the caller does not have to re-derive the second half. The fallback fetch stays, for the one case that genuinely needs it: no input count available, so the scan never ran and nothing was remembered. ### Why this is behaviour-identical - The scan visits the same inputs, in the same order, and returns on the same condition. - The remembered value is exactly what the re-fetch would have produced: both are input 0's `OrtMemoryInfo`, and it is immutable for the lifetime of the value. - The `recon_mem_info` branch reports `is_device = true` without asking. That branch is only reachable when the scan found a device-resident input, so it is device-resident by construction — which is precisely what the removed call was confirming. ### Evidence `mem_info_cost` (new, in `compute.rs`) drives `device_mem_info` through a hand-built `OrtApi` and pins the call count. | scenario | before | after | |---|---|---| | host EP, 1 input | **7** | **4** | | device-resident input (short-circuit) | 3 | 3 | | no input count (fallback path) | 2 | 2 | | zero inputs | 0 | 0 | The "before" column is not a recollection — it is the same test run against the previous implementation. Three FFI calls per `Run`, off the fixed per-`Run` cost that #1394 measures at **~0.9 µs worse than ORT's**. ### Why counters and not a stopwatch This machine has not been quiet enough all session for a trustworthy wall-clock A/B (`mpstat` shows cores at 75–78% idle against a ≥93% gate). An FFI-call count is deterministic and load-independent, so it is reproducible now and will still be reproducible on someone else's machine. That is the case for #1387 existing. ### Also pinned The three non-host paths, so a future change cannot quietly break them while the host count stays pretty: device-resident short-circuit, missing input count, and a node with no inputs (reports no memory info rather than inventing one). ### Validation - `cargo test -p onnx-runtime-ep-plugin --lib` — 253 pass (debug; `--release` silently disables `debug_assert!`) - same with `--features dispatch_probe` — 264 pass - `plugin_ort_e2e` — **55/55**, including `every_assigned_node_is_also_executed_by_this_ep` - clippy clean in both feature configurations No ORT CPU fallback. No MLAS. --- ## Independent review (Opus 4.8, adversarial) — **APPROVE WITH NITS**, all findings addressed Commissioned specifically to attack "behaviour is unchanged in every case". It held for every EP that ships, with one crack worth fixing. **Finding 1 (MINOR) — the recon branch's `is_device = true` was convention, not construction.** Upheld, and fixed properly. My comment claimed the reconstruction is "device-resident by construction". The reviewer traced it: `recon_mem_info` is built from the EP's reported `device_type`, and `device_staging` is attached whenever an EP supplies a `HostToDeviceCopier` — and **nothing in that trait requires a copier-providing EP to be non-CPU**. Every one today is (CUDA→GPU, QNN→NPU), which is exactly what made the hardcode look safe. A CPU-typed EP that grew a copier would build a host-typed memory info and the hardcode would hand it to the device scratch path — host pointers treated as device pointers, the failure this code exists to prevent, inverted. The old code had no such exposure because it asked ORT. Rather than paper over it with a `debug_assert`, the invariant is now structural. `mem_info_is_device` is exactly `device_type != CPU`, and the reconstruction is created by passing that same `device_type` to `CreateMemoryInfo_V2` — so recording it at construction reproduces the old query **in every case, including the one that does not exist yet**, and still costs no FFI call. **Finding 2 (MINOR) — the one branch whose reporting changed was untested.** Correct: all four original tests passed `staging = None` and never reached it. Three tests added — CPU-typed reconstruction is not reported as a device, device-typed one is (4 calls, not 5), and a real device input still wins over the fallback. Falsified: restoring the hardcoded `true` fails **exactly one** test, the one naming the case; the other six correctly stay silent. **Finding 3 (NIT) — test 2's `== 4` does not distinguish new from old.** Accurate. The device-input path cost 4 before as well; the real guard in that test is the tuple value, not the count. Left as-is (it pins the device path against future regressions), noted here so nobody reads it as evidence for this PR. ### Claims the reviewer tried to break and could not These are now load-bearing rather than merely unchallenged: - **Pointer stability** — the remembered input-0 memory info cannot differ from what the re-fetch produced: same `OrtValue`, straight-line code with no intervening mutation, and the pointer's lifetime window is identical in both versions. - **The fallback path does not hardcode anything** — it still queries, so a device-resident input on that path still reports `true`, exactly as before. - **Zero-input nodes** — identical (`None`, not an invented memory info). - **Caller equivalence** — `Option<(*const _, bool)>` is `Copy`; no move, no shadowing, no ordering change. - **`mem::zeroed::<OrtApi>()`** — sound; all fields are `Option<extern fn>`, zero is the null-fn niche, and the sentinel pointers are never dereferenced. - **The `ffi_coverage` tripwire stays honest** — 9 members / 9 `ort_call()` sites, unchanged and correct: this PR removes *dynamic* calls, not *static* sites. - **Concurrency** — entirely stack-local; no new shared mutable state. - **No vacuous test** — the reviewer was briefed that I have twice shipped a test that passed against the broken version, and specifically hunted a third. None found. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Issue #1077 has ~7 allocations per node left and no way to say where they are. This adds per-phase allocation attribution and gets it reporting from a live routed dispatch. The allocator hook charges each allocation to the innermost open phase. PhaseGuard now saves the enclosing phase on enter and restores it on drop, so nested phases -- which the dispatch path really does produce, because a guard closes at scope exit rather than at an early return -- attribute to the inner one rather than losing the outer. Reading it back needs the counters to come from the library ORT dlopens, not from the test binary: the cdylib has its own global allocator, so an allocator installed in the harness never sees an allocation made inside Compute. The counting allocator is therefore installed in the cdylib under a new dispatch_probe feature, and the harness reads totals through the existing C export. Also fixes a resolver bug this uncovered. cdylib_resolve rebuilds the cdylib with a fixed feature list, and dispatch_probe was missing from it, so the rebuild silently replaced the instrumented library with a default-feature one and the probe reported zeros for every phase. The file already warned about this hazard for mlas. Nothing fails loudly when a feature is omitted, so the comment now says so explicitly. Production is unaffected: without the feature record_alloc is an inline no-op, no global allocator is installed, and the harness path is additionally gated on NXRT_MM_BENCH_PROBE=1. Falsified: reverting the outer-phase restore fails exactly the two nesting tests; the five attribution tests install the counting allocator for real rather than calling record_alloc by hand. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ing them The first live attribution run said four allocations per node and gave no way to tell whether that was all of them. It was a third of them. Allocations made with no phase open were being discarded rather than counted, so the per-phase rows could not be checked against any total. They now land in an explicit UNATTRIBUTED bucket, which makes the columns add up: 12.11 allocations per node on a 100-node chain, of which 4 are inside named phases and 8.03 are in the routed loop between them. The hand-placed DispatchAlloc tally independently reads 12.19, which is the cross-check that this is now complete. The harness also kept its own copy of the phase order, and the copy was wrong: it labelled TensorBind's allocations "OutputMeta" and Allocate's "TensorBind". Every number in that table was correct and attached to the wrong phase, which is the kind of error that survives review. The names now come from the library through a new nxrt_dispatch_probe_phase_name export, so there is one source of truth, and a test pins the export against Phase::name. Falsified: renaming one exported phase fails the pinning test; reverting the unattributed bucket fails the outside-a-phase test, which now asserts the allocation is visible rather than absent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 19, 2026
Main landed several dispatch-path changes that overlap this stack. Resolved in favour of keeping both sides' wins rather than either side wholesale: - staging_log! doc comment: main's wording (the macro bodies were identical). - Scratch memory-info resolution: main moved it inside the routed branch so the single-node path stops paying an ORT memory-info query per kernel-context input per Run. Kept that placement, but on this stack device_mem_info already returns (mem_info, is_device) so the follow-up mem_info_is_device call is not made a second time. SubgraphFallback:: Deferred maps the tuple back to the pointer. - Single-node output views: kept this stack's absent_slot_strides, which builds one stride entry per *absent* slot and allocates nothing when there are none, over main's variant that clones every output shape whenever any slot is absent. Took main's lazy ort_view_iter, which drops the collect- then-drain Vec of views per call. The DispatchAlloc counter that named that Vec goes with it. - Single-node operand list: took main's OrtOperands::Slots, which removes the per-call node_ort_operands Vec. Kept the DispatchLookup phase marker. Gates: onnx-runtime-ep-plugin lib 259 passed; plugin_ort_e2e 55 passed (including every_assigned_node_is_also_executed_by_this_ep); clippy clean on onnx-runtime-ep-plugin and onnx-runtime-ir with --all-targets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
#1387 landed as a squash, so git saw `dispatch_probe.rs` as added on both sides and produced 22 add/add hunks. Resolved by taking this branch's file (it already contains every #1387 commit as an ancestor, plus the allocation attribution on top) and re-applying the one change #1387 gained after this branch forked: gating the `no_mangle` entry points on the `dispatch_probe` feature so the shipped cdylib keeps exporting the ORT plugin ABI and nothing else. That re-application caught a fourth leaking export. This branch adds `nxrt_dispatch_probe_phase_name`, which #1387 never had, so it was neither gated nor in `plugin_export_abi::l1_no_symbol_leakage`'s allowance. Both are fixed here: the export is gated like the other three, and the allowance covers it, still behind `cfg!(feature = "dispatch_probe")` so ungating any of them fails the test rather than silently widening the shipped ABI. Refs #1077 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
Pre-merge validation (local, comprehensive)Retargeted onto That re-application caught a real defectThis branch adds a fourth export, Verified by mutation, not just by a passing run: ungating ABI verified both ways: production exports 0 probe symbols, The instrumentation is free in productionSame tiny (8-element)
No production cost — the small deltas sit inside the ~4% wall-clock noise floor. The counting allocator and the phase counters exist only under the feature. Gates
|
justinchuby
added a commit
that referenced
this pull request
Aug 19, 2026
Three conflicts, all from #1409 landing as a squash. `dispatch_probe.rs` and the cdylib manifest took main's side: main already carries everything this branch had there, plus the feature gating on the four `no_mangle` exports and the MLAS research-reference wording, and this branch changed neither. `compute.rs` kept this branch's side. The conflict was this branch's hoisted per-node scratch declarations against nothing on main, which is the whole point of the change rather than an incidental edit. Refs #1077 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 19, 2026
…1.713 → 1.416) (#1412) ## Why #1409's allocation table showed **12.11 allocations per node** on the routed dispatch path, and only 4 of them were inside named dispatch phases. The other 8 were per-node scratch vectors being created and dropped once each. ## What Six vectors — `ort_outputs`, `buf_writes`, `absent_scratch`, `slot_kinds`, `new_bufs`, `node_ort_operands` — are hoisted above the loop and cleared at the top of each iteration, so node N reuses node N−1's capacity. Clearing at *entry* rather than at exit is deliberate: the loop has many early `return`s on error paths, and clearing on exit would leave a node's contents visible to the next one whenever any of them fired. `buf_writes` also no longer clones the output shape. It stores the output *slot* and indexes `output_shapes`, which is already live for the whole iteration — the clone was an extra allocation per buffer-bound output for data that already existed. ## Result — 8-element Relu chain, depth 100, 1 thread, `taskset -c 13,15` Both arms measured on the **same commit** with latest `main` (`261b21155`) merged in — a control built by checking out this branch's parent, not a cross-session comparison: | | without this change | with it | |---|---|---| | allocations / node | 12.11 | **8.17** | | bytes / `Run` | 102 288 | **45 792** | | **ratio vs ORT CPU** | 1.691 | **1.451** | | per node | 0.39 µs | **0.32 µs** (ORT 0.22 µs) | Depth 100 reproduces to ±0.001 across repeats (1.451, 1.452). Depth 1 is unchanged within noise (1.49–1.56). That is the expected signature and worth stating explicitly: this touches per-node cost only, so it *should* leave the fixed per-`Run` floor alone. A change here that moved depth 1 would mean something unintended had happened. Remaining unattributed allocations are 4.09/node, down from 8.03. ## Safety The hoisted vectors were already per-node scratch with no cross-node meaning; the change is that their *buffers* outlive an iteration, not their contents. - **Dynamic shapes** — `output_shapes` is still recomputed per node by shape inference; `buf_writes` indexes into it rather than caching anything across nodes. - **Optional / absent slots** — `absent_scratch` and `slot_kinds` are cleared together and stay index-aligned by construction. - **Concurrency** — these are function-local, so a concurrent `Run` gets its own set. Nothing was promoted to session or thread-local state. - **Retained outputs** — `new_bufs` is drained into `intermediates` rather than moved, so ownership transfer is unchanged. ## Verification 259 lib tests and all 55 e2e conformance tests pass, including `every_assigned_node_is_also_executed_by_this_ep` — assigned == executed still holds, with no ORT CPU fallback and no MLAS. Clippy clean in both feature configurations. Latest `main` merged normally through the whole stack (no rebase, no history rewrite) and every figure re-measured on that base. `main`'s 13 new commits touch none of `onnx-runtime-ep-cpu`, `-ep-plugin`, or `-ep-cpu-plugin`. Refs #1077. Stacked on #1409. ## Independent review Reviewed by an independent Opus 4.8 agent, briefed adversarially to break the claim that node-to-node scratch reuse is behaviour-preserving. It traced seven attack surfaces: stale data across nodes, `slot_kinds`/`absent_scratch` index alignment, the `out_slot` re-indexing, dangling or aliasing views across the hoist boundary, concurrency, zero/one-node cardinality, and `reserve` vs `with_capacity`. **Verdict: APPROVE WITH NITS.** No correctness defect found. The load-bearing results: - `OwnedOutput` has no `Drop` and its pointer is ORT-owned, so the now-delayed drop has no observable effect. - `IntermediateBuf.data` is moved by `drain(..)` into `intermediates`, so it never aliases the reused `new_bufs` allocation. - All views into these vectors remain iteration-local; the borrow checker enforces they cannot survive into the reused allocation. - `output_shapes` is an immutable binding live for the whole iteration, so `&output_shapes[out_slot]` cannot panic and is identical to the removed clone. The clone that genuinely outlives the iteration (into `IntermediateBuf.shape`) is retained — only the redundant second one was dropped. Both nits were about my *comments*, and both were right: 1. `new_bufs.clear()` is dead while `drain(..)` is the only consumer — now commented as defensive rather than reading as load-bearing. 2. My "no node can observe another node's contents even on an early return" rationale was **vacuous**: every early exit in this loop is a `return` from `compute_execute`, so there is no later node to observe anything. The clears matter for ordinary node-to-node reuse. Comment corrected to say what is actually true. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 19, 2026
Resolves 12 conflicts from the six squash landings of the #1077 stack (#1387, #1409, #1412, #1430, #1433, #1472). compute.rs (6): took main's side throughout. Main is the superset -- #1397's `device_mem_info` now returns `(mem_info, bool)`, #1401 builds one stride entry per *absent* slot rather than cloning every output shape, and the entry/lookup probe spans are new. This branch's `has_absent` guard is subsumed by the per-absent-slot construction. kernel_ctx.rs (6): kept this branch's one-call `GetTensorElementTypeAndShapeDataReference` structure and interleaved main's probe counters into both the borrowed and the legacy path, so `Event::OrtFfiCall` stays a total. Took main's `impl std::fmt::Display` for `validate_dims` (a superset of this branch's `Arguments<'_>`). Kept the inline-rank output-dims path, with the allocation counter moved to the heap-only branch where the allocation actually happens. Two instrumentation updates the merge made necessary: - dispatch_probe's `every_ort_entry_point_is_accounted_for` fired exactly as designed: this branch adds a 12th ORT API member. Table updated to 12 members / 12 `ort_call()` sites. - The pinned costs `1 + 7` calls and `1 + 3` allocations describe the *legacy* path: `fake_api()` zeroes the struct, so the reference hook is null there and the fallback runs. The fast path -- the one real ORT 1.27 takes, and the entire point of this PR -- had no cost pin at all. Added `the_reference_hook_path_costs_exactly_three_ort_calls_per_input` (pins 1 + 3, down from 1 + 7) and `the_reference_hook_path_allocates_less_than_the_legacy_path`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 19, 2026
) ## What this is `read_inputs` runs **once per input per `Run`**, and it asked ORT for the element type and the dimensions through the classic five-call sequence: ``` GetTensorTypeAndShape → GetTensorElementType → GetDimensionsCount → GetDimensions → ReleaseTensorTypeAndShapeInfo ``` The first of those makes ORT **heap-allocate** an `OrtTensorTypeAndShapeInfo` and copy the shape into it; the last frees it. Five FFI crossings and one allocation on ORT's side of the boundary, per input, per `Run`, to read data the `OrtValue` already owns. `GetTensorElementTypeAndShapeDataReference` (ORT C API, since 1.24) returns the element type **and a reference to the value's own shape array** in one call, allocating nothing. This PR routes `read_inputs` through it. Stacked on #1244 (since merged; this branch now targets `main` directly). No kernel is touched, no numerics change, and node assignment and execution ownership are unchanged: the CPU EP still executes every node it claims, locally, with ORT CPU fallback off. ## The three cuts **1. Five ORT calls and one ORT-side allocation per input → one call, no allocation.** The plugin already fails closed below API 27, so the hook is always present in practice. The five-call sequence is kept as a fallback for a host that leaves it null, and a host offering **neither** now fails closed with a message naming both — it does not silently read garbage shapes. The borrowed pointer is used only to build the owned `shape`/`strides` of the `OwnedInput`; nothing retains it. ORT spells a scalar as a **null** pointer with count 0, and `slice::from_raw_parts` is UB on null, so that case is guarded explicitly — see *Correctness* below for how that guard is actually enforced. **2. `validate_dims` takes a lazily-formatted label, not `&str`.** Every call site passed `&format!("input {i}")`, i.e. a heap `String` built on the success path purely to label an error that almost never happens. Callers now pass `format_args!(..)` and the string is only materialised inside the error branches. One `String` per input per `Run` gone. All call sites converted, including `transfer.rs` and the unit tests; no message text changed. (The merge with `main` took main's `impl std::fmt::Display` signature here, which is a superset of this branch's original `Arguments<'_>` and keeps both callers working unchanged.) **3. `allocate_output` converts the output shape to `i64` on the stack.** `let dims: Vec<i64> = shape.iter().map(|&d| d as i64).collect()` was a heap allocation per output per `Run` whose entire lifetime was the `KernelContext_GetOutput` call below it. Rank ≤ 8 now goes to a stack array; a taller tensor still works, through the `Vec`. For a 1-in/1-out elementwise node that is **5 fewer ORT FFI calls**, one fewer ORT-side allocation, and **2 fewer heap allocations** per `Run`, on top of #1244. ## A/B, one thread `taskset -c 8-15`, this branch vs its merge parent (`origin/main` + #1244), **interleaved** (A,B,A,B,… so drift hits both arms equally), 400 iterations after 50 warmup, **five rounds**. Ratio is **ours/ORT, lower is better**; each column is the median of the five rounds. "ORT drift" is the control: ORT's own p50 measured in the two arms, which must not move for the comparison to mean anything. | case | before p50 | after p50 | before p90 | after p90 | ORT drift | rounds won | |---|---|---|---|---|---|---| | `sqrt_f32_4k` | 1.023 | **0.952** | 1.031 | **0.964** | +2.9 % | 4/5 | | `leakyrelu_f32_4k` | 1.202 | **1.097** | 1.210 | **1.104** | 0.0 % | 5/5 | | `hardsigmoid_f32_4k` | 1.213 | **1.144** | 1.226 | **1.149** | 0.0 % | 5/5 | | `thresholdedrelu_f32_4k` | 1.316 | **1.209** | 1.314 | **1.220** | 0.0 % | 5/5 | | `sigmoid_f32_4k` | 1.328 | **1.252** | 1.333 | **1.258** | 0.0 % | 5/5 | | `tanh_f32_4k` | 1.368 | **1.287** | 1.369 | **1.291** | 0.0 % | 5/5 | | `erf_f32_4k` | 1.383 | **1.327** | 1.387 | **1.329** | 0.0 % | 5/5 | | `celu_f32_4k` | 0.430 | **0.411** | 0.435 | **0.418** | 0.0 % | 4/5 | | `elu_f32_4k` | 0.437 | **0.416** | 0.443 | **0.422** | −1.8 % | 4/5 | | `selu_f32_4k` | 0.467 | **0.441** | 0.471 | **0.446** | 0.0 % | 5/5 | | `log_f32_4k` | 0.727 | **0.700** | 0.728 | **0.706** | 0.0 % | 5/5 | | `mish_f32_4k` | 0.268 | **0.264** | 0.271 | **0.266** | 0.0 % | 4/5 | 12 of 12 cases improve at p50 and p90; 56 of 60 individual arm-pairs favour the change, and no case regressed in its median. In absolute terms it is **0.2–0.3 µs off every `Run`**: `tanh_f32_4k` 4.20 µs → 3.90 µs against ORT's 3.10 µs, `thresholdedrelu_f32_4k` 3.10 µs → 2.80 µs against ORT's 2.30 µs. That is the shape of a fixed per-call cost being removed, which is what it is. `sqrt_f32_4k` crosses below 1.00 — the plugin path now dispatches that op faster than plain ORT does. ## A/B, four threads Same protocol, `NXRT_MM_BENCH_THREADS` = `ONNX_GENAI_MLAS_THREADPOOL_THREADS` = `RAYON_NUM_THREADS` = 4, three rounds. | case | before p50 | after p50 | before p90 | after p90 | |---|---|---|---|---| | `leakyrelu_f32_4k` | 1.069 | **0.966** | 1.075 | **0.976** | | `hardsigmoid_f32_4k` | 1.152 | **1.050** | 1.164 | **1.060** | | `thresholdedrelu_f32_4k` | 1.212 | **1.091** | 1.228 | **1.106** | | `sqrt_f32_4k` | 1.121 | **1.036** | 1.130 | **1.045** | | `sigmoid_f32_4k` | 1.346 | **1.264** | 1.359 | **1.263** | | `tanh_f32_4k` | 1.434 | **1.319** | 1.453 | **1.325** | | `erf_f32_4k` | 1.581 | **1.502** | 1.588 | **1.510** | | `celu_f32_4k` | 0.460 | **0.437** | 0.476 | **0.445** | | `elu_f32_4k` | 0.481 | **0.448** | 0.486 | **0.468** | | `selu_f32_4k` | 0.500 | **0.458** | 0.482 | **0.464** | | `log_f32_4k` | 0.766 | **0.731** | 0.781 | **0.734** | | `mish_f32_4k` | 0.332 | **0.322** | 0.345 | **0.339** | 36 of 36 arm-pairs favour the change. The cost is per call, not per element or per worker, so the gain is the same absolute microseconds at every thread count. ## Drift control: the 1 Mi grid does not move Same protocol, `_f32_1m`, 200 iterations after 30 warmup, five interleaved rounds, one thread. A 1 Mi tensor amortises a fixed per-call cost away, so **these must not move** — and they don't: | case | before | after | | case | before | after | |---|---|---|---|---|---|---| | `relu_f32_1m` | 1.024 | 1.028 | | `sqrt_f32_1m` | 0.313 | 0.309 | | `exp_f32_1m` | 0.580 | 0.576 | | `log_f32_1m` | 0.264 | 0.258 | | `sigmoid_f32_1m` | 0.588 | 0.584 | | `mish_f32_1m` | 0.100 | 0.099 | | `tanh_f32_1m` | 0.618 | 0.620 | | `celu_f32_1m` | 0.127 | 0.129 | | `erf_f32_1m` | 0.651 | 0.668 | | `elu_f32_1m` | 0.129 | 0.129 | | `gelu_exact_f32_1m` | 0.585 | 0.584 | | `selu_f32_1m` | 0.136 | 0.138 | | `gelu_tanh_f32_1m` | 0.602 | 0.646 | | `hardsigmoid_f32_1m` | 0.455 | 0.453 | | `fastgelu_f32_1m` | 0.608 | 0.639 | | `leakyrelu_f32_1m` | 0.409 | 0.417 | | `quickgelu_f32_1m` | 0.429 | 0.438 | | `thresholdedrelu_f32_1m` | 0.567 | 0.573 | The per-case round-win counts here are 0/5–4/5 with a median of 2/5, i.e. a coin flip — the signature of noise, not of an effect. The two largest movers, `gelu_tanh` (+7 %) and `fastgelu` (+5 %), are the two cases whose **ORT** side also drifted most in the same rounds (−4.4 % and −2.8 %), so the ratio moved because the denominator did. This host's floor is roughly ±5 % on the 1 Mi grid and I am not claiming anything below it. ## Correctness **Five new tests, three with a verified falsifier**, driving `read_inputs` and `allocate_output` against a hand-built `OrtApi` whose hooks count their calls. | test | what it pins | falsifier | |---|---|---| | `input_shapes_come_from_one_call_when_ort_offers_the_reference_hook` | 1 reference call, **0** legacy calls, and the shape/strides/dtype that come out | forcing the legacy route fails it with `left: 0, right: 1` (run) | | `the_five_call_fallback_produces_the_same_input_as_the_reference_hook` | the fallback is exercised and its `OwnedInput` matches the reference path field for field | — (it *is* the parity check) | | `a_borrowed_scalar_shape_never_dereferences_null` | ORT's documented scalar spelling — null pointer, count 0 — takes the borrowed route and yields rank 0 | deleting the null guard makes **Miri** report `out-of-bounds pointer use: null pointer is a dangling pointer` at `kernel_ctx.rs:280` (run) | | `read_inputs_fails_closed_when_no_shape_route_exists` | the error names **both** routes; no silent garbage shapes | — | | `output_dims_are_identical_on_the_inline_and_heap_ranks` | ranks 0/1/8/9/12 all reach ORT with every dimension intact | dropping the heap arm delivers rank 9 truncated to 8 dims (run) | **The null guard is now actually enforced, not just asserted.** Natively, `from_raw_parts(null, 0)` returns an empty slice, so a test asserting the resulting shape passes with or without the guard — the guarantee only exists if Miri sees it. `onnx-runtime-ep-plugin` was not in the Miri matrix, so this PR adds `kernel_ctx::` as a lane in `.github/workflows/miri.yml` (the crate's other modules dlopen ORT and are not Miri-tractable, which is why the lane is scoped to the module rather than the crate). 25 tests pass under Miri in 4.15 s; with the guard deleted the lane fails. That pairing is what makes the test non-vacuous. **Suites.** `-p onnx-runtime-ep-plugin`: **252** unit tests pass (247 before, +5 new). `-p onnx-runtime-ep-cpu-plugin` with `NXRT_REQUIRE_ORT_TESTS=1`: every suite green, including all **55** `plugin_ort_e2e` cases — `every_assigned_node_is_also_executed_by_this_ep`, `no_supported_node_is_ever_left_to_the_ort_cpu_ep`, `no_matmul_family_node_escapes_to_the_ort_cpu_ep` and `every_fixture_loads_with_cpu_fallback_disabled` all pass, so **assigned still equals executed** with ORT CPU fallback disabled. `cargo clippy --release --all-targets -p onnx-runtime-ep-plugin` and `cargo fmt` are clean. **Build identity.** Pure native CPU EP: no MLAS at runtime, no ORT CPU EP fallback, no new dependency. AVX2/FMA host (`avx2 fma f16c`, no AVX-512), so ORT and we are on the same instruction footing. ## Independent review Reviewed by **Claude Opus 4.8**, read-only, briefed with the exact ORT contract for the borrowed pointer and asked specifically to hunt UB and vacuous tests. Verdict **APPROVE**, no blockers. It confirmed the null/scalar guard is unreachable-by-construction for `from_raw_parts`, that the borrow cannot outlive the `OrtValue` (its only consumer copies), that `ReleaseTensorTypeAndShapeInfo` still runs on every legacy error path — and noted that moving `DataType::from_onnx` after the match incidentally closes a **pre-existing** leak of `type_shape` on the unsupported-dtype path. It raised two test-quality defects, both real and both **fixed** in `6027f6e16`: 1. The three counting tests shared two process-wide `AtomicUsize`es while asserting exact counts, and libtest runs them in parallel — one test's reset could land inside another's assertion window. They now take a shared `SHAPE_COUNTER_LOCK` that resets both counters under the guard. 2. `a_borrowed_scalar_shape_never_dereferences_null` was **vacuous** with respect to its name, for exactly the reason above, and the crate was not under Miri. Hence the Miri lane, and the test now also asserts the scalar went through the borrowed route so it cannot pass on the fallback. It also flagged that `OrtStatus` is not released on error paths in this file. That is pre-existing and repo-wide in `kernel_ctx.rs` — the old five-call code leaked identically — so it is not touched here; it belongs in its own change. ## What is left After #1244 and this PR, a 1-in/1-out elementwise `Run` is down to roughly `KernelContext_GetInputCount` + `GetInput` + the one shape reference + `GetTensorData` + `GetOutput` + `GetTensorMutableData`. What remains: * **`OwnedInput`'s `shape` and `strides` `Vec`s**, `kernel_inputs`, `infer_shapes`'s `Vec<Vec<usize>>`, `slot_map`, `output_views`, and the `Box<HostPool>` in `host_pool::install` — each worth ~25–35 ns. Removing them needs an inline-capacity vector type or per-session caching of the parts that cannot change between `Run`s. Both are worth doing; neither belongs here. * **The `HostPool` box** specifically is @sebastian's 16-thread scheduling file and should come from him or after his PRs land. --- ## Refreshed against `main` @ `6a855d5e0`, and measured as a stack `origin/main` moved a long way while this sat in the CI queue (#1346, #1352 and #1361 on the quality lane; #1154, #1232, #1238 on the CPU side). Merged in normally — no rebase — and re-measured from scratch against the new baseline. Production pure-native A/B, plain ORT as the control arm. No MLAS, no ORT CPU fallback, no deferral. `taskset -c 8-15`, one thread, 400 iterations, five interleaved rounds out of two worktrees, started only once cores 8-15 were >=93% idle. **Ratio is ours/ORT, lower is better.** `before` is `main` at `6a855d5e0`; `after` is #1244 + #1246 together, since #1246 is stacked on #1244 and the pair is what a user gets. | case | ratio p50 main | ratio p50 stack | Δ | ratio p90 main | ratio p90 stack | ours us | ORT drift | rounds won | |---|---|---|---|---|---|---|---|---| | `thresholdedrelu_f32_4k` | 1.512 | **1.216** | -19.6% | 1.519 | **1.216** | 3.6 → **2.8** | -4.2% | 5/5 | | `tanh_f32_4k` | 1.511 | **1.279** | -15.4% | 1.513 | **1.284** | 4.6 → **3.9** | +0.0% | 5/5 | | `sigmoid_f32_4k` | 1.475 | **1.248** | -15.4% | 1.484 | **1.256** | 4.7 → **4.0** | +0.0% | 5/5 | | `erf_f32_4k` | 1.470 | **1.363** | -7.3% | 1.489 | **1.364** | 7.5 → **7.0** | +0.0% | 5/5 | | `hardsigmoid_f32_4k` | 1.416 | **1.127** | -20.4% | 1.426 | **1.133** | 3.5 → **2.8** | +0.0% | 5/5 | | `leakyrelu_f32_4k` | 1.361 | **1.094** | -19.6% | 1.371 | **1.104** | 3.5 → **2.8** | +0.0% | 5/5 | | `sqrt_f32_4k` | 1.141 | **0.946** | -17.1% | 1.150 | **0.955** | 4.0 → **3.4** | -2.8% | 5/5 | | `log_f32_4k` | 0.767 | **0.698** | -9.0% | 0.776 | **0.703** | 7.9 → **7.2** | +0.0% | 5/5 | | `selu_f32_4k` | 0.505 | **0.440** | -12.9% | 0.512 | **0.444** | 5.6 → **4.9** | +0.0% | 5/5 | | `elu_f32_4k` | 0.480 | **0.416** | -13.3% | 0.488 | **0.421** | 5.3 → **4.6** | +0.0% | 5/5 | | `celu_f32_4k` | 0.470 | **0.411** | -12.6% | 0.478 | **0.418** | 5.7 → **5.0** | +0.0% | 5/5 | | `mish_f32_4k` | 0.276 | **0.264** | -4.3% | 0.278 | **0.268** | 17.3 → **16.6** | +0.0% | 5/5 | **Every case, every round.** The two rows with a moving control (`sqrt` -2.8%, `thresholdedrelu` -4.2%) are reported rather than dropped; both won 5/5 anyway and their absolute time fell by the same ~0.7 us as everything else. That constant ~0.7 us is the point. It is not proportional to tensor size — the same absolute amount comes off `hardsigmoid` (3.5 -> 2.8 us) as off `mish` (17.3 -> 16.6 us) — which is what a fixed per-`Run` cost looks like when you remove some of it. It moves the cheap ops the most because they had the least to hide it behind, and `sqrt` crosses from 1.141 to **0.946**, from a loss to a win. ### Where the remaining time goes Measured directly, by instrumenting `compute_execute` segment by segment on top of this stack (temporary probe, not committed; `perf` is unavailable on this host — `perf_event_paranoid=4`). Per `Run`, one-in/one-out elementwise node, 4096 `f32`, microseconds: | segment | us | note | |---|---|---| | `KernelContext_GetOutput` | 0.35 | ORT's own API — ours to call, not to optimise | | `read_inputs` | 0.15 | 4 ORT FFI calls, already one shape call after #1246 | | rest of `allocate_output` | 0.13 | `GetTensorMutableData` + strides | | `prepare_workspace` | 0.09 | metadata vector + plan-cache lookup, for a kernel needing 0 bytes | | `host_pool::install` | 0.05 | @sebastian's, not touched | | `infer_shapes` | 0.05 | | | `kernel_inputs` | 0.04 | | | `output_views` | 0.04 | | Non-kernel node cost is **~1.25 us and near-constant across all twelve operators** (0.28 to 15.2 us of kernel time), which is the direct confirmation that small-node ratios on this EP are dispatch-bound rather than kernel-bound. There is no single large item left — the biggest, `KernelContext_GetOutput`, is ORT's. The rest is a long tail of 0.04-0.15 us items, which is what #1358 (`InlineVec`) starts on. ### And nothing breaks at 1 Mi Same harness, 1048576 elements, 120 iterations, 3 rounds. A fixed per-`Run` cost should be invisible here, and it is: | case | ratio p50 main | ratio p50 stack | ours us | ORT drift | |---|---|---|---|---| | `celu_f32_1m` | 0.141 | 0.140 | 382.1 → 377.9 | -0.0% | | `elu_f32_1m` | 0.138 | 0.136 | 347.7 → 343.6 | +0.1% | | `erf_f32_1m` | 0.671 | 0.670 | 595.2 → 594.6 | +0.1% | | `exp_f32_1m` | 0.606 | 0.590 | 247.0 → 240.5 | +0.1% | | `fastgelu_f32_1m` | 0.643 | 0.648 | 415.3 → 413.2 | -1.7% | | `gelu_exact_f32_1m` | 0.572 | 0.592 | 706.1 → 710.6 | -2.8% ⚠ | | `gelu_tanh_f32_1m` | 0.657 | 0.651 | 414.8 → 410.5 | +0.3% | | `hardsigmoid_f32_1m` | 0.376 | 0.372 | 88.0 → 69.6 | -0.9% | | `leakyrelu_f32_1m` | 0.421 | 0.430 | 90.1 → 87.3 | -2.2% ⚠ | | `log_f32_1m` | 0.270 | 0.274 | 616.4 → 612.4 | +1.7% | | `mish_f32_1m` | 0.105 | 0.105 | 1626.0 → 1626.0 | -0.2% | | `quickgelu_f32_1m` | 0.455 | 0.453 | 321.5 → 320.0 | -0.5% | | `relu_f32_1m` | 1.034 | 1.022 | 131.7 → 130.2 | +0.0% | | `selu_f32_1m` | 0.147 | 0.148 | 374.1 → 371.8 | -1.5% | | `sigmoid_f32_1m` | 0.471 | 0.606 | 231.6 → 229.0 | -23.4% ⚠ | | `sqrt_f32_1m` | 0.314 | 0.302 | 148.1 → 143.9 | +1.1% | | `tanh_f32_1m` | 0.644 | 0.631 | 226.2 → 221.9 | -0.2% | | `thresholdedrelu_f32_1m` | 0.500 | 0.486 | 70.9 → 68.6 | -0.3% | Flat, as predicted — 0.7 us against 70-1626 us of work. Absolute time is equal or better in 16 of 18 cases. The two ⚠ rows had the control move more than the effect: `sigmoid` is unusable (ORT itself moved -23.4%; our own absolute went 231.6 -> 229.0 us), and `gelu_exact`'s +0.6% absolute sits inside its -2.8% control. Reported rather than dropped. This is the coverage claim for the change: it buys ~0.7 us at every size, which is 20% of a small node and nothing at all of a large one, and it costs nothing anywhere. --- ## Status after merging latest `main` (2026-08-19) Validated on latest `main` after the six-PR #1077 stack landed (#1387, #1409, #1412, #1430, #1433, #1472). Full evidence in the PR comment below; the measured summary: **Deterministic counters, `relu_1_tiny`** — `OrtFfiCall` **10 → 6 per `Run`**, `DispatchAlloc` **24 → 20**. Four fewer round trips for one input, i.e. the predicted 7 → 3 per input. **Timing A/B**, production build, 3 clean reps (a 4th discarded for contention — it read 0.973, which would have flattered us): | case | main | this PR | |---|---|---| | `relu_1_tiny` | 1.397 | **1.271** | | `relu_10_tiny` | 1.250 | **1.173** | | `relu_100_tiny` | 1.163 | **1.110** | The gain shrinks as depth grows — the signature of a fixed-cost fix, since this removes FFI calls per `Run`, not per node. Fixed per-`Run` overhead **1.38 → 1.23** against ORT; per-node slope unchanged. **Test coverage gap this merge exposed and closed:** the pinned costs (`1 + 7` calls, `1 + 3` allocations) run against a zeroed `fake_api()`, where the reference hook is null — so they describe the *legacy fallback*, not the fast path this PR exists to add. Added `the_reference_hook_path_costs_exactly_three_ort_calls_per_input` (pins `1 + 3`) and `the_reference_hook_path_allocates_less_than_the_legacy_path`. `dispatch_probe`'s FFI-coverage table updated to 12 members / 12 `ort_call()` sites, which is the guard that flagged the new API member in the first place. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Resch <resch@squad.local>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1409 +/- ##
===========================================
- Coverage 82.10% 80.12% -1.99%
===========================================
Files 12 377 +365
Lines 5471 164946 +159475
Branches 5471 164946 +159475
===========================================
+ Hits 4492 132158 +127666
- Misses 780 27955 +27175
- Partials 199 4833 +4634
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
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.
Why
Issue #1077 has ~7 allocations per node left on the CPU Plugin EP's dispatch path and no way to say where they are. This makes them countable.
The answer turned out to be 12.11 allocations per node, not 7.
Attributing allocations across the ORT ABI
The allocator hook charges each allocation to the innermost open dispatch phase.
PhaseGuardnow saves the enclosing phase onenterand restores it on drop, so nested phases — which the dispatch path really does produce, because a guard closes at scope exit rather than at an earlyreturn— attribute to the inner one rather than losing the outer.Reading it back needs the counters to come from the library ORT
dlopens, not from the test binary: the cdylib has its own global allocator, so an allocator installed in the harness never sees an allocation made insideCompute. The counting allocator is therefore installed in the cdylib behind a newdispatch_probefeature, and the harness reads totals through the existing C export.Result — 100-node 8-element Relu chain, 1 thread
callback_entrymetadata_querytensor_bindallocatedispatch_lookupkernel_invokestatus_crossingunattributedFixed per
Run: 8 allocations (callback_entry1,metadata_query4,allocate3).The bulk of the per-node cost is not in the named phases — it is the 8.03/node in the routed loop between them. That is the target for the follow-up.
Three corrections this PR makes to its own instrument
Allocations outside every phase were discarded, not counted. The first live run reported 4 allocations/node inside phases and gave no way to tell whether that was all of them or a third of them. It was a third. They now land in an explicit
UNATTRIBUTEDbucket so the columns add up to the process total. The independent hand-placedDispatchAlloctally reads 12.19/node against this table's 12.11 — that agreement is the check that the count is now complete.The harness mislabelled two phases. It kept its own copy of the phase order and the copy was wrong: it printed
TensorBind's allocations under "OutputMeta" andAllocate's under "TensorBind". Every number was correct and attached to the wrong phase — the kind of error that survives review because nothing looks anomalous. Names now come from the library via a newnxrt_dispatch_probe_phase_nameexport, and a test pins that export againstPhase::name, so there is one source of truth.The resolver silently measured the wrong binary.
cdylib_resolverebuilds the cdylib with a fixed feature list, anddispatch_probewas missing from it, so the rebuild replaced the instrumented library with a default-feature one and the probe reported zeros for every phase. The file already warned about this hazard formlas; nothing fails loudly when a feature is omitted, so the comment now says so and lists the requirement.Production impact
None. Without the feature,
record_allocis an#[inline(always)]no-op, no global allocator is installed, and the harness path is additionally gated onNXRT_MM_BENCH_PROBE=1. Default-config clippy is clean and 259 lib tests pass.Falsification
#[global_allocator]for real rather than callingrecord_allocby hand.Scope
Instrumentation and measurement only — no dispatch behaviour changes. Refs #1077.