Repository navigation
Report the weight-offload counters the profile gate already consults - #719
Merged
Merged
Conversation
Both documents described a target architecture in the present tense, which is the complaint that started this line of work: a reader could not tell which parts existed. Today produced enough end-to-end measurement on a model that genuinely does not fit an 8 GB card to say precisely. WEIGHT_OFFLOAD.md gains a section 0 ahead of the design, and its status header stops claiming Phase 1 is cleared to implement without qualification. Section 0 records four things, three of which contradict what a reader would assume: * The memory paths do not compose. Of the four VMM x offload configurations, two failed to load. Every A/B that toggled one of them was therefore comparing a working configuration against a broken one (#704). * On Windows WDDM, `cudaMalloc` silently spills past dedicated VRAM into system RAM while `cuMemCreate` cannot. The baseline "fits" a 15.5 GB model on an 8 GiB card only because the OS is paging GPU memory behind the driver's back, and `cuMemGetInfo` cannot see it. So VRAM-floor comparisons are invalid on this platform, and the arena failing where the baseline succeeds is the arena being correct rather than broken. * Our weight offload is 27x slower than the OS doing it naively -- 0.22 tok/s against 6.01 -- while holding a complete description of the future the OS does not have. The largest known cause was recorded only in a warning about CUDA graph capture (#705). * `--vram-limit` does not limit VRAM. A 2 GB ceiling on a model needing ~15.5 GB still generates (#712). And what does work: prefix reuse (44x TTFT, 0.9886 hit rate on the server), concurrency to 8, and the headline property -- a model that does not fit loads and answers correctly. MEMORY_ARCHITECTURE.md gets the same findings as status rows, plus a new section "The platform is part of the memory system". That last one is the general lesson: several hours went into deriving from source why a 15.5 GB model fit on an 8 GB card, and the answer was a property of Windows, not of this codebase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
The previous revision put a dated "measured behaviour" section at the top of
WEIGHT_OFFLOAD.md. That was the right content in the wrong place: it made the
document read as a status report, and status reports rot. Throughput figures and
issue numbers have a short shelf life; the objective does not.
Section 0 now states the objective first, in one sentence:
One model package runs on any machine that can physically hold one
activation set, and gets faster as the machine gets bigger -- without a
different build, a different file, or a different code path.
Then four properties in priority order, so conflicts have an answer rather than
a debate: capacity degrades into latency and never into failure; the user states
one number and everything else is derived; we must beat the operating system's
own overflow mechanism or honestly recommend against ourselves; one authority
accounts for every byte.
The third is worth stating as a goal rather than leaving implicit. Every platform
already overflows device memory somehow -- WDDM shared memory, UVM, swap. Ours is
only worth having if it is faster, and it should be, because we know the access
schedule in advance and the OS does not.
Section 0.2 lists what would falsify the design, so the north star stays testable
instead of becoming a slogan, and 0.3 records what is deliberately out of scope
-- including that when everything fits, the correct behaviour is to add no
per-token cost at all.
Section 0.4 is the only dated part. It is explicitly marked as a snapshot, is
scored against the four properties rather than as a list of numbers, and links
out to issues instead of restating figures, so stale measurements live in threads
where they can be superseded rather than in the design.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
`record_cuda_offload_counters` computed `page_ins`, `hits` and `evictions`, used all three in the gate deciding whether the weight-offload section was worth printing, and then emitted only the five prefetch counters. The three that actually drive the residency cache were dropped on the floor. The failure mode is worse than a missing row. Because the gate fires when *any* of the eight counters moved, a run whose residency cache was paging hard would print a weight-offload section in which every visible row read `0.00` -- the section appeared, and it said the cache was inert. That is how the cache was read as not engaging at all on a 14b offload run, when the section printing at all proved one of the three hidden counters was non-zero. Emit page-ins, hits and evictions, and derive a hit rate over `hits + page_ins` lookups. The hit rate is omitted rather than reported as zero when the cache was never consulted, so `never looked up` stays distinguishable from `missed on everything`. Split the emit out of the snapshot read as `emit_cuda_offload_counters` so it is testable without a CUDA device, and add a test that walks each of the eight counters, moves it alone, and asserts some emitted row carries its value. Verified it fails on the original bug: dropping the page-ins row reproduces the exact panic message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #719 +/- ##
==========================================
+ Coverage 80.48% 81.05% +0.57%
==========================================
Files 328 331 +3
Lines 125517 132604 +7087
Branches 125517 132604 +7087
==========================================
+ Hits 101024 107485 +6461
- Misses 20318 20878 +560
- Partials 4175 4241 +66
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🔴 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
|
This was referenced Aug 7, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 7, 2026
Consolidating today's findings into the document rather than leaving them in issues. ## The new section: a third failure mode `MEMORY_ARCHITECTURE.md` already recorded two ways this area fails — code that is right, tested and never reached; and measurements that are reproducible but support a conclusion they do not license. Today produced a third, and it cost more than either. **An instrument that runs, reports a plausible number, and measures the wrong thing.** `h2d_enqueue_copy_ms` put host-to-device transfer at **1.7%** of decode step time. That figure retired an entire line of work — prefetch depth, pinned versus pageable staging, transfer overlap — and was published on #705 and #718. The arithmetic gives it away: the same step stages **7.33 GiB**, and 7.33 GiB in 30 ms would be ~250 GB/s, roughly ten times this machine's PCIe link. The counter bracketed an asynchronous enqueue *returning*. The first repair renamed it `copy_wait_ms`, pointing it at `compute_wait_fence` — whose own doc comment reads *"a stream-ordered, **non host-blocking** cross-stream wait."* The same non-measurement survived under a name that invited the wrong reading **more** strongly than before. Measured properly with CUDA events, transfer is **18.8%**. Two habits, both cheap: - **Divide the bytes by the time and ask whether the answer is a real bandwidth.** A number near link speed, near memcpy speed, or ten times either tells you what the counter actually bracketed. This is what caught it, and it takes seconds. - **State on every timing counter exactly what lies between start and stop, and whether anything there blocks the host.** Enforced in `weight_paging.rs` since #715. ## The pattern underneath all three Telemetry that never reaches the operator, three times in one subsystem: documented env vars nothing reads (#688), counters computed and never printed (#719), counters printed with nothing writing them (#715). The #719 case is the sharpest. A residency cache paging at a **0% hit rate** printed a weight-offload section in which **every visible row read `0.00`** — and my first reading was that the cache was inert. The opposite was true and provably so: the section printing at all required one of the three *hidden* counters to be non-zero. The instrument reported the exact inverse of what it measured. After a third instance the response should be a guard, not more vigilance. ## Status table rows where today changed the facts - **`--vram-limit`** — was "does not limit VRAM"; now enforced at load, auto-enabling offload with the weight budget derived as `limit − device state`. Caveat carried rather than closed: activations and runtime overhead are still `unknown` and unsubtracted (#514). PR #717 - **Weight residency cache (CUDA)** — new row. It had a **0% hit rate for its entire life**: 6,936 page-ins, 0 hits, at both 3 GB and 6 GB, because `evict_to_fit` is LRU against a cyclic sequential scan. Fixed to **74.18%** against a `B/W ≈ 76%` ceiling, evictions 6,286 → **0**. #720, PR #723 - **Weight offload vs the OS** — the 27x gap now has a named mechanism instead of a mystery, with ~39% of step time placed downstream of that one defect. - **KV page storage** — new row. `onnx-genai-kv` never touched device memory; the contract is now open (`Box<dyn KvPageStore>`), host-only pending stages 2–5. PR #726 - **Captured graphs across a VMM remap** — new row, verified on hardware and sentinel-proven. PR #727 Docs only. Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
This was referenced Aug 12, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 12, 2026
…804) (#807) ## Why This class of defect produced **two wrong conclusions this week**, both of which cost real work before being caught. Both are the same family: **state that is frozen or warmed once per process, making results depend on test order.** On a measurement-driven subsystem a wrong number does not just fail — it redirects the design. - **#797** — a GPU residency test failed ~50% of the time on a cold context and looked exactly like a correctness bug in growth-under-capture. Root cause: a readback on the **legacy default stream** raced a memset on a created **non-blocking stream** (the two are mutually exempt from implicit synchronization), so the readback returned partial fills **with no CUDA error**. `cuCtxSynchronize` did *not* fix it, which refuted the obvious race theory. It only "passed" because an alphabetically-earlier sibling subtest warmed the context. - **#801 / #804** — `captures=0` was reported as the decoder structurally declining CUDA graph capture, and that attribution reached a merged PR body. Root cause (#804): the capture-OFF phase set `ONNX_GENAI_CUDA_GRAPH=0` before the process-wide `RuntimeConfig` `OnceLock` was first read, and that config is frozen on first read — so every later phase in the same process inherited it. #794 and #801 failed to reproduce an end-to-end measurement and misattributed the reason. This is the same telemetry-that-misleads lineage as #719 / #715 (documented in `MEMORY_ARCHITECTURE.md`'s "third failure mode"). After the second instance in a week, the response is a **guard**, not more vigilance. This PR is **test infrastructure + documentation only**. No production behavior changes. ## What ### 1. A guard for process-frozen configuration (`onnx-genai-runtime-config`) `runtime_config()` records the environment it froze the `RuntimeConfig` snapshot from (captured through the same lookup `from_env` uses, so the set can never drift from the parsed fields). On every later call it re-reads those variables and **panics loudly** — with the frozen and current values, and a pointer to #804 — if any now differs. A test can no longer silently believe it set a knob that was already frozen. - **Gated on `debug_assertions`, not `cfg(test)`.** The #804 mutation lives in an *integration* test of a downstream crate, where `cfg(test)` for this crate is never set. `debug_assertions` is enabled for dependencies under the `dev`/`test` profile, so the guard is live for every `cargo test` in the workspace, and it is compiled out of `--release` — **the production path is byte-for-byte unchanged and pays nothing** (a single `OnceLock` read). - The comparison is a pure `find_changed()` helper with unit tests for the unchanged, changed, newly-set (the #804 shape), and cleared cases. ### 2. A helper for stream-correct device test code (`onnx-runtime-cuda-memory::test_support::TestStream`) A shared, test-only (`#[cfg(any(test, feature = "gpu-tests"))]`) RAII wrapper around **one** non-blocking stream. Every device op a test needs — `fill`, `write`, `read`, plus `raw()` for graph launches/memsets — flows through that stream, so a single `cuStreamSynchronize` is a total order. The module doc explains the legacy-default-vs-non-blocking exemption **at the call site** so the next author does not rediscover #797 the hard way. `vmm_kv_layout_residency_gpu.rs` (the exact #797 site) is converted to it and its four local stream helpers removed; **8/8 reliable in isolation** and green in the full single-threaded suite. ### 3. The pattern, documented where it will be read (`docs/MEMORY_ARCHITECTURE.md`) A tight "**a fourth failure mode: order-dependent test state**" subsection next to the existing third-failure-mode material: both case studies, the two mechanisms (frozen process config; context warmed by an earlier sibling), the two remedies (process isolation for anything that reads process-frozen config; single-stream discipline for device tests), and the one-line discipline that names the defect — *run any test you touch in isolation as well as in the full suite*. Cites #797/#801/#804/#794/#719/#715 and links rather than duplicating (#791 consolidated this doc to stop drift). ## Inventory of other order-dependent tests found (not fixed here) Per scope discipline, these are **listed, not converted**. The `TestStream` helper and the config guard make converting them mechanical follow-ups. ### Device tests that mix a created stream with default-stream copies (#797 hazard) Each creates a non-blocking stream **and** issues default-stream `cuMemcpyHtoD_v2`/`cuMemcpyDtoH_v2` copies. Where those touch the same buffers as stream-ordered work on a cold context, they are exposed to the exact #797 race. They should migrate to `test_support::TestStream`: - `crates/onnx-runtime-cuda-memory/tests/vmm_commit_shared_prefix_gpu.rs` - `crates/onnx-runtime-cuda-memory/tests/vmm_dummy_page_gpu.rs` - `crates/onnx-runtime-cuda-memory/tests/vmm_dummy_write_protect_gpu.rs` - `crates/onnx-runtime-cuda-memory/tests/vmm_granularity_gpu.rs` - `crates/onnx-runtime-cuda-memory/tests/vmm_graph_remap_gpu.rs` - `crates/onnx-runtime-cuda-memory/tests/vmm_kv_contiguous_tail_gpu.rs` - `crates/onnx-runtime-cuda-memory/tests/vmm_prefix_share_gpu.rs` - `crates/onnx-runtime-cuda-memory/tests/vmm_prefix_share_write_protect_gpu.rs` - `crates/onnx-runtime-cuda-memory/tests/vmm_stable_va_weight_slot_gpu.rs` (Non-vmm siblings `virtual_memory_gpu.rs` and `physical_pool_production_gpu.rs` also use default-stream copies; they pass but are the same latent shape.) ### Tests that mutate a `RuntimeConfig`-feeding env var within a process These change a variable that feeds the frozen snapshot (`ONNX_GENAI_CUDA_GRAPH`, `ONNX_GENAI_EP`) across phases. The correct pattern is **child-process-per-phase**; the new guard will now fire loudly on any that mutate in-process and then read `runtime_config()`: - `crates/onnx-genai-engine/tests/mobius_seqmajor_growth_parity_native_cuda.rs` — **already correct** (spawns a child per mode; its own doc comment states `ONNX_GENAI_CUDA_GRAPH` is parsed once). This is the reference pattern. - `crates/onnx-genai-engine/tests/stable_va_capture_offload_e2e.rs` — sets `ONNX_GENAI_CUDA_GRAPH` per capture phase. - `crates/onnx-genai-engine/tests/gemma4_e2b_native_cuda_pipeline_bench.rs` — sets `ONNX_GENAI_CUDA_GRAPH` to `1` then `0`. - `crates/onnx-genai-engine/tests/qwen2_5_1_5b_divergence.rs` — switches `ONNX_GENAI_EP` between `cpu` and `cuda`. - `crates/onnx-genai-engine/tests/qwen3_0_6b_capture_step_inputs_decline.rs` and `gemma3n_native_cuda_capture_realmodel.rs` — toggle the native-decoder capture env per phase. Session-crate tests that toggle env (`ONNX_GENAI_DECODE_MEMO`, `ONNX_GENAI_SCAN_INLINE_SINGLE_TRIP`, `ONNX_RT_SIBLING_MERGE`, the EP-CPU weight-offload knob) read those variables **directly at their call sites**, not through `RuntimeConfig`, so they are *not* affected by the freeze guard and the full `onnx-runtime-session` suite stays green. ## Validation (all local; not waiting for CI) - `cargo fmt -p onnx-genai-runtime-config -p onnx-runtime-cuda-memory -- --check` ✅ (`--all` hits the Windows `os error 206` long-command artifact, not a formatting issue) - `cargo clippy --locked --all-targets -p onnx-genai-engine --features native-backend -- -D warnings` ✅ - `cargo clippy -p onnx-runtime-cuda-memory --features gpu-tests --all-targets -- -D warnings` ✅ - `cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings` ✅ - `cargo test -p onnx-runtime-session` ✅ (full suite) - `cargo test -p onnx-runtime-cuda-memory --features gpu-tests -- --test-threads=1` ✅ (all vmm_*/gpu suites, RTX 4060) - The converted `seq_major_fixed_stride_grows_under_captured_replay_without_recapture` run **8× in isolation**: 8/8 pass — the property this PR is about. ## Notes - Kept strictly to test infra + the docs subsection so the two other active workstreams (GQA pooled QKV staging in `group_query_attention.rs`; head-major vs seq-major re-capture measurement) merge cleanly — no overlap. - Does **not** merge and does **not** close any issue. 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 bug
record_cuda_offload_counterscomputes eight counters, uses all eight in the gate that decides whether the weight-offload section is worth printing, and then emits five of them.page_ins,hitsandevictions— the three that describe what the residency cache actually did — were computed and dropped.Why this is worse than a missing row
The gate fires when any of the eight moved. So a run whose residency cache is paging hard prints a weight-offload section in which every visible row reads
0.00. The section appears, and it says the cache is inert.That is not hypothetical — it is how I read this output from a 14b offload run today:
weight offload prefetch issued 0.00 prefetches weight offload prefetch declined guard 0.00 prefetches weight offload prefetch joined 0.00 prefetches weight offload prefetch staging allocs 0.00 allocs weight offload prefetch staging reuses 0.00 reusesand concluded the device residency cache was not engaging at all. The opposite is true: the section printing at all proves one of the three hidden counters was non-zero, because all five printed ones are zero. The instrument reported the exact inverse of what it measured.
The fix
page-ins,cache hitsandevictions.hits + page_inslookups. Omitted — not reported as0%— when the cache was never consulted, so never looked up stays distinguishable from missed on everything.emit_cuda_offload_countersso it is testable without a CUDA device; the device is only needed to sourceafter.Test
every_counter_the_offload_gate_consults_is_also_emittedwalks each of the eight counters, moves that one alone, and asserts some emitted row carries its value.Verified it catches the original bug. Replacing the page-ins emit with
let _ = page_ins;fails with exactly:page_ins made the offload section print, but no emitted row carries its value — the counter is invisible to the operatorTwo further tests cover the hit-rate numerator/denominator and the never-consulted case.
Validation
cargo fmt --all -- --checkcleancargo test -p onnx-genai-cli --lib generate::— 5 passedWhy it matters beyond the row
Hit rate is the number needed to settle whether the eviction policy is fit for the access pattern. The weight walk during decode is a cyclic sequential scan over the layers, and
evict_to_fitis a plain LRU evicting from the least-recently-used end — the textbook pessimal pairing, which yields a hit rate near zero because the scan wraps back to exactly the page that was evicted first. I could not test that hypothesis because the hit rate was unobservable. Now it is one line of--profileoutput. That investigation is separate from this PR.This is the same shape as #688 and #690: telemetry that exists, compiles, and does not reach the operator.