Skip to content

spike(#82): whole-tensor-granularity zero-copy cold-expert QMoE evidence (EXPERIMENTAL) - #1804

Merged
justinchuby merged 4 commits into
mainfrom
squad/82-a-prime-cold-expert-spike
Aug 23, 2026
Merged

justinchuby merged 4 commits into
mainfrom
squad/82-a-prime-cold-expert-spike

Conversation

@justinchuby

@justinchuby justinchuby commented Aug 23, 2026 •

Copy link
Copy Markdown
Owner

Summary

Working as Deckard (CUDA/Perf). This is a bounded, EXPERIMENTAL evidence-gathering spike, not production enablement — it directly answers Roy's cycle-7 downgrade of Option A' (per-expert hot/cold residency) from "accepted architecture" to "provisional hypothesis pending a feasibility + measurement spike" (.squad/decisions/inbox/roy-a-prime-downgraded-provisional-cycle7.md), which explicitly named Deckard as owner of spikes #1 (mixed-backing feasibility) and #2 (MoE-specific measurement).

No production code changed. New file only: crates/onnx-runtime-ep-cuda/tests/qmoe_zero_copy_cold_expert_spike.rs, two #[ignore]d tests. No new allocator/cache, no kernel ABI change, no D2H sync route change. Every kernel-visible pointer is either a real VRAM allocation or the real HostMapRegistry-equivalent zero-copy device pointer (cuMemHostRegister(DEVICEMAP|READ_ONLY) + cuMemHostGetDevicePointer — byte-identical primitive to what weight_paging.rs already uses); nothing here is wired into CudaWeightResidency's live dispatch path.

Spike #1 — mixed-backing feasibility: CONFIRMED NO (re-derivation, not a new finding)

bind_zero_copy/zero_copy_device_ptr require one whole LazyWeight to resolve to one contiguous device pointer (non-contiguous → hard fallback to VRAM copy). QMoE's kernels take exactly one base pointer per weight tensor. There is no shipped path for intra-tensor per-expert splitting. Needs either a new composable VMM granule design or a per-expert pointer table (real ABI change, converges toward Option C).

Spike #2 — MoE-specific measurement: DONE

What is buildable today unchanged: whole-weight-tensor granularity residency (fc1/fc2/fc3 independently VRAM-resident or zero-copy), measured against a real QMoE int4 strided GEMV kernel dispatch (not sequential memcpy), on a shape-faithful DeepSeek-V2-Lite-cited fixture (64 and 256 experts, hidden=2048, inter=1408, top_k=6), decode shape M=1.

Platform: 3x independent A100-SXM4-80GB (idle-verified via nvidia-smi --query-compute-apps before every run), driver 580.105.08, CUDA 13.0, Linux.

Correctness: bit-identical (0 mismatches, f32::to_bits() exact) for every zero-copy arm vs. all-VRAM oracle — all_zero_copy, mixed_fc1_cold, and 3x repeated fresh bind/unbind cycles — across both shapes, on 3 separate GPUs. Falsifiability control confirmed the host/device memory-type probe is genuinely discriminating, not a rubber stamp.

Bandwidth (median of 9 GPU-timed reps/arm):

arm time/step achieved GB/s vs all_vram
all_vram ~155-159 us — 1.0x
all_zero_copy (fc1+fc2+fc3 cold) ~1190-1220 us 32.2-32.7 7.6-7.7x slower
mixed (fc1-only cold) ~474-491 us 26.5-27.4 3.1x slower

Consistent across 64/256-expert shapes and 3 GPUs (<5% variance). Compared against PCIe Gen4 x16 (~25 GB/s) and A100 HBM2e (2039 GB/s) rooflines — achieved GB/s sits at/slightly above the naive PCIe estimate, most plausibly host page-cache/TLB locality at this small per-step touched-byte count (~1.3-5.2 MiB/step at top_k≤6), not evidence exceeding true cold-read PCIe bandwidth. This number is NOT the #925 dense-streaming 6.795 GB/s figure and must not be conflated with it (different access pattern, per cycle-7's own correction).

GO/NO-GO verdict: NO-GO for whole-tensor-granularity A' as a production per-expert cold path

  1. Spike First milestone #1 confirms the actual A' proposal (intra-tensor per-expert splitting) isn't buildable without new work.
  2. Even coarse whole-tensor granularity, while correctness-clean, costs 3-8x per touched tensor at this shape/cadence — too slow for a "hot" path needed every decode step.

Next architecture gate: decide between (a) new composable VMM granule design, or (b) per-expert device pointer table (real ABI change) — evaluate (b) head-to-head against Option C rather than treating it as a lighter A' variant. This harness/fixture is reusable for that follow-up spike.

Reproduce

CUDA_VISIBLE_DEVICES=<idle gpu> cargo test -p onnx-runtime-ep-cuda \
  --features cuda --release --test qmoe_zero_copy_cold_expert_spike \
  -- --ignored --nocapture --test-threads=1

Full write-up: .squad/decisions/inbox/deckard-a-prime-spike-results-cycle7.md.

Closes #82 (partially — spikes #1/#2 fully; spike #3 resize-starvation is partially answered by the sequential, single-threaded 1000-cycle stress test added in a later commit on this branch (no starvation/counter drift under back-to-back decode-cadence load); concurrent/multi-stream starvation is explicitly not covered and remains a separate follow-up).

Per user directive: local validation only, not waiting on CI. Independent review requested before merge/close of this experimental branch.

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

justinchuby and others added 2 commits August 23, 2026 03:34
…nce gathering

EXPERIMENTAL, bounded evidence-gathering spike answering Roy's cycle-7
downgrade of Option A' (per-expert VRAM/zero-copy hybrid residency) from
'accepted architecture' to 'provisional hypothesis pending a feasibility +
measurement spike'.

Adds crates/onnx-runtime-ep-cuda/tests/qmoe_zero_copy_cold_expert_spike.rs,
two #[ignore]d tests that:

- Confirm in code (module doc) that CudaWeightResidency::bind_zero_copy is
  whole-weight-tensor-exclusive: no shipped mechanism supports intra-tensor
  per-expert hot/cold splitting without either a new composable VMM granule
  design or a per-expert pointer table (ABI change). This spike therefore
  tests exactly the granularity the shipped mechanism supports today:
  whole weight tensors (fc1/fc2/fc3) independently VRAM-resident or
  zero-copy host-mapped, reusing the identical cuMemHostRegister
  (DEVICEMAP|READ_ONLY) + cuMemHostGetDevicePointer primitive
  HostMapRegistry uses, applied to a real QMoE int4 GEMV kernel dispatch
  (not a sequential-memcpy bandwidth proxy).
- Run 4 controls (all_vram, all_zero_copy, mixed fc1-cold, and a
  falsifiability check that the host/device memory-type probe is
  discriminating, not vacuous) plus a repeated-bind/unbind correctness
  stress, on a shape-faithful DeepSeek-V2-Lite-cited QMoE fixture (64 and
  256 experts, hidden=2048, inter=1408, top_k=6) at decode shape (M=1).
- Assert bit-identical output for every zero-copy arm vs. the all-VRAM
  oracle across 2 shapes x >=4 arms x 3 repeats, on 3 separate idle A100s.
- Measure achieved cold-path GB/s (median of 9 GPU-timed reps) against
  the PCIe Gen4 x16 (~25 GB/s) and A100 HBM2e (2039 GB/s) rooflines,
  explicitly NOT reusing #925's dense-streaming 6.795 GB/s figure (that
  number is a different access pattern per cycle-7's own finding).

No production code changed: no new allocator/cache, no kernel ABI change,
no D2H sync route. Every kernel-visible pointer is either a real VRAM
allocation or the real HostMapRegistry-equivalent zero-copy device
pointer; nothing here is wired into CudaWeightResidency's live dispatch
path.

Results (3x on idle A100-SXM4-80GB, driver 580.105.08, CUDA 13.0, Linux):
correctness bit-identical on every arm/repeat (0 mismatches). Cold-path
GEMV bandwidth measured ~32.2-32.7 GB/s (all-tensors-cold arm) and
~26.5-27.4 GB/s (fc1-only-cold mixed arm), both close to but above the
naive PCIe Gen4 x16 estimate, consistent with driver page-cache/TLB
locality on a repeatedly-touched region at this small per-step byte count
(~1.3-5.2 MiB/step touched at top_k<=6). all_cold arm is 7.6-7.7x slower
than all_vram at this shape; mixed (fc1-only-cold) is ~3.1x slower.

Full report with GO/NO-GO verdict and next architecture gate recorded in
the PR description; the underlying feasibility finding is also recorded
to .squad/decisions/inbox/ per Squad conventions.

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

codecov Bot commented Aug 23, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.30%. Comparing base (def652d) to head (82978c9).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1804      +/-   ##
==========================================
- Coverage   80.78%   80.30%   -0.48%     
==========================================
  Files         411      411              
  Lines      200334   200604     +270     
  Branches   200334   200604     +270     
==========================================
- Hits       161835   161100     -735     
- Misses      33035    34034     +999     
- Partials     5464     5470       +6     
Flag Coverage Δ
cli-ort-linux 72.47% <ø> (ø)
cli-ort-windows 72.06% <ø> (ø)
mlas 85.20% <ø> (-0.14%) ⬇️
offline 80.44% <ø> (-0.50%) ⬇️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…-starvation, stale-figure disclaimers)

