Repository navigation
Integrate CUDA workspace with activation memory patterns - #32071
Chi Lo (chilo-ms) wants to merge 87 commits into
Conversation
Move workspace reporting out of GetCapabilityForEP and into the completed GraphPartitioner::Partition boundary. Track workspace estimates as pending during capability probing and commit them only for nodes that ORT actually assigns. Replace the generic safety-margin workspace with the MatMulNBits Level-1 estimate before CUDA's budget decision. Preserve workspace estimates across the layout-transformation deferred-commit path, and report non-workspace, workspace, and total estimated memory once per resource-accounted EP. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Separate runtime workspace, persistent prepack buffers, and temporary prepack scratch in Level-1 estimates, and conservatively charge each component during CUDA partitioning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep nodes missing from a supplied stats file at zero cost so profile-based sessions do not fall through to ad-hoc initializer and workspace bookkeeping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add estimator-aware NHWC and CUDA budget coverage, make resource arithmetic checked, clarify per-graph Level-2 reporting, and document deferred plugin estimator parity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1faf9517-5062-4655-a967-3834143334d0
Extract Level-1 runtime workspace into explicitly initialized scalar state so GCC 14 reduced builds do not diagnose nested optional storage as maybe uninitialized. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1faf9517-5062-4655-a967-3834143334d0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1faf9517-5062-4655-a967-3834143334d0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1faf9517-5062-4655-a967-3834143334d0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1faf9517-5062-4655-a967-3834143334d0
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1faf9517-5062-4655-a967-3834143334d0
Document that Level-1 prepack estimates include only allocations additional to already-accounted initializers, and distinguish sequential MatMulNBits tactic profiling from PrePack scratch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1faf9517-5062-4655-a967-3834143334d0
Persist accepted per-node workspace reservations through partitioning and compare them with kernel declarations during session finalization. Add an opt-in strict mode for constrained deployments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Add an opt-in sequential static-workspace pilot that reuses one aligned buffer per device and routes MatMulNBits slot zero through it with dynamic fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Use the cached fpA/intB tactic for fixed small-M shapes, while retaining conservative workspace declarations for cache misses and CUTLASS GEMM. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
Compare scratch and planned MatMulNBits execution with a controlled BFCArena strategy and verify that workspace overlap reduces second-run CUDA allocation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cdd38fe6-fcf3-45c2-acea-b8e6206a7839
33bca06 to
f4e45cb
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3258464 to
80b532a
Compare
Trace workspace patterns in training builds, cover oversized cached requests, and reset MatMulNBits instrumentation before early returns. Preserve CUDA plugin and no-fpA builds, Linux internal test exports, tensor-only profiling, and minimal-build size. Keep arena diagnostic outputs unchanged on failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the public frame output API and attach the memory profiler in the new regression. Avoid unsupported workspace validation in minimal builds and unnecessary formatting for literal diagnostic errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the dynamic scratch fallback without unused workspace vtable entries. Match other unsupported experimental APIs by returning not-implemented directly in minimal builds, with documented behavior and output-preservation coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Serialize the complete statistics table through one loop instead of duplicating conversion and insertion at each field. Preserve all 11 keys, including ReservedBytes, and add output-shape regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Final review follow-up is pushed through e964fed. All four review threads have been answered and resolved, and GitHub reports the branch as mergeable (remaining checks still gate merging). The changes address training workspace tracing, stale MatMulNBits usage probes, oversized-request fallback coverage, memory-profiler compatibility, CUDA plugin/no-fpA build issues, Linux internal-test exports, diagnostic output preservation, and benchmark formatting. Minimal builds retain dynamic scratch without unavailable workspace machinery. Allocator statistics retain all 11 fields, including ReservedBytes, using a shared serialization loop to avoid repeated code. Validation on the final source:
These are targeted local results, not a full-suite or CUDA E2E result. The remaining platform jobs are still running. The separate Suggest fixes job failed during Setup Rust before linting; I requested a retry of that infrastructure failure. The PR description now reflects review against main and the complete test-branch implementation, diagnostics, and benchmarks. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved ABI, Windows test-target, API-versioning, and GQA cache issues can break compatibility or workspace reuse.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (6)
Preserve CUDA internal module exports and Windows linkage · New Register new experimental API as SinceV31 · New Append ProviderHost method to preserve vtable compatibility · New Preserve unique CUDA test coverage on Windows · New Keep CUDA unit test module dependency on all platforms · New Cache workspace traces for out-of-bound runs · New
Resolved since last review (3)
Append the workspace bridge method, register arena diagnostics at SinceV31, and trace bounded GQA slots even when a run must use scratch. Add API lookup and out-of-bound-first-run regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Build session-based CUDA tests in the provider-test executable and defer provider-internal calls through a test-only interface after host initialization. Preserve the CUDA test-module dependency on every platform and retain distinct chain regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the six new review threads in 02f4fad and 327f615:
Validation: the rebuilt shared library passed all 10 ExperimentalCApiTest cases. The affected Windows CUDA test/support objects and new GQA regression compiled. A synthetic-DLL smoke check covered deferred loading and selected forwarding paths; a COFF check covered all 26 provider-internal references. No-FPA, no-contrib, and CUDA-minimal support-file compilation checks passed. All 75 moved test declarations are retained (one disambiguated name), and all 266 generated provider-test sources were scanned for duplicate literal test registrations. Formatting and whitespace checks passed. Limitations: these are focused checks, not a full CUDA DLL/executable link or GPU test run. The new GQA runtime regression and restored CUDA session suites still need GPU execution; fresh CI is pending. Each review thread has a specific reply and is resolved. |
|
I think the GQA results point to an important distinction between the admission bound and the runtime allocation policy. Today the integration effectively uses the same value for three different quantities:
That appears consistent with the benchmark data:
The conservative maximum still looks appropriate for partition admission. The concern is using it as the allocation size for every cached pattern, especially when the unfused fallback is reachable and dominates the envelope. A relatively bounded follow-up could separate the quantities without requiring mutable cache replacement:
C cannot simply be the first observed R. The existing memory-pattern key does not include scalar contents or binding/alias facts, so identical feed shapes can see different This would preserve the value of the Level-1 worst case for deterministic partitioning while preventing the normal fixed-capacity decode path from materializing the global unfused envelope. It would also keep the current cache entries immutable. More general guaranteed-success behavior across route changes would require a later pre-materialization variant/key design; it does not seem necessary for the first memory-efficiency improvement. Given the current benchmark results, I would describe GQA's present behavior as conservative workspace placement/accounting rather than an effective per-pattern preallocation optimization. It may be worth either separating W/C/R before enabling GQA by default in this pilot, or explicitly scoping the current GQA path as accounting-only until that follow-up lands. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Benchmark device selection can measure the wrong GPU, no-fpA legacy coverage is missing, and one analysis document contains an unedited transcript.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (3)
Resolved since last review (6)
Append ProviderHost method to preserve vtable compatibility Register new experimental API as SinceV31 Preserve CUDA internal module exports and Windows linkage Cache workspace traces for out-of-bound runs Keep CUDA unit test module dependency on all platforms Preserve unique CUDA test coverage on Windows
| def start(self) -> None: | ||
| command = [ | ||
| "nvidia-smi", | ||
| f"--id={self._device_id}", |
| TEST(MatMulNBitsWorkspace, LegacyWorkspaceUsesChunkForLargeOutputProjection) { | ||
| const auto workspace = ComputeLegacyMatMulNBitsWorkspaceInfo( | ||
| /*n=*/120818, /*k=*/2048, /*block_size=*/32, sizeof(MLFloat16), | ||
| /*column_wise_quantization=*/true, /*has_g_idx=*/false, | ||
| /*force_chunked=*/false, /*chunk_target_rows=*/32768); |
| ## From the copilot cli chat | ||
|
|
||
| after a memory pattern has been recorded for a particular input-shape signature, subsequent executions matching that pattern obtain the legacy dequantization workspace from the preallocated workspace slot instead of calling GetScratchBuffer() . |
Ti-Tai Wang (titaiwangms)
left a comment
There was a problem hiding this comment.
Reviewed at 832850b9508fc458ad7ae72c8012a1aca4af5213, concentrating on production planning/allocation contracts and directly related tests.
Please address the three blocking items below before approval:
- The refactored single-node MatMulNBits builder supplies three inputs, but retained agreement assertions require six positional inputs.
- The GQA regression verifies outputs and a recorded slot, but does not detect unconditional dynamic fallback instead of planned-workspace consumption.
- The unconditional GQA constructor parser rejects explicit
"0", contrary to the already-merged envelope option's unspecified-value contract.
Each is detailed inline. The test-fixture defect and explicit-zero rejection are established from source; I am not attributing the existing CI failures to them. The GQA item is a central-behavior coverage gap, not a claim of incorrect current numerical output.
Non-blocking follow-ups:
- Document first-Run tracing and subsequent run-scoped pattern reuse, the one-slot opt-in restriction, and non-owning workspace lifetime; this is not Initialize-time persistent allocation.
- Consolidate generic framework tracing and use the repository logger rather than duplicated kernel-specific environment switches and once-only stderr output.
- GQA currently uses the worst-case declaration W as cached capacity C; an out-of-envelope fallback can coexist with that materialized block. This is a structural allocation-policy observation, not a measured OOM or performance finding.
- Document or handle partial-training support: bypassing activation-only static caching does not cause
PartialExecuteThePlanto publish a combined pattern, so opted-in partial execution does not get the same reuse as the full executor. - Reconcile the older-base option declaration, estimator eligibility and layout with the merged option and the #33016/#33118 changes before stacking.
Concerns considered but not reported as blockers: No shipped planner-device/alignment mismatch was demonstrated. The public experimental arena API preserves output parameters on failure through local temporaries. The proposed source-extension configure failure was refuted on an inert CMake 4.1.2/Linux/Ninja glob containing .cc/.md/.py: configuration succeeded and only .cc had a compilation rule; the ORT executable wrapper does not override source language. This does not certify a full ORT configure or other generators.
Validation/exclusions: No local ORT build, CUDA test, sanitizer or benchmark was run. Older failing CI logs were unavailable, so their causes remain unattributed. Combined-tree behavior, non-FPA runtime coverage, plugin compatibility, asynchronous lifetimes and arena-destruction concurrency remain unverified rather than implied safe.
| node->set_name(node_name); | ||
| node->add_input(input_name); | ||
| node->add_input(weight_name); | ||
| node->add_input(scales_name); |
There was a problem hiding this comment.
Major: the refactored single-node fixture no longer matches its retained six-input assertions. This helper appends only A, B and scales, and BuildMatMulNBitsModelBytes no longer adds the two optional holes plus bias-or-empty. ResolveNodeInputShapes emits one entry per InputDefs(); graph resolution normalizes argument counts, not the input-definition vector. The assertions at lines 832 and 953 therefore see three entries instead of six, preventing the agreement test from reaching its planned-workspace checks. Please restore [A, B, scales, "", "", bias-or-empty] and the conditional bias initializer in the single-node builder; the chain helper can remain three-input. This is a source-established mismatch in eligible FPA-enabled configurations, not an observed CI-root-cause claim.
| ASSERT_NE(device_pattern, nullptr); | ||
| const auto* block = device_pattern->GetBlock(plan.pattern_id); | ||
| ASSERT_NE(block, nullptr); | ||
| EXPECT_EQ(block->size_, plan.allocation_bytes); |
There was a problem hiding this comment.
Major coverage gap: this does not prove GQA consumes the planned block. The kernel traces the declared slot before deciding whether to discard its pointer and use dynamic scratch. If it unconditionally discarded the pointer, these block-size and numerical-output assertions would still pass. Please add test-only allocation-source observability for the existing {128,64,128,64} sequence: recording/out-of-envelope runs should use dynamic fallback, while cached in-envelope runs must consume planned regions. Retain the numerical oracle. MatMulNBits already has analogous planned-use checks.
|
|
||
| int64_t parsed = 0; | ||
| const auto [end, error] = std::from_chars(value.data(), value.data() + value.size(), parsed); | ||
| ORT_ENFORCE(error == std::errc{} && end == value.data() + value.size() && parsed > 0, |
There was a problem hiding this comment.
Major: explicit zero is valid under the merged option contract but fails here. #33014 defines ep.cuda.gqa_workspace_max_total_sequence_length="0" as unspecified, equivalent to unset. This parser accepts unset but requires parsed > 0 for an explicit value. Because the GQA constructor calls it unconditionally, a valid zero setting now fails Initialize even without workspace-preallocation opt-in. Please reuse the validated nonnegative configuration, or accept parsed >= 0 with zero meaning unspecified, and add an initialization regression covering explicit zero. Also reconcile the older-base duplicate key/documentation on rebase.
|
Closing this broad integration PR for now to avoid coupling review of the staged GQA sizing and estimation work to an outdated combined tree. The unique activation-memory-pattern integration is still valuable and the branch is retained as source material. It will be rebased or extracted into focused follow-up work after #33118, #33016, and the Level-2 declaration plumbing settle. The three current blocking findings are acknowledged and will be addressed in that reconstructed follow-up rather than patched onto this conflicted 64-file/87-commit branch. |



Summary
Integrates opted-in in-tree CUDA kernel workspace with ORT's run-scoped activation memory patterns. Review against
main; this PR includes the implementation, tests, diagnostics, and benchmark updates fromchilo/static-workspace-preallocation-test.Builds on Level-1 workspace estimation/reporting (#31962) and Level-2 reservation verification (#32189). Tracking: #29775.
Configuration and runtime flow
Set
session.enable_static_workspace_preallocation=1. Sequential execution and active memory-pattern optimization are required. For non-windowed CUDA GQA, additionally set the positive boundep.cuda.gqa_workspace_max_total_sequence_length.Review follow-up
Regression coverage
ExecutionFrameTest.WorkspacePatternCachesAndRejectsOversizedRequests: first-run tracing, cached reuse, a 129-byte request against a 128-byte declaration despite larger alignment padding, dynamic fallback, and subsequent reuse.MemPatternPlannerTest.WorkspaceSharesNonOverlappingActivationBlock: activation/workspace offset reuse.See the Checks tab for current platform validation. Historical benchmark measurements and their limitations are recorded in
onnxruntime/test/providers/cuda/workspace_preallocation_benchmark_results.mdandonnxruntime/test/providers/cuda/ort_genai_workspace_benchmark_results.md; they are not blanket performance guarantees.Scope