Repository navigation
fix(weight-offload): make async page-in opt-in (default sync) - #572
Conversation
Async fence-ordered device page-in net-regresses in the eviction/thrash regime: measured A/B on qwen3-0.6b-int4 (96MiB device budget) shows sync 15.84 tok/s vs async 12.16 tok/s (~1.30x). The tax is materialize- + pinned-staging-bound (~48% each); H2D is ~3%, so there is nothing for async to overlap when every admit evicts and the eviction drain re-serializes. Async only adds a non-overlappable pinned-staging alloc. Invert ONNX_GENAI_WEIGHT_OFFLOAD_ASYNC_PAGEIN to opt-IN: unset/falsey => sync (new default), truthy (1/true/yes/on) => async. The async path is kept fully intact behind the flag (A/B arm; net win once a warm-host materialize cache lands). Only the default changes. Byte-exact preserved (weight_paging §9): offloaded == resident verified on native CUDA e2e with the new sync default (page_ins=12544, evictions=12541, token-identical). WAR/eviction-drain and fence-ordering primitives untouched; the async fence anti-regression GPU test still guards the async path. No capture/GAP-3 interaction. Adds a pure opt-in parse helper + unit test (mirrors #571 style), extends the default-policy test, asserts the default-sync policy in the e2e, and documents the new semantics in WEIGHT_OFFLOAD.md. #544 lineage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #572 +/- ##
=======================================
Coverage 80.74% 80.74%
=======================================
Files 315 315
Lines 123688 123688
Branches 123688 123688
=======================================
Hits 99876 99876
Misses 19745 19745
Partials 4067 4067
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
VERDICT: APPROVE Independent review by Harry (detached at PR head ae116fd, GPU CUDA_VISIBLE_DEVICES=6). This is a low-risk, data-backed DEFAULT flip: async fence-ordered device page-in becomes opt-IN; sync is the new default. I independently confirmed it changes only a default, preserves byte-identical offload output, and keeps the async path fully guarded. 1. Diff scope — clean (4 files, +137/-17)Behavioral change is limited to the async-page-in default:
No edits to the async transfer machinery: upload/upload_async, admit/eviction compute-stream drain, fence-ordering (compute_wait_fence/record_copy_fence), WAR safety, capture, or GAP-3 are all untouched. provider.rs with_async_pagein wiring is unchanged (transfer path intact, just no longer default). No out-of-scope edits found. 2. Flag helper correct + NON-VACUOUS (mutation-proven)
3. Byte-identical offload parity under the NEW sync default (key regression guard) — PASSReal-GPU e2e offloaded_native_cuda_decode_is_token_identical_and_pages (qwen3-0.6b-int4, native CUDA, 2 MiB device budget):
4. Async path still guarded — PASSprovider::tests::async_pagein_fence_orders_weight_page_in_consumer ... ok, run with ONNX_GENAI_WEIGHT_OFFLOAD_ASYNC_PAGEIN=1. The fence-ordering anti-regression test drives the async primitives directly and remains correct; the async path is preserved and tested, just opt-in. 5. fmt — cleancargo fmt --all --check: exit 0. Summary of re-run results
Byte-identical parity is proven and the async guard still runs. Approving. |
🔴 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
|
…gs) (#575) ## CI hotfix — make `DeviceOffloadPolicy` `Default` derivable (clippy `-D warnings`) **Unblocks origin/main** — the "CUDA compile (Linux+Windows)" job is RED on every PR (including #574) due to a `clippy::derivable_impls` failure under `-D warnings`. ### Root cause #572 flipped `DeviceOffloadPolicy::async_pagein`'s default to `false`. That made the hand-written `impl Default` fully equal to a derivable default (all three fields = their type default: `bool` → `false`, `Option` → `None`), so `clippy::derivable_impls` now fires at `crates/onnx-runtime-ep-cuda/src/weight_paging.rs:117`. ### Fix (provably behavior-identical) Replace the hand-written `impl Default for DeviceOffloadPolicy` with `#[derive(Default)]` on the struct. The derive produces exactly the same defaults — `enabled: false`, `device_budget_bytes: None`, `async_pagein: false`. `from_env()` and all runtime behavior unchanged; field doc comments preserved. Net diff: **1 insertion, 11 deletions**, one file. ### Green confirmation - `cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings` → **exit 0** (the failing gate; no other `derivable_impls` / `-D warnings` hits in the crate). - CI first step `cargo check --locked -p onnx-runtime-ep-cuda -p onnx-runtime-python --features onnx-runtime-python/cuda` → **exit 0**. - `cargo fmt --all --check` → clean. - Existing guard test `weight_paging::tests::device_policy_defaults_to_disabled` (asserts `enabled=false`, `device_budget_bytes=None`, `async_pagein=false`) → **passes**, locking the derived defaults against field default-drift. No new test needed. Decision note: `.squad/decisions/inbox/cohaagen-cuda-clippy-hotfix.md`. Trivial lint hotfix — fast-track to unblock main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
What
Flip
ONNX_GENAI_WEIGHT_OFFLOAD_ASYNC_PAGEINfrom 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):
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)
weight_offload_native_cuda_e2ewith the NEW sync default — tokens byte-identical to resident baseline, page_ins=12544, evictions=12541 (non-vacuous).async_pagein_fence_orders_weight_page_in_consumer) still passes and still guards the async path.Tests
async_pagein_env_is_opt_in:None -> false, truthy spellings -> true, falsey/garbage -> false (non-vacuous both directions).device_policy_defaults_to_disabledextended to assert the default policy is sync.from_env()policy is sync by default AND offloaded == resident on real int4 GPU.cargo fmt --all --checkclean.Blast radius
Flag default + tests + docs only. Pager internals, capture (#571), and GAP-3 untouched. Escape hatch:
ONNX_GENAI_WEIGHT_OFFLOAD_ASYNC_PAGEIN=1restores async.