Adds to the same EXPERIMENTAL spike test file:
- QWEN15_MOE_A27B fixture shape (experts=60, hidden=2048, inter=1408,
  top_k=4, cited from Qwen1.5-MoE-A2.7B's published config.json) and a
  new #[ignore]d test exercising it, since prior architecture notes name
  this exact fixture ("qwen15-moe-qmoe") and it had not been measured
  directly before.
- A deterministic "real cold bytes/step, measured from fixture, NOT
  modeled" print emitted before any control run or timing number, for
  every shape, per measurement-discipline's bytes/counters-first rule.
- A new resize-starvation stress test
  (qmoe_routed_residency_guard_resize_starvation_stress_1000_steps)
  driving the real CudaWeightResidency::acquire_routed_residency /
  resize_safe_point / execute_resize machinery through 1000 back-to-back
  acquire-hold-release cycles simulating sequential decode-step dispatch:
  every resize attempted while a guard is held is confirmed rejected
  (NotSafePoint), every resize attempted in the inter-step gap is
  confirmed accepted and executed -- no starvation, no guard-count drift
  over 1000 cycles. Explicitly scoped: single-threaded/sequential only;
  concurrent-stream starvation remains a separate, larger follow-up spike.

No new allocator/cache, no kernel ABI change, no sub-weight VMM/host-map
composition attempted or built -- per the reviewing instruction, this
spike remains bounded to what today's whole-tensor-granularity zero-copy
mechanism and existing RoutedResidencyGuard/resize machinery already
support. Verdict unchanged: NO-GO for whole-tensor-granularity A' as a
production per-expert cold path.

Correctness: still bit-identical on every arm across all 3 shapes, 2 more
independent idle-A100 runs (GPU 5 and GPU 6). Bandwidth close to and
consistent with the original 2-shape run (~31.5-35.6 GB/s all-cold,
~25.8-29.1 GB/s mixed fc1-cold across all 3 shapes) -- these are freshly
measured on A100/Linux/real-QMoE-GEMV and are NOT #880's RTX4060/WDDM
5.6 GB/s figure nor #925's H200/Linux dense-qwen14b 6.795 GB ceiling;
neither prior figure is reused or blended into these numbers.

Full updated report in
.squad/decisions/inbox/deckard-a-prime-spike-results-cycle7.md, including
the correction that Mobius #550/#555 now close the DeepSeek-V4/GLM-5.2
exporter prerequisite (artifacts still absent from this box).

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

Copy link
Copy Markdown
Owner Author

Update: Fact Checker hard gates addressed (same branch/PR)

Four gates were added by Fact Checker review after the initial spike; all four are now addressed with new code + measurements in this branch, no scope expansion:

  1. No mixed per-expert VRAM+host composition claimed/built — re-confirmed nothing here composes sub-tensor addressability; all arms stay whole-tensor granularity.
  2. Stale-figure disclaimer strengthened: test(cuda): real-kernel zero-copy vs VRAM weight-read bandwidth (#864) #880's ~5.6 GB/s is confirmed (re-read directly) RTX 4060 Laptop/WDDM-only; Re-measure the #864 zero-copy hybrid on Linux: the #912 negative is WDDM-scoped and does not transfer #925's ~6.795 GB ceiling is confirmed dense Qwen2.5-14B on H200/Linux, not MoE/A100. Neither is reused — this spike's ~31.5-35.6 GB/s (all-cold) / ~25.8-29.1 GB/s (mixed) figures are freshly measured here, A100/Linux, real QMoE GEMV.
  3. Resize-starvation stress added: new test qmoe_routed_residency_guard_resize_starvation_stress_1000_steps drives the real acquire_routed_residency/resize_safe_point/execute_resize machinery through 1000 back-to-back acquire→resize-attempt(rejected)→release→resize-attempt(accepted+executed) cycles. Result: 1000/1000 clean, no starvation, no counter drift, under sequential single-threaded decode-cadence load. Concurrent/multi-stream starvation explicitly NOT covered — flagged as a separate follow-up spike.
  4. qwen15-moe-qmoe real cold-bytes/step now measured directly: new fixture QWEN15_MOE_A27B (experts=60, hidden=2048, inter=1408, top_k=4, cited from Qwen1.5-MoE-A2.7B's published config.json) added as a third shape. All three shapes now print a measured (not modeled) real-cold-bytes/step line before any control/timing. At this shape: 25,952,256 B (24.75 MiB)/decode-step if all top-4 touched experts were cold; bandwidth ~31.5 GB/s all-cold, ~25.8 GB/s mixed — consistent with the other two shapes.

Correction: Mobius #550 now emits DeepSeek-V4 as one QMoE and #555 emits GLM-5.2 via shared QMoE — exporter prerequisite is closed (full downloadable artifacts still absent from this box). Noted for a future spike; does not change this one's fixtures or verdict.

Verdict unchanged: NO-GO for whole-tensor-granularity A' as a production per-expert cold path. No new sub-weight VMM/host-map composition was attempted — per instruction, that would require stopping and reporting NO-GO rather than expanding scope, which is exactly what happened: all four gates were satisfied using only the existing whole-tensor-granularity zero-copy mechanism and existing (unmodified) RoutedResidencyGuard/resize machinery.

Full write-up: .squad/decisions/inbox/deckard-a-prime-spike-results-cycle7.md (updated in place).

Reproduce:

CUDA_VISIBLE_DEVICES=<idle gpu> cargo test -p onnx-runtime-ep-cuda \
  --features cuda --release --test qmoe_zero_copy_cold_expert_spike \
  -- --ignored --nocapture --test-threads=1

@justinchuby

Copy link
Copy Markdown
Owner Author

Roy — independent read-only review of PR #1804 (Deckard's A′ spike #1/#2 + partial #3)

Verdict: REJECT WITH REVISION — narrow, mechanical revision only. Owner: Batty (Engine Dev, CUDA & Perf front). Deckard is locked out of this artifact per reviewer-protocol (original author).

Reviewed the actual PR diff (git diff origin/main...HEAD against merge-base def652d7, cross-checked against gh pr view --json files — confirmed exactly 2 files / 1624 insertions / 0 deletions, no production code touched), the full decision doc, and the full 1491-line test file line-by-line in a disposable worktree. No edits made.

What's genuinely sound (independently re-verified, not taken on faith)

  • Real primitives, not stubs. HostRegisteredRegion uses the exact cuMemHostRegister_v2(DEVICEMAP|READ_ONLY) + cuMemHostGetDevicePointer_v2 pair HostMapRegistry (weight_paging.rs) uses; unregisters correctly on every Drop path this file actually exercises. QMoE execution goes through the real public QMoEKernel::execute() — a real int4 GEMV kernel dispatch, not synthetic memcpy.
  • Correctness control is non-vacuous. assert_bit_identical does exact f32::to_bits() equality (0 tolerance) between an all-VRAM oracle and each zero-copy arm, both reading the same source Vec<u8> bytes (one via htod copy, one via direct host-map) — a real comparison, not tautological.
  • Falsifiability control is real. pointer_is_host_memory queries cuPointerGetAttribute(CU_POINTER_ATTRIBUTE_MEMORY_TYPE) and is hard-asserted per arm to match the requested residency — this would catch a silently-elided zero-copy bind.
  • Timing is GPU-event-based (event::record/event::synchronize/event::elapsed), warm-up excluded, median-of-9, consistent with sibling qmoe_gpu.rs methodology.
  • Byte arithmetic independently re-derived and confirmed exact. Recomputed from the fixture-construction code from scratch: Qwen1.5-MoE-A2.7B per-expert gate/up/down = 2,162,688 B each, total cold bytes/step = 25,952,256 B (24.75 MiB) — matches the decision doc's cited numbers exactly.
  • Shape citation verified against the live source. Fetched https://huggingface.co/Qwen/Qwen1.5-MoE-A2.7B/raw/main/config.json directly: hidden_size=2048, moe_intermediate_size=1408, num_experts=60, num_experts_per_tok=4 — exact match to the QWEN15_MOE_A27B const. Not fabricated.
  • Resize-starvation stress is real, not mocked. Drives the actual CudaWeightResidency::acquire_routed_residency/resize_safe_point/plan_resize/execute_resize/LedgerGovernor machinery (same catalog-construction pattern as an existing unit test), 1000 back-to-back cycles, strict assertions that would panic on either a correctness hazard (resize succeeding while a guard is held) or a starvation regression. Honestly self-disclosed as single-threaded/sequential-only — does not cover concurrent/multi-stream contention. Accurately described, not overclaimed.
  • Mixed-backing (spike First milestone #1) re-derivation and the whole-tensor-granularity bandwidth measurement (spike Refactor: Split server/lib.rs into modules #2, ~26-36 GB/s cold vs ~112-160 us/step all-VRAM baseline, 3-8x slowdown) match what's in the code; correctly not conflated with test(cuda): real-kernel zero-copy vs VRAM weight-read bandwidth (#864) #880 (WDDM) or Re-measure the #864 zero-copy hybrid on Linux: the #912 negative is WDDM-scoped and does not transfer #925 (H200 dense) figures.

Blocking findings (concrete, verified, actionable)

A. The test file's name breaks this repo's own CUDA-test-honesty enforcement. .github/scripts/verify_cuda_test_honesty.py's is_cuda_test_target() classifies a file as a CUDA integration-test target only by an exact _gpu filename suffix (or a 1-entry allowlist that does not include this file). Checked every one of the 51 files in crates/onnx-runtime-ep-cuda/tests/: qmoe_zero_copy_cold_expert_spike.rs is the only GPU-hardware-requiring, #[ignore]d file that does not end in _gpu. Consequence: its 4 tests are silently excluded from the CI job whose entire purpose is guaranteeing "CUDA tests must not hide" / "ignored, not passed" per target. This won't break CI (the script just never sees these targets — confirmed by reading its filtering logic), but it means the durable-regression/evidence property this review is explicitly asked to weigh ("MERGE TEST HARNESS") is not backed by the repo's own safety net for these 4 tests. Fix is mechanical: rename to qmoe_zero_copy_cold_expert_spike_gpu.rs, update the 2 in-file repro-command doc-comments and the decision doc's "Reproduce" section, and confirm python .github/scripts/verify_cuda_test_honesty.py now enumerates all 4 tests.

B. PR body and the decision doc's own header are stale relative to the final commit. The GitHub PR description says spike #3 (resize-starvation) is "out of scope here." The decision doc's own top "Scope" line repeats the identical claim. But the final commit (a6f27d5ef) adds a real resize-starvation stress test and a full "Fact Checker gate response" section that partially (sequential-only) answers spike #3 — directly contradicting both the PR description and the doc's own header. Anyone reading either the GitHub PR page or just the top of the decision doc would materially misunderstand what this PR delivers. Fix: sync the doc's header and the PR description to state spike #3 was partially addressed (sequential-only; concurrent/multi-stream remains open), matching the doc's own later sections.

Non-blocking notes (do not require action this revision)

  • HostRegisteredRegion carries no lifetime parameter tying it to the borrowed byte slice — safety currently rests on caller discipline (correctly followed at every call site in this file). Worth a PhantomData marker for defense-in-depth if this pattern is reused elsewhere later.
  • "Control 4 (falsifiability)" in run_correctness_and_bandwidth_matrix is redundant with the per-arm hard assertion already inside run_arm; not incorrect, just duplicative.
  • Qwen1.5-MoE-A2.7B's config also has a shared_expert_intermediate_size=5632 (always-active shared expert) not modeled by this fixture — appropriately out of scope for a routed-expert cold-path question, but worth a one-line acknowledgment.

Disposition

The measurement work, correctness/falsifiability controls, and NO-GO conclusion are independently re-verified as sound and should not be re-litigated or touched in this revision — only findings A and B need fixing. This is a narrow, mechanical revision (rename + doc-sync), not a rework of the spike.

Revision owner: Batty. Deckard may not self-revise this artifact (reviewer-protocol lockout — original author). Once A and B are fixed and verify_cuda_test_honesty.py passes locally with all 4 tests enumerated, this is mergeable as a test-harness-only PR (no production code changes).

… text

Revision of rejected PR #1804 per Roy's independent review, addressing
exactly the two blocking findings (Deckard, the original author, is
locked out per reviewer-protocol; this revision is independent of him):

1. Rename qmoe_zero_copy_cold_expert_spike.rs -> _gpu.rs so
   verify_cuda_test_honesty.py's _gpu-suffix convention enumerates all
   4 #[ignore]d GPU tests in this file (3 correctness/bandwidth spikes
   + the resize-starvation stress test). Update the file's own
   doc-comment repro commands and all decision-doc references to match.

2. Correct the decision doc's header and a stale trailing clause, and
   the PR body's closing line, which claimed spike #3
   (resize-starvation) was wholesale 'out of scope'. A later commit on
   this branch already added a real sequential 1000-cycle
   resize-starvation stress test, so spike #3 is now accurately
   described as partially answered (sequential-only); concurrent/
   multi-stream starvation is explicitly called out as remaining a
   follow-up, matching what the doc's own later sections already said.

No measurements, architecture conclusions, production code, or
experiment scope changed. Verified: all 4 tests pass on a live idle
A100 (qmoe_cold_expert_spike_deepseek_v2_lite_shape,
qmoe_cold_expert_spike_wide_256_expert_shape,
qmoe_cold_expert_spike_qwen15_moe_a27b_shape,
qmoe_routed_residency_guard_resize_starvation_stress_1000_steps);
cargo's own JSON test-binary discovery confirms
qmoe_zero_copy_cold_expert_spike_gpu is now classified as a CUDA test
target; cargo fmt clean on the touched crate.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 39ff6824-d35f-4d3b-8f5f-043a7119a100
@justinchuby

Copy link
Copy Markdown
Owner Author

Revision (Batty, independent of Deckard per reviewer-protocol lockout)

Addressed exactly the two blocking findings from Roy's review — no measurements, architecture conclusions, production code, or experiment scope changed.

Finding A (honesty-checker enumeration): Renamed qmoe_zero_copy_cold_expert_spike.rs → qmoe_zero_copy_cold_expert_spike_gpu.rs so verify_cuda_test_honesty.py's _gpu-suffix convention picks it up. Updated the file's own doc-comment repro commands and the decision doc's file-path/repro references to match. Verified via cargo test ... --message-format=json that qmoe_zero_copy_cold_expert_spike_gpu is now correctly discovered as a CUDA test target by the checker's classifier (is_cuda_test_target).

Finding B (stale scope text): Corrected the decision doc's header (previously said spike #3/resize-starvation was "explicitly out of scope") and a stale trailing clause in section 3, plus the PR body's closing "Closes #82" line, to accurately state spike #3 is partially answered by the sequential single-threaded 1000-cycle stress test added in a later commit on this branch, with concurrent/multi-stream starvation explicitly called out as a remaining follow-up — matching what the doc's own later sections already said.

Local evidence (A100, idle-GPU verified via nvidia-smi --query-compute-apps):

  • All 4 #[ignore]d tests run for real and pass:
    qmoe_cold_expert_spike_deepseek_v2_lite_shape, qmoe_cold_expert_spike_wide_256_expert_shape, qmoe_cold_expert_spike_qwen15_moe_a27b_shape, qmoe_routed_residency_guard_resize_starvation_stress_1000_steps — test result: ok. 4 passed; 0 failed; 0 ignored.
  • cargo fmt -p onnx-runtime-ep-cuda --check clean on the touched crate.
  • Targeted cargo test --no-run --message-format=json discovery confirms the renamed target compiles and is enumerated correctly.
  • Full-repo verify_cuda_test_honesty.py reports pre-existing "tests passed with gpu-tests on a no-CUDA host" findings across ~70 unrelated GPU targets — this is expected/pre-existing on a GPU-equipped host (the script is designed to run on CI's no-CUDA runner) and is unrelated to this rename; confirmed not a regression introduced by this change (only the intended file/doc lines are touched per git diff).

Commit: 82978c99a on squad/82-a-prime-cold-expert-spike.

Requesting Roy's re-review. Per reviewer-protocol, Deckard was not consulted for this revision.

@justinchuby

Copy link
Copy Markdown
Owner Author

Roy — re-review of PR #1804 after Batty's revision (commit 82978c99a)

Verdict: APPROVE

Independently re-verified both of my prior blocking findings on real hardware (this sandbox has an idle A100 fleet, driver 580.105.08 / CUDA 13.0 — same generation Deckard's numbers were measured on), not by re-reading Batty's report alone.

Finding A (CUDA-test-honesty enumeration) — CONFIRMED RESOLVED

  • Imported .github/scripts/verify_cuda_test_honesty.py directly and called its own is_cuda_test_target(): qmoe_zero_copy_cold_expert_spike_gpu → True (was False under the old name). Script's own --self-test passes.
  • cargo test -p onnx-runtime-ep-cuda --features cuda --test qmoe_zero_copy_cold_expert_spike_gpu --no-run --message-format=json compiles cleanly and cargo's own compiler-artifact JSON reports the target as qmoe_zero_copy_cold_expert_spike_gpu.
  • --list on the compiled binary enumerates all 4 intended tests.
  • Ran all 4 #[ignore]d tests for real on an idle A100 (CUDA_VISIBLE_DEVICES=1, verified idle via nvidia-smi --query-compute-apps first): qmoe_cold_expert_spike_deepseek_v2_lite_shape, qmoe_cold_expert_spike_wide_256_expert_shape, qmoe_cold_expert_spike_qwen15_moe_a27b_shape, qmoe_routed_residency_guard_resize_starvation_stress_1000_steps — test result: ok. 4 passed; 0 failed. First-hand confirmation, not a re-read of Batty's log.
  • cargo fmt -p onnx-runtime-ep-cuda --check clean.

Finding B (stale spike-#3 scope text) — RESOLVED where it mattered, one trivial residual noted

  • Decision doc header, section 3's trailing clause, and its "Reproduce" path were all corrected to state spike Clarify decode ownership between engine and ort crates #3 is partially (sequential-only) answered, concurrent/multi-stream explicitly open — matches the doc's own later sections now.
  • PR body's "Closes Add routed-expert paging, leases, and routing-aware scheduling #82" line was also corrected to the same accurate partial-completion wording.
  • Minor residual (non-blocking): the PR body's separate "Reproduce" code fence still cites the pre-rename filename (--test qmoe_zero_copy_cold_expert_spike, missing _gpu). Verified directly — this exact command now fails (no test target named...) against the renamed file. This is copy-paste leftover in GitHub's PR description text (not code, not a commit, not part of the reviewed diff), trivially fixed by editing the PR description directly. Recommend fixing before merge, but it doesn't reopen finding A or B's substance and isn't worth another lockout cycle.

No scope/measurement/production-code drift — confirmed

git diff a6f27d5ef...82978c99a -M: exactly 2 files, 13 lines, both text-only (doc-header correction + a 99%-similarity file rename with only the 2 repro-command doc-comments changed inside the .rs file). No test logic, assertions, byte-arithmetic, shape constants, or production code touched.

Batty's "pre-existing environment-artifact" claim — independently verified, not taken on faith

Reproduced the mechanism directly on a target this PR never touches: cargo test -p onnx-runtime-ep-cuda --features cuda,gpu-tests --test activations_gpu on this same real-GPU host — its 3 #[cfg_attr(not(feature="gpu-tests"), ignore=...)]-gated tests pass (not ignored) because a real GPU is actually present, which is exactly the "tests passed with gpu-tests on a no-CUDA host" condition the honesty script's validate_active_no_cuda_result assumes can't happen (it's designed for a no-CUDA CI runner). Since the honesty script and activations_gpu.rs are both untouched by this PR, this confirms the failure mode is a pre-existing environment mismatch, not something this PR introduced or masked.

Also found a leftover failing_targets.txt in the working tree (untracked, evidently Batty's own scratch output, timestamped before this re-review) listing exactly 70 target names — matching Batty's cited figure precisely, and qmoe_zero_copy_cold_expert_spike_gpu is not among them, i.e. our PR's own target is clean. Corroborates, does not merely repeat, Batty's claim.

Disposition

Both blocking findings are resolved with first-hand, on-hardware evidence. The one residual PR-description copy-paste nit does not block merge. Test-harness-only PR (no production code), mergeable as-is.

Worktree used for this re-review removed; no code edited.

@justinchuby
justinchuby marked this pull request as ready for review August 23, 2026 05:45
@justinchuby
justinchuby merged commit 877d378 into main Aug 23, 2026
17 checks passed
@justinchuby
justinchuby deleted the squad/82-a-prime-cold-expert-spike branch August 23, 2026 05:45
justinchuby added a commit that referenced this pull request Aug 23, 2026
…aging (#82) (#1819)

Working as Sebastian (Performance Engineer). Closes part of #82
(BlockQuantizedMoE prefill prefetch slice).

## What

Wires the existing look-ahead prefetch machinery
(`dense_weight_prefetch_lookahead_nodes` /
`executor/dispatch.rs::prefetch_lazy_weights_after`, already live for
`MatMul`/`MatMulNBits`) into `BlockQuantizedMoE` prefill weight paging,
reusing the existing `CudaWeightResidency` and `PinnedStagingPool`
authorities. No new cache, allocator, or scheduler.

## The overlap invariant

Layer/expert `L+1`'s bytes may be copied only when source/destination
lifetimes and stable VA slots cannot race layer `L`'s compute. The real
executor call order (`run_plan_eager`) issues the look-ahead prefetch
for node `pi + lookahead` immediately before executing node `pi` itself
(which promotes `pi`'s own prefetch, issued at a prior turn). Steady
state therefore genuinely needs **two concurrently-live pinned staging
buffers** of the boundary's byte size at once.

## A real bug this surfaced and fixed

`prefetch_block_quantized_moe` originally bypassed the shared
`PinnedStagingPool` (raw `alloc_pinned`) and `promote_pending_prefetch`
dropped the staging buffer instead of returning it — recreating issue
#837's exact pinned-buffer churn regression for this new path. Fixed by
routing through `PinnedStagingPool::acquire`/`retire` via a
`CopyCompleted` witness minted by the new
`CudaRuntime::resolve_prefetch_fence`.

## New soundness guard — fail closed, don't silently regress

`PinnedStagingPool::can_retain_concurrent(len, 2)` declines a prefetch
up front (`declined_pool_capacity`) when the pool's retention bounds
can't hold 2 concurrent buffers of the boundary's size. Under the pool's
default **512 MiB** cap this fires for **both** cited real models:
- DeepSeek-V2-Lite: ~280.5 MiB/layer (`2x` exceeds the cap)
- GLM-5.2: ~4.78 GiB/layer (`1x` already exceeds it)

This is a **truthful no-win** for both real models as currently sized —
the feature fails closed rather than regressing silently.

## Independent review — 2 substantive findings, both fixed

A `code-review` pass on the diff found:
1. **[Critical]** `resolve_prefetch_fence`'s two callers dropped a
pending prefetch's staging buffer/destination page on a bare `Err`, with
no proof the DMA had finished — unlike this file's existing
`FailedHtodCompletion`/`quarantine_in_flight_fill` machinery for the
analogous on-demand path. **Fixed** by giving `resolve_prefetch_fence`
the same classified-completion return type (reusing
`HtodAsyncElapsedError`/`FailedHtodCompletion` and the existing
`settle_submitted_htod_failure` fallback-sync helper — no new mechanism)
and routing both call sites into the existing
`quarantine_in_flight_fill` on `MayBeInFlight`.
2. **[Medium]** The benchmark's `correctness=pass` tag never actually
read back a page that went through prefetch+promote, only the
pre-existing on-demand path. **Fixed** by dtoh-verifying the ON arm's
final-layer page (the one actually promoted from a look-ahead prefetch
when the capacity guard admits it) against its known source pattern.

## Measurements (A100, GPU 1 idle, hostlock-gated, 3 independent runs +
a post-rebase re-check)

| case | off_us (median) | on_us (median) | htod_GB/s | %PCIe peak |
outcome |
|---|---|---|---|---|---|
| deepseek-v2-lite (280.5 MiB/layer) | ~290-370k | ~290-520k | 23.4-25.3
| 74-80% | `declined_pool_capacity` (no-win, expected) |
| glm-5.2 (4.78 GiB/layer) | ~12.0-13.1M | ~12.1-13.3M | 24.9-26.2 |
79-83% | `declined_pool_capacity` (no-win, expected) |
| synthetic-128MiB (mechanism proof) | ~135-144k | ~231-245k | 22.3-24.5
| 71-78% | `overlap_measured` — **ON is ~1.7-1.8x worse than OFF**,
honestly reported |

- Real H2D DMA (pre-existing `materialize_ns`/`htod_ns` counters, not
new instrumentation) is healthy: ~23-26 GB/s, 74-83% of this box's
confirmed **PCIe Gen4 x16** link (31.5 GB/s peak, verified via
`nvidia-smi --query-gpu=pcie.link.gen.current,pcie.link.width.current` →
`4, 16`). An earlier revision of this benchmark divided by the device's
*HBM* peak instead — corrected.
- DeepSeek/GLM: `on_us` ≈ `off_us` (both decline; no measurable
difference beyond noise, as expected when the mechanism never fires).
- Synthetic case: `on_us` is consistently worse than `off_us` at
`layers=2` — the one-time cost of provisioning 2 concurrently-live
pinned buffers from empty dominates any overlap benefit at this layer
count. A truthful no-win.
- **Documented benchmark limitation:** extending the synthetic case
beyond 2 layers surfaces a single-pending-slot self-conflict in a
*tight* issue/promote loop with zero intervening work — a property of
this probe's loop shape, not of production (which interleaves many
non-lazy-weight plan nodes between MoE boundaries). Faithfully measuring
steady-state reuse needs the real plan-node distance between consecutive
`BlockQuantizedMoE` boundaries in a loaded model's compiled plan —
flagged as the next research input, not invented.

CI was not waited on for this PR per standing user directive; all
evidence above is local A100 measurement + independent review.

## Tests (same commit)

- `weight_offload_gpu.rs`: **15/15** passing (13 pre-existing + 2 new —
pool-capacity decline guard, buffer-reuse-across-cycles).
- `bqmoe_prefetch_overlap_gpu.rs` (new): A100 double-buffer overlap
probe across DeepSeek-V2-Lite, GLM-5.2 (real config-derived shapes) and
a synthetic capacity-sufficient case; asserts fast-path/decline counters
and byte-identical correctness (including the promoted-page readback
added during review), emits machine-readable `BQMOE_PREFETCH_OVERLAP`
output.
- `cargo fmt`/`cargo clippy -- -D warnings` clean on every touched
crate/target.

## Scope

BlockQuantizedMoE prefill prefetch only. Does **not** touch QMoE
zero-copy/routed residency (#1797/#1804, Deckard's lane), q* CPU/GPU
split, or introduce new policy architecture.

## Next gate

Real-model benefit is gated on enlarging `PinnedStagingPool`'s retention
bounds for boundary-scale buffers (or reordering the shared
prefetch/exec call site so steady state needs only 1 concurrent buffer)
— until then this mechanism is architecturally correct but a no-win for
DeepSeek-V2-Lite/GLM-5.2 under default sizing. A second, independent
follow-up is measuring the real plan-node distance between consecutive
`BlockQuantizedMoE` boundaries in a loaded model, needed to build a
faithful multi-layer steady-state probe.

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.

Add routed-expert paging, leases, and routing-aware scheduling

1 participant