Repository navigation
fix(session): stop --phase-profile paying for the activation-memory planner - #1226
Merged
Merged
Conversation
…lanner The executor ran its activation-memory planner whenever phase profiling was on. The planner rebuilds the view map and re-plans every activation on every run - work the shipped runtime never does. So the profiler perturbed the run it was measuring and then reported its own cost back as a phase of that run, `run_scoped.activation_memory_plan`, which no unprofiled run ever pays. The planner now has its own gate. --phase-profile calls enable_exec_phase_profile_for_process, which drives the profiler only; NXRT_EXEC_PHASE_PROFILE=1 in the environment still enables both, so the CLI memory report keeps its stats via the path it already used, and NXRT_ACTIVATION_MEMORY_PLAN=1 opts in without profiling. Magnitude, measured honestly. The planner's own span reads 1.9-6.0 us per run. Removing it lowers native_min on sm_decode_h32_kv4096 from 0.065 ms to 0.063 ms, median of 15 interleaved repetitions, 300 runs each, single thread - about 3%, in line with the span. An earlier draft of this message claimed 35% and claimed the planner also polluted the adjacent exec_kernel.compute figure. Review could not reproduce either, and a properly interleaved re-measurement showed both were artifacts: this host is bimodal under contention, the 0.065/0.088 ms split appears in both arms, and exec_kernel.compute swings 75-168 us/call regardless of the planner. Both claims are withdrawn. The change stands on the smaller effect and on not reporting a phantom phase. The CUDA step profiler used to switch the planner on as a side effect of enabling phase accounting, so it taxed the very decode steps it exists to time. It no longer does; that coupling was accidental and is not restored. The first attempt at a regression test was vacuous. The gate was hardcoded true under cfg(test), so the production wiring was the one line no unit test could reach, and re-coupling the planner to phase profiling still passed. That override is gone: tests and production now share one gate, the two tests that need planning ask for it explicitly through an RAII guard that clears it again on drop, and re-coupling the wiring now fails both of them. The process-global gates are plain atomics with no per-test isolation, so every test that writes them takes a shared lock. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
force-pushed
the
squad/leon-p15-profiler
branch
from
August 18, 2026 06:49
075d8ea to
25a9c10
Compare
justinchuby
marked this pull request as ready for review
August 18, 2026 06:49
justinchuby
added a commit
that referenced
this pull request
Aug 18, 2026
…ments (#1230) Records Phase 10 in the CPU-EP evidence document. **30.1-30.2** — the profiler defect fixed in #1226. The activation-memory planner ran on every run whenever phase profiling was on, so `--phase-profile` perturbed the run it was measuring and reported its own cost back as a phase of that run. Real magnitude: the planner's own span reads 1.9-6.0 us/run, and removing it lowers `native_min` on `sm_decode_h32_kv4096` from 0.065 ms to 0.063 ms (median of 15 interleaved repetitions, 300 runs each, 13/15 favouring the fixed arm) — about 3%. **Withdrawn:** the 35% figure (0.065 -> 0.088 ms, ratio 1.023 -> 1.080) and the claim that the planner polluted the adjacent `exec_kernel.compute` measurement. Review could not reproduce either; interleaved re-measurement showed the 0.065/0.088 split appears in *both* arms and `exec_kernel.compute` swings 75-168 us/call regardless. **30.3** — the methodology lesson, which is the point of the section. The withdrawn numbers came from running arm A three times then arm B three times and reading the mean, on a host that drifts between the halves. Now the standing rule for this document: interleave at the innermost level, prefer `native_min` to the mean on small graphs, and distrust an effect an order of magnitude larger than the mechanism it is attributed to — the planner's span said 2-6 us while the wall clock claimed 23 us, and that inconsistency was visible in my own output before review saw it. **30.4** — the fourth vacuous test found in this effort, and the one that is actually interesting: it passed, and still passed when the behaviour it covered was deliberately reverted, because the production wiring sat under `#[cfg(test)]` where no unit test could reach it. Also notes that section 21's boundary instrument, and every per-phase attribution derived from it, were taken under the coupled gate and should be re-read. Documentation only. No code changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🔴 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 18, 2026
## What `main` at 4d231ea fails two blocking CI steps on current stable (rustc/rustfmt 1.9.0, 1.97.1): ``` $ cargo fmt --all -- --check Diff in crates/onnx-runtime-ep-cuda/src/provider.rs Diff in crates/onnx-runtime-memory-api/src/allocator.rs Diff in crates/onnx-runtime-memory-api/src/capability.rs $ cargo clippy --locked --all-targets -p onnx-runtime-session -- -D warnings error: field `0` is never read --> crates/onnx-runtime-session/src/executor/mod.rs:175:45 ``` Both landed while the runner queue was saturated (71 queued / 1 in progress at the time of writing, nothing completed on `main` since 08:18Z), so no PR has seen a red check yet. Every open PR in the repo currently inherits both failures. ## Why these fixes **rustfmt** — mechanical normalisation, no semantic change. **`ActivationPlanForTest`** (from #1226) is a tuple struct whose single field is the `globals_lock()` `MutexGuard`. `dead_code` does not model "this field's value is its `Drop`", so it fires. The guard must stay: releasing it early is exactly the leaked-planner-gate race the struct was added to prevent. So the lint is silenced with a comment explaining the RAII intent, rather than the field removed. ## Verification | check | result | | --- | --- | | `cargo fmt --all -- --check` | clean | | `cargo clippy --locked --all-targets -p onnx-runtime-session -- -D warnings` | clean | | `cargo test --locked -p onnx-runtime-session --lib` | 181 passed, 0 failed | Found while trying to land the CPU task-runtime stack (#1201 → #1202 → #1207 → #1232 → #1238); this is unrelated to that work and is deliberately kept out of it so it can go in on its own. Working as sebastian (Performance Engineer) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 18, 2026
…-ORT headline) (#1259) ## What Records the correction of record for **#1218** (erf's Estrin-scheme reassociation), appended as **§32** to the CPU-EP-vs-ORT ledger — the same document and style where the #1226 (§30.2) and #1230 withdrawals live. #1218 landed as *"1.66x -> 1.36x vs ORT"* (~17.7% kernel-time reduction). On the review host that ratio is **unreproducible** — there is no pinned ORT reference build here, and the original number came from different hardware — so it is **withdrawn**. Stated plainly: it was **neither reproduced nor refuted**, because it was measured against a reference we do not run. ## What the doc now records - **The withdrawal**, with the reason (no ORT arm on this box), and a note that the overstated figure reached **no benchmark table, README, or source comment** — only the (unrewritable) squash-commit subject. - **The measured number:** ORT-independent same-binary A/B on `erf_avx2`, single-thread, min-of-200 × 9 rounds, 1M elements, on **Intel Core i7-13800H** — Horner 0.6346 ms → Estrin 0.6042 ms, non-overlapping ranges → **~5.0% (1.05x)**. Real, but a fraction of the implied ~17.7%. - **The durable correctness fact, stated in as many words:** this reassociation is **NOT bit-identical**. Estrin differs from Horner by **at most exactly 1 ULP** (1.09% of 404k points, max abs 5.96e-8); vs f64 truth Horner 5.77e-8 / Estrin 6.55e-8 (both within ~1 ULP of correctly-rounded erf); large |x| → ±1.0, denormals bit-identical, NaN → NaN, ±inf → ±1.0. Names the gate test **`erf_reassociation_costs_no_accuracy`** as the tripwire. ## Validation Documentation only — no source, no benchmarks re-run. No markdown linter/build exists in-tree, so there is nothing to build; the change is prose appended to an existing report. Refs #1218. Companion to the separate threshold-tuning issue filed for #1149. Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 defect
The executor ran its activation-memory planner whenever phase profiling was on:
The planner rebuilds the view map and re-plans every activation, on every run -
work the shipped runtime never does. So
--phase-profileperturbed the run it wasmeasuring, and then reported its own cost back as a phase of that run
(
run_scoped.activation_memory_plan) that no unprofiled run ever pays.Magnitude - corrected after review
The planner's own span reads 1.9-6.0 us per run. Removing it lowers
native_minon
sm_decode_h32_kv4096from 0.065 ms to 0.063 ms - median of 15 interleavedrepetitions, 300 runs each,
--native-threads 1 --ort-intra-threads 1, both arms inone loop. About 3%, in line with the span. 13 of 15 repetitions favour the fixed
arm.
Two claims from the first draft of this PR are withdrawn. It claimed a 35%
inflation (0.065 -> 0.088 ms, ratio 1.023 -> 1.080) and that the planner also
polluted the adjacent
exec_kernel.computefigure (84.5 vs 60.3 us/call). Thereviewer could not reproduce either. A properly interleaved re-measurement showed
both were artifacts of a bimodal, contended host and of comparing arms across
separate invocation windows: the 0.065/0.088 split appears in both arms, and
exec_kernel.computeswings 75-168 us/call regardless of the planner. My originalnumbers came from non-interleaved runs, which is exactly the discipline this repo's
own benchmark notes insist on and which I did not follow.
The change stands on the smaller, reproducible effect and on the profiler no longer
reporting a phantom phase.
The fix
The planner gets its own gate, separate from the profiler's:
--phase-profilecallsenable_exec_phase_profile_for_process, which now drivesthe profiler only.
NXRT_EXEC_PHASE_PROFILE=1in the environment still enables both, so the CLImemory report (
interactive.rs->activation_memory_plan_stats) keeps workingthrough the path it already used.
NXRT_ACTIVATION_MEMORY_PLAN=1, orenable_activation_memory_plan_for_process(),opts into planning without profiling.
Reading the environment directly, rather than calling
enabled(), is what separatesthe programmatic enable from the environment one.
Deliberate behaviour change: the CUDA step profiler
(
native_decode/cuda.rs) calledenable_exec_phase_profile_for_processand soswitched the planner on as a side effect - taxing the very decode steps it exists to
time. That coupling is not restored, and the reason is now a comment at the call
site. A CUDA-step-profiling user who was relying on the incidental activation-plan
numbers must now ask for them explicitly. Raised by review as finding 1.
The first regression test was vacuous
The gate was hardcoded
trueundercfg(test):so the production wiring was the one line no unit test could reach. My first test
asserted on the
phase_profilemodule functions, passed, and still passed when Ire-coupled the planner to phase profiling in
run.rs. It pinned the parts, not thewiring.
The
cfg(test)override is gone. Tests and production share one gate, the two teststhat need planning ask for it through an RAII guard that clears it again on drop, and
re-coupling the wiring now fails both of them (verified).
Review findings addressed
"works exactly as before" wording is corrected to "the environment path works as
before".
PLAN_STATE = on- replaced the bareforce_activation_plan_enabled(true)with an RAII guard that takes the globalslock and clears the gate on drop, including on panic.
cost->coststypo - fixed.activation_memory_plan_statsdoc implied native+executed was sufficient -now states the gate is required.
Tests
cargo test -p onnx-runtime-session --lib- 181 passedcargo test -p onnx-genai-engine --lib- 405 passedcargo test -p onnx-runtime-ep-cpu --lib- 1340 passedcargo test -p onnx-runtime-ep-cpu-plugin --test plugin_ort_e2e- 54 passedcargo fmt --all --check,cargo clippycleanNo ORT fallback, no defer, no runtime MLAS, no scheduler changes.