Repository navigation
fix(v4): the raw xs window may hold rows shorter than itself — lift of #116 onto master - #128
Merged
Merged
Conversation
`set_row_lens` refused any row whose token count was below the batched
window's width. That is not the invariant the end-anchored layout needs —
this file's own doc states the right one, `base[i] >= tokens[i] - W` — and
it is violated by every batch of unequal prompts, because the window is
sized for the greediest row and every shorter row is front-padded into it.
Measured consequence, twice on an H200: `ARC_V4_XS_PER_SEQ=1
ARC_MTP_PER_SEQ_KV=1` at B=8 returned one token and `finish_reason: None`
for every request, dying at the first decode step with
row 3 holds 9 tokens, fewer than the 11-wide retained window
Three changes, all in the representation, none relaxing a check:
* `set_row_lens` and `plan_xs_advance` check `base + W >= tokens` — the
row's promised run fits the window — instead of `tokens >= W`, which is
sufficient for the arithmetic rather than necessary for correctness.
* The history-gap guard now bounds reads by the row's OWN `base` rather
than by the shared window's physical start. The columns between the two
are a neighbour's width — older tokens this row never promised, or zero
front pad — and reading them is a silent wrong answer.
* `advance` dispatches to the scalar fast path on the assumption that path
actually makes (one `tokens`, one `base`, `W == tokens - base`) instead
of on token equality alone. Equal lengths with unequal `base` took the
fast path and compressed the wrong tokens for every row.
Offsets are now derived per row from that row's own token count, which
makes `off >= W - tokens_i` an identity: the front pad is unreadable
rather than merely unread, so no mask is needed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fix above stopped requiring `W == max_i (tokens_i - base_i)` as a side
effect: `plan_xs_advance` and `set_row_lens` take `W` as given and ask only
that every row fit inside it, and every read offset is
`need_start + W - tokens_i`, which tracks `W` exactly. So `W` may be any
width at or above the widest row's run — including a constant one.
Worth recording because this buffer does not have a constant size today.
`tail` is rebuilt by `cat` + `narrow` every step at width `tokens - base`,
and `tokens` climbs by one per token while `base` jumps a whole `ratio` at a
group boundary, so the allocation cycles through `ratio` consecutive sizes.
The ArcGraph chain measured that from the outside — `4096 x {18,19,20,21}`
at `hidden = 4096`, `ratio = 4`. Their hypothesis that it is what stops a
graph replaying is retracted as unproven; the size measurement stands as a
fact about this buffer regardless, and nothing here depends on changing it.
`an_oversized_window_reads_exactly_the_same_absolute_tokens` turns the claim
into a check: widening the window shifts every offset by exactly the
widening and reads the same absolute tokens, with identical groups, dests,
takes, bands and per-row bookkeeping.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Metrics Report━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Language Files Lines Code Comments Blanks ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ C Header 5 305 210 52 43 CSS 2 1181 1036 34 111 CUDA 72 24645 17702 4215 2728 Dockerfile 1 39 22 8 9 JavaScript 16 3546 2676 482 388 Jinja2 7 694 656 5 33 JSON 74 4600 4597 0 3 Makefile 1 6 5 0 1 Metal Shading Lan| 33 12224 9431 1142 1651 PowerShell 1 300 227 30 43 Python 143 14830 12217 797 1816 Shell 24 5924 4071 1457 396 Plain Text 4 3801 0 2479 1322 TOML 33 1485 1292 43 150 YAML 3 25 23 2 0 ───────────────────────────────────────────────────────────────────────────────── HTML 4 2687 2604 43 40 |- CSS 2 543 479 37 27 |- JavaScript 1 1233 1215 12 6 (Total) 4463 4298 92 73 ───────────────────────────────────────────────────────────────────────────────── Jupyter Notebooks 4 122 83 23 16 |- Markdown 1 60 30 22 8 |- Python 1 122 113 1 8 (Total) 304 226 46 32 ───────────────────────────────────────────────────────────────────────────────── Markdown 194 38699 0 29715 8984 |- BASH 71 1632 1192 324 116 |- C 3 17 17 0 0 |- CUDA 2 84 56 16 12 |- JSON 18 708 708 0 0 |- PowerShell 1 1 1 0 0 |- Python 23 1008 787 113 108 |- Rust 65 2048 1713 77 258 |- TOML 6 207 164 0 43 |- YAML 4 38 33 5 0 (Total) 44442 4671 30250 9521 ───────────────────────────────────────────────────────────────────────────────── Rust 663 313742 271458 14550 27734 |- Markdown 478 24544 471 21171 2902 (Total) 338286 271929 35721 30636 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Total 1284 461100 335289 76855 48956 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ |
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.
Lift of #116's two commits onto current master, unchanged. Same author, same content (
73a0c72e3+6e408eb2e, cherry-picked); this exists because #116 is parked on #104 by branch topology, not by content, and the defect it fixes is real and measured.#116 sits above #114 → #104, and #104 is held on a genuine open question — it and #122 (now master) are two mutually exclusive designs for ragged decode that contend on the
clone_in_cachecadence, which needs an A/B nobody has run. None of that touches this fix.Separability, proved rather than asserted
The two commits touch only
kv_cache/xs_rolling.rsandmodels/deepseek4.rs. #104's work isscheduler/,engine/mod.rs,mtp_pipeline.rs,ragged_admission.rs. Disjoint. Cherry-picked onto006657e05:including the three that carry the fix:
The defect
ARC_V4_XS_PER_SEQ=1 ARC_MTP_PER_SEQ_KV=1at B=8 returned 1 token,finish_reason: None, on 8/8 requests — measured twice on an H200 by #114, atbe29f397, with zero occurrences in the OFF control and a full 48 tokens at B=1.Root cause, and it is a corrected diagnosis rather than the first theory.
xs_rolling.rsrefused any row whose token count was below the batched window's width. That is not the invariant the end-anchored layout needs — the file's own doc already stated the right one:base[i] >= tokens[i] - W, i.e. the row's promised run fits in the window. The code checkedtokens >= W, which is sufficient to keep the offset arithmetic insideusizebut not necessary for correctness, so it rejected rows that were perfectly representable.So the fix is not the shape #114 ranked first. Every piece of the per-row machinery already existed:
tokens/baseare alreadyVec<usize>,clone_in_cachealready front-pads a short row into the shared window (v_slack_at_front: true→pad_slack), andsplit_rowalready re-anchors on the way out. What was wrong was the invariant being checked.What this does NOT include
#121's window pin (the measured +23.2% at uniform B=32) is deliberately not here, and it is not separable. Two independent reasons, both checked:
5ccd1a49acallsragged_bucket_len(&running, **&self.admission**)indefault_scheduler.rs—self.admissionis feat(scheduler): admit ragged cohorts — the admission layer #103 named, and a general throughput bug #104's ragged-admission state. That commit cannot exist without feat(scheduler): admit ragged cohorts — the admission layer #103 named, and a general throughput bug #104, and every later perf(v4): pin the xs window (+23.2% at uniform B=32) + the ladder that prices per-sequence advance — 4.46x at spread B=8 [MEASURED] #121 commit sits on it.57d330c8b, which touches only kv_cache/deepseek4 files) conflicts with this PR's own second commit: it removescompressor_needs_from(), which6e408eb2eadds — 0 occurrences survive on perf(v4): pin the xs window (+23.2% at uniform B=32) + the ladder that prices per-sequence advance — 4.46x at spread B=8 [MEASURED] #121's tip. So fix(v4): the raw xs window may hold rows shorter than itself — B=8 per-seq advance generates again [MEASURED] #116→perf(v4): pin the xs window (+23.2% at uniform B=32) + the ladder that prices per-sequence advance — 4.46x at spread B=8 [MEASURED] #121 is a genuine rebase, not a set of independent cherry-picks.⇒ #116 lifts; #121 does not. The pin stays parked until #104 resolves or its owner rebases the sequence deliberately.
Disposition of #116
#116 is not closed by this. Once this lands, its content is on master and it can close on its own terms, by its own author. Nothing has been force-pushed, rebased, or deleted — #116, #114, #121 and #104 are all untouched on the remote.