Build the unified read stream directly, without the page-table rectangle - #37512
Merged
Merged
Conversation
ch-wan
requested review from
BBuf,
DarkSharpness,
Fridge003,
HaiShaw,
HydraQYH,
Qiaolin-Yu,
Ying1123,
alphabetc1,
celve,
hanming-lu,
hebiao064,
hnyls2002,
huangtingwei9988,
hzh0425,
ispobock,
merrymercy,
xiezhq-hermann,
yizhang2077 and
yuan-luo
as code owners
September 2, 2026 02:43
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Collaborator
Author
|
/tag-and-rerun-ci |
5 tasks
This was referenced Sep 2, 2026
ch-wan
force-pushed
the
cheng/unified-drop-capture-tables
branch
from
September 2, 2026 08:11
9540561 to
d6b0123
Compare
ch-wan
force-pushed
the
cheng/unified-alloc-fusion
branch
from
September 2, 2026 19:50
f9feea3 to
972729f
Compare
ch-wan
force-pushed
the
cheng/unified-drop-capture-tables
branch
from
September 2, 2026 19:50
d6b0123 to
308631e
Compare
ch-wan
force-pushed
the
cheng/unified-alloc-fusion
branch
from
September 2, 2026 20:54
972729f to
7d39aaf
Compare
ch-wan
force-pushed
the
cheng/unified-drop-capture-tables
branch
from
September 2, 2026 20:54
308631e to
f315619
Compare
ch-wan
force-pushed
the
cheng/unified-alloc-fusion
branch
from
September 2, 2026 23:54
7d39aaf to
9a9fae4
Compare
`build_kv_read_table` fills a `[bs, max_pages]` page table, which three of the four unified-pool consumers then hand straight to `create_flashinfer_kv_indices_triton` to repack into the indptr-addressed stream a wrapper plans over. The rectangle is never read by an attention kernel on those paths -- it exists only to be re-gathered. `build_kv_read_table_packed` does both halves in one pass, writing `out[indptr[b] + p] = entry * ps + p % ps` directly. This commit adds the kernel and the translator entry point; the backends move over next. Two things follow from the layout rather than from the fusion. The stream holds one id per resident token, so its length is `sum(seq_lens)` -- bounded by the pool, where a rectangle is bounded by `max_bs x max_context_len` and at ps=1 reserves 32x more slots than the pool can ever fill. And the repack turns out to cost more than the build it reads: bs= 64 seq= 8224 8.6us -> 2.8us 3.1x bs= 8 seq= 8224 7.6us -> 1.8us 4.2x bs=256 seq= 8224 12.3us -> 5.8us 2.1x bs= 64 seq=131072 162.8us -> 25.1us 6.5x Verified bit-identical to the two steps it replaces over 40 randomized cases (page_size 1..4, mixed seq_lens, `-1` holes, tombstoned v2p rows, and half the trials driving `kv_start_idx` so the windowed gather the non-MLA path uses is covered), plus CPU/CUDA agreement on the pure-torch reference the allocator's CPU tests run through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both indices updaters built a `[max_bs, max_pages]` page table and immediately handed it to `create_flashinfer_kv_indices_triton` to repack; nothing else read it. They now fill the stream directly and the rectangle is gone from this backend. The updaters took `kv_view` rather than `req_pool_indices` because the rectangle is indexed by batch lane -- the row ids were `arange(bs)` and the real pool indices were never needed. Filling from `req_to_token` needs them, so the decode updater takes `req_pool_indices` instead of the view; the prefill updater already had it. Kimi-Linear TP2 + unified, 1M context, cuda_graph_max_bs 256: decode CUDA graph memory 2.89 GB -> 1.89 GB which is the 1.0 GiB the rectangle held, to the reported precision. Throughput is unchanged: the repack this removes is ~5.8us per step against ~780ms of decode, two orders of magnitude under the run-to-run spread. GSM8K 0.9175 vs 0.9125 on main -- the same server returns 0.9175 / 0.9175 / 0.9125 across three runs, so 400-example GSM8K on this box carries a +-2 question band and neither number is evidence either way. The correctness argument is the randomized equality against the two steps this replaces, not the eval. `make_capture_tables` still serves flashattention, flashinfer, triton and trtllm_mha. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The packed builder arrived as a second `@triton.jit` alongside the page-table one, duplicating the gather-and-translate formula that is the load-bearing part of this file -- a fix to the entry derivation would have had to land twice. They now share one kernel. What differs is an output layout and a granularity, both constexpr: `row_starts` null means rows sit a uniform stride apart, and `EMIT_PER_TOKEN` selects whether an item is a page (emit the entry) or a token (emit `entry * ps + pos % ps`). The two public launchers stay, so a call site still names the form it wants; the CPU reference paths share `_entries` for the same reason the kernels now share a body. Deriving the per-row item count moved into the kernel. Computing `n_pages` on the host first cost the page-table path 2.66us -> 5.55us at bs=64 / seq=8224 -- a whole launch for one elementwise op, which a captured graph then replays every step. The kernel already loads `seq_lens`; the delivery form tells it what an item is. No behaviour change: both forms re-verified against what they were checked against before (the page table against an independent CPU model over 30 randomized cases plus graph replay at four sequence lengths; the stream against [page table + repack] over 40, half driving `kv_start_idx`), and the page table's timings are unchanged across bs 1..256 x seq 1k..128k. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same move as the MLA backend, over a wider surface: `kv_view` was threaded
through five frames of each updater chain. The prefill chain already carried
`req_pool_indices`, so those frames just lose a parameter; the decode chain
never needed the real pool indices (a rectangle is indexed by batch lane, so
its row ids were `arange(bs)`) and now takes them instead.
Two things the view was answering for get names of their own:
- `use_swa_source` asked `kv_view.is_translated`, which has to be known
before the fill picks a source. That predicate is now
`KVIndexTranslator.reads_are_translated` -- unified, minus the DCP defer.
- `fill_packed_read_stream` gained `sliding_window`, selecting the swa
sub-pool's own v2p and multiplier. Built from VIRTUAL ids, never chained
through full-physical, which is what the swa canonical array was for.
`fill_packed_read_stream` now takes the batch size from `seq_lens` and slices
`req_pool_indices` to it. Callers hold indices padded to the graph bucket while
asking for fewer rows; the grid used to come from the indices, so the extra
lanes would have run with no length to bound them.
Falcon-H1-1.5B + unified + flashinfer, branch vs base, GSM8K over 400:
base 0.7725 / 0.7850 / 0.7825 mean 0.7800
tip 0.7850 / 0.7775 / 0.7775 mean 0.7800
decode CUDA graph memory 0.89 GB -> 0.77 GB. (A 1.5B model's context makes the
rectangle small; the 1.0 GiB figure is Kimi-Linear at 1M.)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last stream consumer. `_fill_kv_indptr_and_indices` and `update_sliding_window_buffer` both built a page table and immediately repacked it through `create_flashinfer_kv_indices_triton`; both now fill their CSR buffer directly. The SWA one already had a window offset per row, which the stream builder takes as `kv_start_idx`. Triton's index buffers are int64, so the builder emits either width, chosen from the destination's dtype. Validated on Falcon-H1-1.5B + unified + triton (hybrid mamba, so the full stream only), GSM8K over 400: base 0.7850 / 0.7825 / 0.7775 mean 0.7817 tip 0.7750 / 0.7850 / 0.7850 mean 0.7817 The SWA stream is NOT covered by that run and I could not cover it: the one hybrid-SWA model available here, gemma-4-31b-it, dies under unified memory on plain main -- a device-side index assert, GSM8K 0.0 and the scheduler gone, where the same model on the same commit with the static pool scores 0.98. This branch reproduces it identically, so it is not from this change, but it means the SWA wiring here rests on the kernel-level equality (which does cover `kv_start_idx` and both id spaces) and not on an end-to-end run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`make_capture_tables` is gone, and with it the last rectangle the unified pool allocated on anyone's behalf. trtllm_mla and flashmla were already doing this right: they call `fill_read_table` with their own block table and consume it in place. The other two page-table consumers each held a second rectangle instead. **trtllm_mha** allocated `kv_read_tables` and, at capture, rebound `metadata.page_table` to it -- while its own `page_table`, allocated in `init_cuda_graph_state`, is explicitly *not written* under unified (`page_table=None`, `skip_page_table=unified`). Two buffers of identical shape and dtype, one dead. It now fills its own, which also retires the `in_capture` rebinding special case: one buffer, refreshed in place on capture and on every replay-prep, so there is nothing to bind once. **flashattention** built the rectangle and had `normal_decode_set_metadata` copy it verbatim into `metadata.page_table`. That fused kernel gains `SKIP_PAGE_TABLE`, a constexpr early-out after the prefix sum, so the translator can write the page tables directly and the kernel is left with the work only it can do. The skip path launches one block, because one block is what the prefix sum uses. `fill_read_table` grew `sliding_window_out` to fill the swa twin in the same pass, which both of these need and the two MLA callers do not. Measured, Falcon-H1-1.5B + unified + fa3, decode CUDA graph memory 0.52 GB -> 0.39 GB, with GSM8K over 400 across five runs a side: base 0.7775 / 0.7625 / 0.7800 / 0.7900 / 0.7775 mean 0.7775 tip 0.7825 / 0.7975 / 0.7825 / 0.7825 / 0.7800 mean 0.7850 At three runs a side those bands looked disjoint; they are not, and the base's own spread is 11 questions. **trtllm_mha has no end-to-end run behind it.** No configuration on this box reaches trtllm_mha with unified memory: Falcon-H1 OOMs in `forward_extend` (215 GiB), and gemma-4 is rejected for `head_dim: 512`. The change is a mechanical swap onto the API its two MLA siblings already use, and the unit suite passes, but a reviewer with the hardware should exercise it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two findings from auditing the read path, both predating this branch. **The page table was built one length and read another.** The builder was handed `seq_lens`; fa3's fused metadata kernel and trtllm_mha's attention kernels bound their page-table reads by `cache_seqlens`, which is `seq_lens + seq_len_delta` on a draft decode and `seq_lens + draft_token_num` on a target verify. The columns in between were read and never translated -- zeros (the page-0 sink) on a fresh capture buffer, and the previous batch's entries on a replay. A comment at fa3's draft-decode site asserted this could not happen: "Spec is asserted off under the unified pool." It is not. `kv_cache_hook` allows `--speculative-algorithm DSPARK` with unified memory, and DSPARK is a chain draft with topk 1, which is exactly the branch that comment sits in. Both now fill to `metadata.cache_seqlens_int32` -- the tensor the reader bounds by, already computed by the fused call above -- instead of to `seq_lens`. For fa3 that meant running the fused call first, which costs nothing: it was already going to run, and it is what produces the bound. **`fill_read_table` could silently fill nothing.** It asserted `is_translating`, but `build_index_table` returns the passthrough view without touching `into` when reads are deferred, which is what DCP does. trtllm_mla returns early on `dcp_enabled` before reaching it; flashmla does not, and its `block_kv_indices` is a reused buffer, so unified + DCP + flashmla would have read the previous batch's indices. The precondition is now `reads_are_translated`, so that combination fails by name instead. Also drops `src_is_read_table` / `swa_src_table` / `SRC_IS_KERNEL_PAGE_TABLE` from the fused metadata kernel. fa3 was the only caller and no longer takes that path, so the "copy the read table verbatim" branch is dead: -41 lines, and one mechanism in the file instead of two. `test_unified_mla_block_table.py` moves onto the new one; the invariant it pins -- the page table ends up holding kernel-facing ids -- is unchanged. Falcon-H1-1.5B + unified + fa3 after the reorder: 0.7825 / 0.7900 / 0.7900, against a base spanning 0.7625-0.7900 over five runs. Decode graph memory stays 0.39 GB. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ch-wan
force-pushed
the
cheng/unified-drop-capture-tables
branch
from
September 2, 2026 23:55
f315619 to
4841c24
Compare
ch-wan
added a commit
to ch-wan/sglang
that referenced
this pull request
Sep 4, 2026
sgl-project#37511 and sgl-project#37512 took the unified pool's READ path off the per-step launch path: the read table is built by one kernel whose grid is sized from bs, and the flat consumers (fa3, flashinfer, triton, trtllm_mha) were converted to a packed stream. The WRITE path was left eager, and the block-table backends (trtllm_mla, cutedsl_mla, tokenspeed_mla, flashmla) were not converted at all. Hopper's MLA default is fa3, so it got both halves of that work. Blackwell runs the trtllm_mla family, which got neither -- which is why the Hopper fix did not carry over. `translate_kv_loc_for_kernel` was 6 torch ops (floor_divide, remainder, take, mul, add, clamp) and `translate_write_loc_for_kernel` layered 6 more on top (remainder, eq, floor_divide, zeros_like, where, copy). Both run per forward on `out_cache_loc`, OUTSIDE any cuda graph, so each op is a real launch on the critical path. Profiling a bs=8 decode on B300 (24 steps, 2 ranks): GPU kernel time unified 147.56 ms static 153.67 ms -6.11 ms wall span unified 81.10 ms static 77.92 ms +3.18 ms (+4.1%) cudaLaunchKernel unified 988 static 532 +456 cpu_op time unified 51.40 ms static 35.24 ms +16.16 ms GPU time was already LOWER; the loss was ~10 extra launches per step per rank. That is invisible next to a Hopper decode step and is not next to a Blackwell one. Route both translates through one Triton kernel in virtual_slot.py, the module sgl-project#37511 added for exactly this class of fusion. It carries the DCP owner rule, so the widened-loc path is the same single launch. Triton truncates division toward zero where torch floors it, so a negative loc is tested explicitly rather than relying on the division to reach the v2p sentinel row. Measured on B300, TP2 DCP2, cutedsl_mla, unified vs static decode throughput over 6 shape/batch points: mean gap -1.70% -> -0.23%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
caihuali95
pushed a commit
to caihuali95/sglang
that referenced
this pull request
Sep 5, 2026
The four spec-input CSR builders took a raw `(req_pool_indices,
req_to_token)` pair and ran `create_flashinfer_kv_indices_triton`
themselves, so a translating batch had no way to reach them: the ids in
`req_to_token` are VIRTUAL under the unified pool, and nothing in that
argument pair can say so.
They now take the `KVIndexTranslator` and gather through
`fill_packed_read_stream`, which is what upstream's non-spec branches in
these same functions already do. One call replaces the hand-rolled
launch:
translator.fill_packed_read_stream(
req_pool_indices=req_pool_indices,
seq_lens=paged_kernel_lens, # widened just above
indptr=cum_kv_seq_len,
total_tokens=total_tokens,
out=kv_indices,
)
Three things fall out of using it rather than a page-table rectangle:
* No rectangle. The old path built a `[bs, max_pages]` table and
repacked it into the CSR stream the wrapper actually plans over --
the exact pattern sgl-project#37512 removed from the non-spec branches, kept
alive here only because spec was refused on unified pools. At page
size 1 that rectangle reserves far more slots than the pool has
tokens.
* No widening contract. The builders already widen their own lens
(`paged_kernel_lens + draft_token_num`, per-row for DFlash's ragged
layout) and now hand those widened lens straight to the translator.
There is no separate table whose live prefix could fall short, so
`seq_len_delta` is not part of this path at all.
* No id-space plumbing. `sliding_window=` selects the swa sub-pool's
own id space, mirroring the non-spec branches, instead of re-aiming
a table at it.
Behaviour on a plain pool is unchanged by construction: with
`reads_are_translated` false, `fill_packed_read_stream` runs the same
`create_flashinfer_kv_indices_triton` launch over the same
`req_to_token` at `ENTRY_PAGE_SIZE=1` -- the call these builders made
by hand.
layers/attention/aiter_backend.py: its three updaters bind
`kv_index_translator` beside the `req_to_token` they already hold. aiter
is absent from the spec-verify allow-list, so it only ever sees the
non-translating branch -- the bind lets it satisfy the signature, not a
claim that it translates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ch-wan
added a commit
to ch-wan/sglang
that referenced
this pull request
Sep 5, 2026
sgl-project#37511 and sgl-project#37512 took the unified pool's READ path off the per-step launch path: the read table is built by one kernel whose grid is sized from bs, and the flat consumers (fa3, flashinfer, triton, trtllm_mha) were converted to a packed stream. The WRITE path was left eager, and the block-table backends (trtllm_mla, cutedsl_mla, tokenspeed_mla, flashmla) were not converted at all. Hopper's MLA default is fa3, so it got both halves of that work. Blackwell runs the trtllm_mla family, which got neither -- which is why the Hopper fix did not carry over. `translate_kv_loc_for_kernel` was 6 torch ops (floor_divide, remainder, take, mul, add, clamp) and `translate_write_loc_for_kernel` layered 6 more on top (remainder, eq, floor_divide, zeros_like, where, copy). Both run per forward on `out_cache_loc`, OUTSIDE any cuda graph, so each op is a real launch on the critical path. Profiling a bs=8 decode on B300 (24 steps, 2 ranks): GPU kernel time unified 147.56 ms static 153.67 ms -6.11 ms wall span unified 81.10 ms static 77.92 ms +3.18 ms (+4.1%) cudaLaunchKernel unified 988 static 532 +456 cpu_op time unified 51.40 ms static 35.24 ms +16.16 ms GPU time was already LOWER; the loss was ~10 extra launches per step per rank. That is invisible next to a Hopper decode step and is not next to a Blackwell one. Route both translates through one Triton kernel in virtual_slot.py, the module sgl-project#37511 added for exactly this class of fusion. It carries the DCP owner rule, so the widened-loc path is the same single launch. Triton truncates division toward zero where torch floors it, so a negative loc is tested explicitly rather than relying on the division to reach the v2p sentinel row. Measured on B300, TP2 DCP2, cutedsl_mla, unified vs static decode throughput over 6 shape/batch points: mean gap -1.70% -> -0.23%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
caihuali95
pushed a commit
to caihuali95/sglang
that referenced
this pull request
Sep 6, 2026
The four spec-input CSR builders took a raw `(req_pool_indices,
req_to_token)` pair and ran `create_flashinfer_kv_indices_triton`
themselves, so a translating batch had no way to reach them: the ids in
`req_to_token` are VIRTUAL under the unified pool, and nothing in that
argument pair can say so.
They now take the `KVIndexTranslator` and gather through
`fill_packed_read_stream`, which is what upstream's non-spec branches in
these same functions already do. One call replaces the hand-rolled
launch:
translator.fill_packed_read_stream(
req_pool_indices=req_pool_indices,
seq_lens=paged_kernel_lens, # widened just above
indptr=cum_kv_seq_len,
total_tokens=total_tokens,
out=kv_indices,
)
Three things fall out of using it rather than a page-table rectangle:
* No rectangle. The old path built a `[bs, max_pages]` table and
repacked it into the CSR stream the wrapper actually plans over --
the exact pattern sgl-project#37512 removed from the non-spec branches, kept
alive here only because spec was refused on unified pools. At page
size 1 that rectangle reserves far more slots than the pool has
tokens.
* No widening contract. The builders already widen their own lens
(`paged_kernel_lens + draft_token_num`, per-row for DFlash's ragged
layout) and now hand those widened lens straight to the translator.
There is no separate table whose live prefix could fall short, so
`seq_len_delta` is not part of this path at all.
* No id-space plumbing. `sliding_window=` selects the swa sub-pool's
own id space, mirroring the non-spec branches, instead of re-aiming
a table at it.
Behaviour on a plain pool is unchanged by construction: with
`reads_are_translated` false, `fill_packed_read_stream` runs the same
`create_flashinfer_kv_indices_triton` launch over the same
`req_to_token` at `ENTRY_PAGE_SIZE=1` -- the call these builders made
by hand.
layers/attention/aiter_backend.py: its three updaters bind
`kv_index_translator` beside the `req_to_token` they already hold. aiter
is absent from the spec-verify allow-list, so it only ever sees the
non-translating branch -- the bind lets it satisfy the signature, not a
claim that it translates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
StevenChenSE
pushed a commit
to StevenChenSE/sglang
that referenced
this pull request
Sep 6, 2026
…gle (sgl-project#37512) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
ch-wan
added a commit
to ch-wan/sglang
that referenced
this pull request
Sep 7, 2026
sgl-project#37511 and sgl-project#37512 took the unified pool's READ path off the per-step launch path: the read table is built by one kernel whose grid is sized from bs, and the flat consumers (fa3, flashinfer, triton, trtllm_mha) were converted to a packed stream. The WRITE path was left eager, and the block-table backends (trtllm_mla, cutedsl_mla, tokenspeed_mla, flashmla) were not converted at all. Hopper's MLA default is fa3, so it got both halves of that work. Blackwell runs the trtllm_mla family, which got neither -- which is why the Hopper fix did not carry over. `translate_kv_loc_for_kernel` was 6 torch ops (floor_divide, remainder, take, mul, add, clamp) and `translate_write_loc_for_kernel` layered 6 more on top (remainder, eq, floor_divide, zeros_like, where, copy). Both run per forward on `out_cache_loc`, OUTSIDE any cuda graph, so each op is a real launch on the critical path. Profiling a bs=8 decode on B300 (24 steps, 2 ranks): GPU kernel time unified 147.56 ms static 153.67 ms -6.11 ms wall span unified 81.10 ms static 77.92 ms +3.18 ms (+4.1%) cudaLaunchKernel unified 988 static 532 +456 cpu_op time unified 51.40 ms static 35.24 ms +16.16 ms GPU time was already LOWER; the loss was ~10 extra launches per step per rank. That is invisible next to a Hopper decode step and is not next to a Blackwell one. Route both translates through one Triton kernel in virtual_slot.py, the module sgl-project#37511 added for exactly this class of fusion. It carries the DCP owner rule, so the widened-loc path is the same single launch. Triton truncates division toward zero where torch floors it, so a negative loc is tested explicitly rather than relying on the division to reach the v2p sentinel row. Measured on B300, TP2 DCP2, cutedsl_mla, unified vs static decode throughput over 6 shape/batch points: mean gap -1.70% -> -0.23%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
caihuali95
pushed a commit
to caihuali95/sglang
that referenced
this pull request
Sep 8, 2026
The four spec-input CSR builders took a raw `(req_pool_indices,
req_to_token)` pair and ran `create_flashinfer_kv_indices_triton`
themselves, so a translating batch had no way to reach them: the ids in
`req_to_token` are VIRTUAL under the unified pool, and nothing in that
argument pair can say so.
They now take the `KVIndexTranslator` and gather through
`fill_packed_read_stream`, which is what upstream's non-spec branches in
these same functions already do. One call replaces the hand-rolled
launch:
translator.fill_packed_read_stream(
req_pool_indices=req_pool_indices,
seq_lens=paged_kernel_lens, # widened just above
indptr=cum_kv_seq_len,
total_tokens=total_tokens,
out=kv_indices,
)
Three things fall out of using it rather than a page-table rectangle:
* No rectangle. The old path built a `[bs, max_pages]` table and
repacked it into the CSR stream the wrapper actually plans over --
the exact pattern sgl-project#37512 removed from the non-spec branches, kept
alive here only because spec was refused on unified pools. At page
size 1 that rectangle reserves far more slots than the pool has
tokens.
* No widening contract. The builders already widen their own lens
(`paged_kernel_lens + draft_token_num`, per-row for DFlash's ragged
layout) and now hand those widened lens straight to the translator.
There is no separate table whose live prefix could fall short, so
`seq_len_delta` is not part of this path at all.
* No id-space plumbing. `sliding_window=` selects the swa sub-pool's
own id space, mirroring the non-spec branches, instead of re-aiming
a table at it.
Behaviour on a plain pool is unchanged by construction: with
`reads_are_translated` false, `fill_packed_read_stream` runs the same
`create_flashinfer_kv_indices_triton` launch over the same
`req_to_token` at `ENTRY_PAGE_SIZE=1` -- the call these builders made
by hand.
layers/attention/aiter_backend.py: its three updaters bind
`kv_index_translator` beside the `req_to_token` they already hold. aiter
is absent from the spec-verify allow-list, so it only ever sees the
non-translating branch -- the bind lets it satisfy the signature, not a
claim that it translates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
caihuali95
pushed a commit
to caihuali95/sglang
that referenced
this pull request
Sep 9, 2026
The four spec-input CSR builders took a raw `(req_pool_indices,
req_to_token)` pair and ran `create_flashinfer_kv_indices_triton`
themselves, so a translating batch had no way to reach them: the ids in
`req_to_token` are VIRTUAL under the unified pool, and nothing in that
argument pair can say so.
They now take the `KVIndexTranslator` and gather through
`fill_packed_read_stream`, which is what upstream's non-spec branches in
these same functions already do. One call replaces the hand-rolled
launch:
translator.fill_packed_read_stream(
req_pool_indices=req_pool_indices,
seq_lens=paged_kernel_lens, # widened just above
indptr=cum_kv_seq_len,
total_tokens=total_tokens,
out=kv_indices,
)
Three things fall out of using it rather than a page-table rectangle:
* No rectangle. The old path built a `[bs, max_pages]` table and
repacked it into the CSR stream the wrapper actually plans over --
the exact pattern sgl-project#37512 removed from the non-spec branches, kept
alive here only because spec was refused on unified pools. At page
size 1 that rectangle reserves far more slots than the pool has
tokens.
* No widening contract. The builders already widen their own lens
(`paged_kernel_lens + draft_token_num`, per-row for DFlash's ragged
layout) and now hand those widened lens straight to the translator.
There is no separate table whose live prefix could fall short, so
`seq_len_delta` is not part of this path at all.
* No id-space plumbing. `sliding_window=` selects the swa sub-pool's
own id space, mirroring the non-spec branches, instead of re-aiming
a table at it.
Behaviour on a plain pool is unchanged by construction: with
`reads_are_translated` false, `fill_packed_read_stream` runs the same
`create_flashinfer_kv_indices_triton` launch over the same
`req_to_token` at `ENTRY_PAGE_SIZE=1` -- the call these builders made
by hand.
layers/attention/aiter_backend.py: its three updaters bind
`kv_index_translator` beside the `req_to_token` they already hold. aiter
is absent from the spec-verify allow-list, so it only ever sees the
non-translating branch -- the bind lets it satisfy the signature, not a
claim that it translates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
caihuali95
pushed a commit
to caihuali95/sglang
that referenced
this pull request
Sep 9, 2026
The four spec-input CSR builders took a raw `(req_pool_indices,
req_to_token)` pair and ran `create_flashinfer_kv_indices_triton`
themselves, so a translating batch had no way to reach them: the ids in
`req_to_token` are VIRTUAL under the unified pool, and nothing in that
argument pair can say so.
They now take the `KVIndexTranslator` and gather through
`fill_packed_read_stream`, which is what upstream's non-spec branches in
these same functions already do. One call replaces the hand-rolled
launch:
translator.fill_packed_read_stream(
req_pool_indices=req_pool_indices,
seq_lens=paged_kernel_lens, # widened just above
indptr=cum_kv_seq_len,
total_tokens=total_tokens,
out=kv_indices,
)
Three things fall out of using it rather than a page-table rectangle:
* No rectangle. The old path built a `[bs, max_pages]` table and
repacked it into the CSR stream the wrapper actually plans over --
the exact pattern sgl-project#37512 removed from the non-spec branches, kept
alive here only because spec was refused on unified pools. At page
size 1 that rectangle reserves far more slots than the pool has
tokens.
* No widening contract. The builders already widen their own lens
(`paged_kernel_lens + draft_token_num`, per-row for DFlash's ragged
layout) and now hand those widened lens straight to the translator.
There is no separate table whose live prefix could fall short, so
`seq_len_delta` is not part of this path at all.
* No id-space plumbing. `sliding_window=` selects the swa sub-pool's
own id space, mirroring the non-spec branches, instead of re-aiming
a table at it.
Behaviour on a plain pool is unchanged by construction: with
`reads_are_translated` false, `fill_packed_read_stream` runs the same
`create_flashinfer_kv_indices_triton` launch over the same
`req_to_token` at `ENTRY_PAGE_SIZE=1` -- the call these builders made
by hand.
layers/attention/aiter_backend.py: its three updaters bind
`kv_index_translator` beside the `req_to_token` they already hold. aiter
is absent from the spec-verify allow-list, so it only ever sees the
non-translating branch -- the bind lets it satisfy the signature, not a
claim that it translates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
caihuali95
pushed a commit
to caihuali95/sglang
that referenced
this pull request
Sep 9, 2026
The four spec-input CSR builders took a raw `(req_pool_indices,
req_to_token)` pair and ran `create_flashinfer_kv_indices_triton`
themselves, so a translating batch had no way to reach them: the ids in
`req_to_token` are VIRTUAL under the unified pool, and nothing in that
argument pair can say so.
They now take the `KVIndexTranslator` and gather through
`fill_packed_read_stream`, which is what upstream's non-spec branches in
these same functions already do. One call replaces the hand-rolled
launch:
translator.fill_packed_read_stream(
req_pool_indices=req_pool_indices,
seq_lens=paged_kernel_lens, # widened just above
indptr=cum_kv_seq_len,
total_tokens=total_tokens,
out=kv_indices,
)
Three things fall out of using it rather than a page-table rectangle:
* No rectangle. The old path built a `[bs, max_pages]` table and
repacked it into the CSR stream the wrapper actually plans over --
the exact pattern sgl-project#37512 removed from the non-spec branches, kept
alive here only because spec was refused on unified pools. At page
size 1 that rectangle reserves far more slots than the pool has
tokens.
* No widening contract. The builders already widen their own lens
(`paged_kernel_lens + draft_token_num`, per-row for DFlash's ragged
layout) and now hand those widened lens straight to the translator.
There is no separate table whose live prefix could fall short, so
`seq_len_delta` is not part of this path at all.
* No id-space plumbing. `sliding_window=` selects the swa sub-pool's
own id space, mirroring the non-spec branches, instead of re-aiming
a table at it.
Behaviour on a plain pool is unchanged by construction: with
`reads_are_translated` false, `fill_packed_read_stream` runs the same
`create_flashinfer_kv_indices_triton` launch over the same
`req_to_token` at `ENTRY_PAGE_SIZE=1` -- the call these builders made
by hand.
layers/attention/aiter_backend.py: its three updaters bind
`kv_index_translator` beside the `req_to_token` they already hold. aiter
is absent from the spec-verify allow-list, so it only ever sees the
non-translating branch -- the bind lets it satisfy the signature, not a
claim that it translates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
caihuali95
pushed a commit
to caihuali95/sglang
that referenced
this pull request
Sep 10, 2026
The four spec-input CSR builders took a raw `(req_pool_indices,
req_to_token)` pair and ran `create_flashinfer_kv_indices_triton`
themselves, so a translating batch had no way to reach them: the ids in
`req_to_token` are VIRTUAL under the unified pool, and nothing in that
argument pair can say so.
They now take the `KVIndexTranslator` and gather through
`fill_packed_read_stream`, which is what upstream's non-spec branches in
these same functions already do. One call replaces the hand-rolled
launch:
translator.fill_packed_read_stream(
req_pool_indices=req_pool_indices,
seq_lens=paged_kernel_lens, # widened just above
indptr=cum_kv_seq_len,
total_tokens=total_tokens,
out=kv_indices,
)
Three things fall out of using it rather than a page-table rectangle:
* No rectangle. The old path built a `[bs, max_pages]` table and
repacked it into the CSR stream the wrapper actually plans over --
the exact pattern sgl-project#37512 removed from the non-spec branches, kept
alive here only because spec was refused on unified pools. At page
size 1 that rectangle reserves far more slots than the pool has
tokens.
* No widening contract. The builders already widen their own lens
(`paged_kernel_lens + draft_token_num`, per-row for DFlash's ragged
layout) and now hand those widened lens straight to the translator.
There is no separate table whose live prefix could fall short, so
`seq_len_delta` is not part of this path at all.
* No id-space plumbing. `sliding_window=` selects the swa sub-pool's
own id space, mirroring the non-spec branches, instead of re-aiming
a table at it.
Behaviour on a plain pool is unchanged by construction: with
`reads_are_translated` false, `fill_packed_read_stream` runs the same
`create_flashinfer_kv_indices_triton` launch over the same
`req_to_token` at `ENTRY_PAGE_SIZE=1` -- the call these builders made
by hand.
layers/attention/aiter_backend.py: its three updaters bind
`kv_index_translator` beside the `req_to_token` they already hold. aiter
is absent from the spec-verify allow-list, so it only ever sees the
non-translating branch -- the bind lets it satisfy the signature, not a
claim that it translates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Motivation
Three of the seven backends that read the unified pool never wanted a page table. They built a
[max_bs, max_pages]rectangle and handed it straight tocreate_flashinfer_kv_indices_tritonto repack into the indptr-addressed stream a paged wrapper plans over; no attention kernel ever read the rectangle.A rectangle's width is bounded by
max_context_len, because any single request could be that long and every row must be as wide as the widest. At ps=1, max_bs 256 and a 1M context that reserves 268M slots against a pool that holds 8.15M tokens — 32x more than any reachable state. The stream holds one id per resident token, so its length issum(seq_lens), which the pool bounds exactly.Modifications
One gather-and-translate, two delivery forms:
The rule that comes out of it is stateable: a consumer that plans over a stream calls
fill_packed_read_stream(flashinfer_mla, flashinfer, triton); a consumer whose kernel reads a page table fills the table it already owns, viafill_read_table(flashattention, trtllm_mha, trtllm_mla, flashmla).@triton.jit, differing by two constexpr:row_startsnull means a uniform stride, andEMIT_PER_TOKENselects page or token granularity. The CPU reference paths share_entriesfor the same reason.kv_viewwas threaded through five frames of each flashinfer updater chain; the decode chains never needed the real pool indices (a rectangle is indexed by batch lane, so its row ids werearange(bs)) and now take them instead.page_tableis explicitly not written under unified (skip_page_table=unified) whilekv_read_tablesis what gets bound, so one of its two identically-shaped buffers was dead. Both now match their siblings, andmake_capture_tablesis deleted.normal_decode_set_metadatagainsSKIP_PAGE_TABLE, a constexpr early-out after the prefix sum, so the translator can write fa3's page tables directly. Its now-unreachablesrc_is_read_tablepath goes with it: -41 lines, one mechanism in the file instead of two.KVIndexTranslator.reads_are_translated(unified, minus the DCP defer), which callers need before picking a source, andfill_packed_read_stream(sliding_window=...), selecting the swa sub-pool's own v2p.seq_lens; fa3's fused kernel and trtllm_mha's attention kernels bound their reads bycache_seqlens=seq_lens + deltaon a draft decode or target verify. The columns in between were read and never translated. A comment claimed this was unreachable — "Spec is asserted off under the unified pool" — butkv_cache_hookallows--speculative-algorithm DSPARK, a chain draft with topk 1, which is exactly that branch. Both now fill tometadata.cache_seqlens_int32.fill_read_tableassertedis_translating, butbuild_index_tablereturns the passthrough without touchingintowhen reads are deferred, which is what DCP does. trtllm_mla returns early ondcp_enabled; flashmla does not, and itsblock_kv_indicesis a reused buffer, so unified + DCP + flashmla would have read the previous batch's indices. The precondition is nowreads_are_translated, so that combination fails by name.Accuracy Tests
Branch vs base, GSM8K over 400 unless noted:
--dcp-size 2Means match on the Falcon and gpt-oss rows; the Kimi rows sit inside this box's ±2 question band, which three runs of one unchanged server reproduce. Two rows carry their own weight: the DCP row is the one path where
fill_packed_read_stream's return value is load-bearing (the stream comes back deliberately virtual and the caller finishes it), and the gpt-oss row is the sliding-window stream, which nothing else here exercises.On the fa3 row: at three runs a side the two bands looked disjoint (base 0.7625–0.7800, branch 0.7825–0.7975). They are not — the base's own spread is 11 questions over five runs.
Kernel level: the stream builder is bit-identical to the two steps it replaces over 40 randomized cases (page_size 1..4, mixed
seq_lens,-1holes, tombstoned v2p rows, half the trials drivingkv_start_idx). A CPU test pinspacked[indptr[b] + p] == ids[b, p // ps] * ps + p % ps, so a change to either builder alone turns red.test/registered/unit/mem_cache/: 2127 passed.Speed Tests and Profiling
This PR buys memory, not speed. Kimi-Linear TP2, 1M context,
cuda_graph_max_bs256:Throughput is unchanged, and that is the honest framing: the repack this removes costs ~5.8us per step against ~780ms of decode, two orders of magnitude under the run-to-run spread. (At kernel level the fused build is 2.1–6.5x the two steps it replaces; that ratio does not survive contact with a decode loop.)
A caveat on the memory figures: they come from the
CUDA graph end ... mem usagelog line, which isempty_cache()+mem_get_info()— true device-free memory, and therefore a lower bound rather than the allocation delta. On gpt-oss-20b it reports 0.67 GB on both sides whiletorch.cuda.memory_allocated()acrossinit_cuda_graph_statereportsbase +545.3 MiBvstip +289.3 MiB— 256.0 MiB, exactly the two rectangles. The caching allocator satisfies the request out of the block it already holds for the KV pool, so device-free does not move. Non-zero deltas are real; zero ones prove nothing.Also removed: one kernel launch per step per backend (the repack), and for fa3 the verbatim copy of the read table into
page_table.Checklist
On #35245
That PR introduced
KVIndexTranslatorto give id-space translation a single owner, after every backend reached for the hook itself throughgetattr(allocator, "translate_kv_loc_for_kernel", None) or getattr(...). That design is kept in full — no backend here gains a v2p read or a translate hook;grepforv2p_page_table|kernel_page_multiplier|translate_kv_loc_for_kerneloutsidemem_cache/still returns nothing. The converted call sites know strictly less than before: they used to drive the repack kernel themselves with four fields of the table's internal layout (ids,row_ids,row_stride,entry_page_size), and now hand over a buffer and get back a bool.What this PR replaces is that PR's choice of a shared, memoized rectangle as the universal intermediate. Those two are in tension — a rectangle is shareable precisely because it is consumer-independent, while a stream's layout is that consumer's indptr — and the trade resolves against the cache at every batch size, because the thing being cached is cheaper than the thing the cache forces afterwards:
2.83N < 2.66 + 5.64Nfor every N.Known gap
trtllm_mha has no end-to-end run behind it. No configuration on this box reaches trtllm_mha with unified memory: Falcon-H1 OOMs in
forward_extendand gemma-4 is rejected forhead_dim: 512. Its change is a mechanical swap onto the API its two MLA siblings already use and the unit suite passes, but it wants a reviewer with the hardware.Cross-shape validation
Benchmarked against the static pool across four hybrid shapes -- same build, only
--enable-unified-memorydiffering. Schedulable KV, accuracy and throughput allmatch; graph memory moves in both directions.
Inkling at TP4 is identical down to the token:
max_total_num_tokens=450605andmax_mamba_cache_size=207on both sides, from the samerest=4.962 GBbudget.Kimi at TP2 likewise,
4536315both sides. At--mem-fraction-static 0.9bothalso boot with
74778tokens, so unified needs no extra headroom.Accuracy is clean throughout -- the worst gap is gemma's 2.3 questions per 200,
against 1 sigma of 0.0236, i.e. 0.49 sigma. Kimi's +1.01 GB of graph memory is
not the index buffers: probing
init_cuda_graph_statereports +1025.0 MiB onboth sides, so that 1.0 GiB
cuda_graph_kv_indicesis a static-pool cost too.One methodology note, because the first pass got it wrong: a multi-GPU A/B run
side by side on two GPU sets is not a valid comparison on a shared box. The TP
pool is sized from the minimum free memory across ranks, so one foreign 1.2 GB
process on one card of one set shrinks that whole side's KV -- it read as a 22.5%
Inkling regression and a 1.57% Kimi one, both of which vanish when the two sides
run sequentially on the same cards.
Follow-ups
cuda_graph_kv_indicesis still sizedmax_bs * max_context_len; the stream'slength is bounded by the pool, so it can be ~32.6 MB instead of 1.07 GB.
CI States
Latest PR Test (Base): 🚫 Run #33697241821
Latest PR Test (Extra): 🚫 Run #33697241692
Latest PR Test (AMD ROCm 7.2): ❌ Run #33697241950