Repository navigation
Free executor intermediates once their last consumer has run - #1498
Merged
Merged
Conversation
A native CUDA pipeline run that failed anywhere below the driver surfaced
as "native CUDA decoder forward pass failed" and nothing else. anyhow's
Display prints only the outermost context, so every layer of diagnosis
the lower crates had carefully attached was discarded at the boundary
where a human would read it. Switching the two driver formats to the
alternate `{err:#}` selector prints the whole chain.
With the chain visible, four real dtype gaps became legible instead of
being guessed at:
* Reduce* on CPU accepted Int64 only for Sum. A graph reducing Int32,
or taking Max/Min/Prod over integers, hit the dtype guard rather
than the integer path that was already sitting there.
* SkipLayerNormalization on CPU required its optional `mean` and
`inv_std_var` outputs to carry the input dtype. ORT's schema types
them `float`, but exporters routinely emit them in the activation
dtype instead, and the write already narrows to whichever was
declared -- so both should be accepted. The bf16 skip-RMSNorm path
documents the same allowance.
* SkipLayerNormalization on CUDA made the mirror-image assumption,
deriving the stat pointers' dtype from the input rather than from
the declared output. Threading the resolved stat dtype through to
the kernel keeps a float32 stat pair on a bf16 input from being
written as bf16.
* Range on CPU had no Int32 branch.
Each gap gets tests at the boundary that was previously unreachable.
Note for reviewers: `onnx-runtime-ep-cuda --lib` has five failures on
this machine (two in graph::tests, three in matmul_nbits fp16 tests).
They reproduce identically on an unmodified main, so they are not from
this change.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
The executor already computed `last_use` for every value, but the only consumer was `inplace_dead_inputs`, whose conditions are narrow enough (output must match an input in both dtype and shape) that in practice it covers elementwise chains and nothing else. Any operator that changes shape -- MatMul, Reshape, Attention -- allocated a buffer that then sat resident until the run ended. A graph is therefore charged for its peak node count rather than its peak live set. The vision encoder made the difference concrete. Its 2545 node outputs were all held simultaneously, costing roughly 23 GB at 448px and making full-resolution images unservable: a 960x686 image OOMed an 80 GB card. With dead values freed the same run peaks at 55.8 GB and completes. Four details worth calling out, because each is easy to get wrong: `DeviceBuffer` is a bare handle with no `Drop`. Dropping it from the value map looks like a release and is actually a leak -- the first version of this change did exactly that and reported 7075 releases while device memory did not move at all. The buffer has to go back through `ep.deallocate`. `buffer_shapes` has to be cleared alongside it. `ensure_buffer` treats a surviving shape entry as proof the allocation is still there and skips sizing it, so leaving one behind hands the next run a value it believes is backed and is not. Every other path that takes a buffer out of the map does the same thing for the same reason. A memoized loop-invariant `If` skips its branch on later runs and serves its outputs straight from the buffers an earlier run left resident, and that memo survives an eager run. Freeing one of those outputs turns the next skip into a missing-buffer error on the *second* request rather than the first. `try_move_host_output` already declines for exactly this reason; this pass is the second buffer-stealing path and now carries the same guard. The release is per-session and off by default because it is not safe under device graph capture. Capture bakes device pointers into the recorded graph, so freeing a buffer lets a later allocation land at a different address and replay then reads the wrong memory; the decoder fails immediately with a capture validation violation. Restricting this to the eager path is not sufficient either, since one session runs eager before it captures. Components that never capture -- the pipeline components, which run once per request -- opt in; the decoder does not. Aliased values (`pinned`) are never released, even after every alias has died. That is deliberate: `pinned` is a monotone record of "was ever an alias source", and releasing on the exact liveness test instead would be wrong for the decode memo, which restores a step's view table on the next run and expects the source buffer to still be resident. An earlier draft did try to reclaim view sources once their views died; it turned out to be unreachable (every view source is pinned) and, when measured, worth 36 MiB out of ~12 GB. Not worth a cross-run aliasing analysis. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
justinchuby
force-pushed
the
fix/executor-release-dead-values
branch
from
August 19, 2026 21:31
f1b3797 to
07d947f
Compare
justinchuby
added a commit
that referenced
this pull request
Aug 19, 2026
…ol (#1499) Two independent gaps in how pipeline components were brought up. **The device was never resolved.** A component was loaded without consulting the session options the rest of the engine had already used to pick a device, so `--device cuda:3` put the decoder on GPU 3 and the vision encoder wherever the default landed. `resolved_native_device` now reads the same session options, and routing warns when a component that asked for CUDA fell back to CPU -- previously that was silent, and the only symptom was running two orders of magnitude slower. **The device pool was never accounted.** A component session builds its own execution provider, which sizes a standing pool for itself. Nothing told the memory governor, so the decoder's admission control and the component each measured the same free VRAM and each concluded it could take ~90% of it. Adopting the governor under a new `Holder::PipelineComponentPool` makes that pool a claim the rest of the engine can see. Adoption failure is logged rather than fatal: a provider holding no standing pool legitimately reports zero, and a governor that refuses the claim leaves the component exactly as unaccounted as it was before. Worth saying out loud; not worth failing a load over. Builds on #1498 (which added `set_release_dead_values` on the same `load` path). ## Validation `onnx-genai-engine --features native-backend,cuda`: 575 + others, 0 failed. End-to-end on a 30B INT4 pipeline with `--device cuda:3`, confirmed via `nvidia-smi` that both decoder and vision encoder land on GPU 3. CI runners are still down, so this was validated locally. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1498 +/- ##
===========================================
- Coverage 82.10% 80.25% -1.86%
===========================================
Files 12 377 +365
Lines 5471 166504 +161033
Branches 5471 166504 +161033
===========================================
+ Hits 4492 133622 +129130
- Misses 780 28041 +27261
- Partials 199 4841 +4642
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
justinchuby
added a commit
that referenced
this pull request
Aug 20, 2026
Both are Tier 2 losses: git produced them without a conflict marker, so nothing in the resolution flagged them and no test went red. 1. matmul_nbits.rs was 257 lines short of main, dropping the whole decode_gemv_achieved_bandwidth_by_projection_shape probe from #1574. The stack never touched this file -- its blob is identical to the fork point -- and the raw auto-merge took main's blob correctly. It was damaged afterwards, while reverting what looked like rustfmt drift. Restored to main's blob; fork-point/main/auto-merge/HEAD now agree. 2. load_with_cuda_memory never called set_release_dead_values(true). main added that in #1498 because holding all 2545 vision-encoder node outputs at once cost ~23 GB. The call survived in `load`, its executor implementation survived, and its tests survived -- but both CUDA component call sites (pipeline/mod.rs:538, routing.rs:337) take load_with_cuda_memory, so the fix had no activation left on the path that actually runs. Reference counting cannot see this shape: the count never dropped, only the reachable path changed. Also carried over main's execution-provider fallback warning, and recorded why adopt_memory_governor is deliberately absent here. adopt_memory_governor is intentionally not called: the provider is already governed, so charging the component holder would double-count. Verified: cargo check --workspace --all-targets, plus the cuda,native-backend and cuda,gpu-tests feature sets, all clean. Tests over the 7 memory crates: 1105 passed / 2 failed / 88 ignored, identical to before these changes; both failures are the known macOS statvfs FFI bug in platform_capacity.rs. Neither fix is exercised here -- there is no CUDA on this host and load_with_cuda_memory is cfg-gated. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
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.
The executor already computed
last_usefor every value, but its only consumer wasinplace_dead_inputs, whose conditions (output must match an input in both dtype and shape) in practice cover elementwise chains and nothing else. Any shape-changing operator -- MatMul, Reshape, Attention -- allocated a buffer that then sat resident until the run ended, so a graph is charged for its peak node count rather than its peak live set.The vision encoder made this concrete: all 2545 node outputs held at once, ~23 GB at 448px, and a 960x686 image OOMing an 80 GB card outright.
Two things that are easy to get wrong
DeviceBufferhas noDrop. It is a bare{device, size, align, ptr, owner}handle. Removing it from the value map looks like a release and is actually a leak -- the first version of this change did exactly that and reported 7075 releases while device memory did not move at all. The buffer must go back throughep.deallocate.Why this is per-session opt-in, default off. It is unsafe under device graph capture: capture bakes device pointers into the recorded graph, so freeing a buffer lets a later allocation land elsewhere and replay reads the wrong memory. The decoder fails immediately with
device capture validation violation (flags=0xc0). Restricting the pass to the eager path is not sufficient, because one session runs eager before it captures. So components that never capture (the pipeline components, run once per request) opt in; the decoder does not.Dead values still backing a live view are retired from the view table first and otherwise held in
pending_deadand retried after each node. Measured worth on its own is near zero (55945 -> 55909 MiB; views are not the bottleneck), but it keeps the pass from silently depending on view lifetimes.The release guard list deliberately mirrors the
inplace_inputguards indispatch.rs-- initializers, graph inputs, external in/outputs, pinned, shared buffers, sequence elements, borrowed. Those two lists should be kept in step.Validation
onnx-runtime-session: 186 + 23 + others, 0 failedonnx-genai-engine --features native-backend,cuda: 575 + others, 0 failedCI runners are still down, so this was validated locally.