Repository navigation
Record the third failure mode: instruments that lie - #729
Merged
Merged
Conversation
The document already covered code that is right and never reached, and measurements that support a conclusion they do not license. A third mode cost more than either today: 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 step time and retired an entire line of work on that basis. It bracketed an async enqueue returning, not a transfer completing. The arithmetic gave it away: the same step stages 7.33 GiB, and 7.33 GiB in 30 ms is ~250 GB/s, about ten times this machine's link. The first repair renamed it to copy_wait_ms around a call whose own doc says "non host-blocking", so the same non-measurement survived under a name that invited the wrong reading more strongly. Measured with CUDA events, transfer is 18.8%. Two habits follow: divide bytes by time and ask whether the answer is a real bandwidth; and state on every timing counter what lies between start and stop and whether anything there blocks the host. Also records the pattern underneath all three - telemetry that never reaches the operator, now three times in one subsystem - and updates the status table where today changed the facts: --vram-limit now enforced, the weight residency cache's 0% hit rate and its fix, the KV page store contract opening, and captured graphs verified to survive a VMM remap. 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 #729 +/- ##
==========================================
- Coverage 80.14% 80.13% -0.02%
==========================================
Files 331 329 -2
Lines 132743 130573 -2170
Branches 132743 130573 -2170
==========================================
- Hits 106383 104629 -1754
+ Misses 22118 21714 -404
+ Partials 4242 4230 -12
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 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.
Consolidating today's findings into the document rather than leaving them in issues.
The new section: a third failure mode
MEMORY_ARCHITECTURE.mdalready 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_msput 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 atcompute_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:
weight_paging.rssince Speed up CUDA weight offload page-in #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 aslimit − device state. Caveat carried rather than closed: activations and runtime overhead are stillunknownand unsubtracted ([memory] Define a MemoryPlanner trait and wire onnx-runtime-memory into the execution path #514). PR Enforce explicit VRAM limit at load #717evict_to_fitis LRU against a cyclic sequential scan. Fixed to 74.18% against aB/W ≈ 76%ceiling, evictions 6,286 → 0. Weight-offload miss traffic is invariant to the residency budget: 3 GB and 6 GB both re-stage 7.33 GiB/token #720, PR Add scan-resistant CUDA weight residency eviction #723onnx-genai-kvnever touched device memory; the contract is now open (Box<dyn KvPageStore>), host-only pending stages 2–5. PR Extract host KV page store #726Docs only.