Fix unified SWA: size a non-owner's v2p by the id space it must address - #37560
Merged
Merged
Conversation
ch-wan
requested review from
Ying1123,
alphabetc1,
hanming-lu,
hnyls2002,
huangtingwei9988,
hzh0425,
ispobock,
merrymercy,
xiezhq-hermann and
yizhang2077
as code owners
September 2, 2026 06:55
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. |
ch-wan
force-pushed
the
cheng/unified-swa-virtual-id-space
branch
from
September 2, 2026 07:05
ef68b57 to
e2319f2
Compare
ch-wan
requested review from
BBuf,
DarkSharpness,
Fridge003,
HaiShaw,
HydraQYH,
Qiaolin-Yu,
celve,
hebiao064 and
yuan-luo
as code owners
September 2, 2026 07:05
ch-wan
changed the base branch from
main
to
cheng/unified-read-path-audit-fixes
September 2, 2026 07:05
ch-wan
force-pushed
the
cheng/unified-swa-virtual-id-space
branch
2 times, most recently
from
September 2, 2026 08:05
74efcb4 to
a4e7526
Compare
This was referenced Sep 2, 2026
ch-wan
force-pushed
the
cheng/unified-read-path-audit-fixes
branch
from
September 2, 2026 08:28
62ba19a to
e848419
Compare
ch-wan
force-pushed
the
cheng/unified-swa-virtual-id-space
branch
2 times, most recently
from
September 2, 2026 09:14
c39e6d3 to
98729a0
Compare
ch-wan
force-pushed
the
cheng/unified-read-path-audit-fixes
branch
from
September 2, 2026 09:19
e848419 to
214eab1
Compare
ch-wan
force-pushed
the
cheng/unified-swa-virtual-id-space
branch
from
September 2, 2026 09:21
98729a0 to
bb3a21c
Compare
ch-wan
force-pushed
the
cheng/unified-read-path-audit-fixes
branch
from
September 2, 2026 09:26
214eab1 to
ee7ae56
Compare
ch-wan
force-pushed
the
cheng/unified-swa-virtual-id-space
branch
from
September 2, 2026 09:28
bb3a21c to
e8dbde6
Compare
ch-wan
force-pushed
the
cheng/unified-read-path-audit-fixes
branch
from
September 2, 2026 19:50
ee7ae56 to
d25053c
Compare
ch-wan
force-pushed
the
cheng/unified-swa-virtual-id-space
branch
from
September 2, 2026 19:51
e8dbde6 to
fb8af1e
Compare
ch-wan
force-pushed
the
cheng/unified-read-path-audit-fixes
branch
from
September 2, 2026 20:54
d25053c to
b6f551c
Compare
ch-wan
force-pushed
the
cheng/unified-swa-virtual-id-space
branch
from
September 2, 2026 20:54
fb8af1e to
035b88a
Compare
ch-wan
force-pushed
the
cheng/unified-read-path-audit-fixes
branch
from
September 2, 2026 23:55
b6f551c to
ceb4aa4
Compare
The unified SWA composite mints one virtual page id per allocation and binds it
on both sides -- `alloc` snapshots `full.free_virtual_ids[:n]` and hands those
same ids to `swa.alloc_with_virtual`. So the swa side's `virtual_to_physical` is
indexed by the FULL side's ids, while it was sized by the swa side's own page
count.
Those two numbers are unrelated. Which side gets more pages out of a shared byte
budget depends on the layer split, and a model with few full-attention layers and
many sliding ones gives the owner far more. gemma-4-31b-it is 10 full and 50
sliding, so at 0.8 mem-fraction on 2xH100:
full v2p len = 2,065,601
swa v2p len = 206,561 <- indexed by ids up to 2,065,600
The id space also rotates: ids come off the front of the owner's free list and
freed ids return to the back, so the cursor sweeps the whole range over time
even though only a fraction is ever live. The failure is therefore not immediate
but arrives after enough churn -- and on GPU it arrives as
`alloc_bind_inplace`'s `tl.store(v2p_ptr + v, p)` writing past the end of the
table. An unchecked write, so the symptom shows up later and elsewhere: the
read in `_swa_write_loc_unified` trips a device-side index assert, the scheduler
dies, and the request batch returns nothing.
`virtual_to_physical` is now sized by `num_virtual_ids` -- the owner's count,
passed down for a non-owner -- while `physical_to_virtual` keeps its own page
count, since that one is indexed by physical id. `is_slot_allocated`'s bound
moves to the virtual count for the same reason.
gemma-4-31b-it + unified + triton, TP2, GSM8K over 200, alternating a side per
round:
round 1 main 0.98 alive branch 0.98 alive
round 2 main 0.00 DEAD branch 0.98 alive
round 3 (main gone) branch 0.98 alive
rounds 4-6 branch 0.98, 0.98, 0.98
The same model on the same commit with the static pool scores 0.98, which is
what said the unified pool was at fault rather than the model.
The new test fails both ways round on main -- the width check on the (1, 5)
layer split, and the churn check with the `IndexError` the CPU reference path
raises where the Triton store would not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing in test/registered covered unified memory on a hybrid sliding-window
model. The existing unified e2e tests are MLA (Kimi-Linear) or ShortConv
(Inkling); the existing SWA e2e tests run the static pool. That missing cell is
why a config that dies inside one eval shipped.
google/gemma-4-E2B-it, 1 GPU, triton, --enable-unified-memory
--disable-radix-cache --mem-fraction-static 0.8 --max-total-tokens 60000
GSM8K over 200
narrow table 0.05 <- red on main
fixed passed <- 65 s wall clock
Every argument is load-bearing, and it took several wrong turns to establish
that:
- `--max-total-tokens 60000` is the trigger, not a convenience. The failure
needs cumulative churn past `swa.num_pages`; at this model's default budget
that table is 3.8M entries and it would take dozens of runs.
- the eval has to be this one. Synthetic short prompts at 3.7x the nominal
churn passed on main; the 5-shot GSM8K traffic is what reaches it.
- it cannot use `GSM8KMixin`: that path scores this model 0.155 whatever the
pool, because its chat template does not fit the model's reasoning config.
`run_eval` directly gives 0.87 on a healthy server.
Registered at `base-b` / `1-gpu-large` -- per-PR, not nightly. The lesson from
the last regression here (#37307) was that the guard existed but was
nightly-gated, so it never blocked the PR that broke it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rationale belongs in the PR body, not next to the line. Cut the parts that argue with a reviewer -- why the small pool is not a convenience, why the two page counts have no relationship to assert, which substitutions made the test pass on main -- and kept what the next editor cannot see from here: that v2p is indexed by virtual id and p2v by physical, that either side can be the larger, and that raising the pool size hides the defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ch-wan
force-pushed
the
cheng/unified-swa-virtual-id-space
branch
from
September 2, 2026 23:56
035b88a to
9d2cd53
Compare
Collaborator
Author
|
Latest PR Test (Base): ✅ Run #33682091141 |
StevenChenSE
pushed a commit
to StevenChenSE/sglang
that referenced
this pull request
Sep 6, 2026
…ss (sgl-project#37560) 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
--enable-unified-memoryon a hybrid sliding-window model dies mid-serving: a device-side index assert, the scheduler gone, and the request batch returning nothing. The same model at the same commit with the static pool is fine.The unified SWA composite mints one virtual page id per allocation and binds it on both sides:
So the swa side's
virtual_to_physicalis indexed by the full side's ids — while it was sized by the swa side's own page count. Those two numbers are unrelated: which side gets more pages out of a shared byte budget depends on its per-page byte cost.gemma-4-31b-itis 10 full-attention layers to 50 sliding, so at--mem-fraction-static 0.8on 2×H100:Two things make it hard to see. It needs cumulative allocation churn before an id that far up is handed out, so a fresh server passes its first eval and dies on the second. And on GPU the offending access is a write —
alloc_bind_inplace'stl.store(v2p_ptr + v, p), unchecked — so the visible symptom is somewhere else entirely: the later read in_swa_write_loc_unifiedis what trips the assert. UnderCUDA_LAUNCH_BLOCKING=1the stack resolves to that read; the CPU reference path raisesIndexErrorfrom the write.Modifications
MultiEndedAllocatortakesvirtual_num_pages.virtual_to_physicalis sized by it — the id owner's count, passed down for a non-owner — whilephysical_to_virtualkeeps this pool's own page count, because that one is indexed by physical id.is_slot_allocated's bound moves to the virtual count for the same reason.virtual_num_pages=self.full_attn_allocator.num_virtual_idswhen building the swa side (both the 2-pool end pair and the tri-pool float).test_unified_swa_shared_virtual_ids.py(CPU): the table spans the owner's id space across three layer splits, and alloc/free churn past the swa side's page count binds cleanly.test_gemma4_unified_swa_virtual_ids.py(1 GPU,base-b, est 65s): the end-to-end cell that was missing — no registered test ran unified memory on a hybrid-SWA model. The existing unified e2e tests are MLA or ShortConv; the existing SWA e2e tests run the static pool.Cost is one int64 row per id in the owner's space on the non-owner side: 16.5 MB at the gemma-4-31b shape above, against the 1.65 MB it had.
Accuracy Tests
The failure needs two conditions, and separating them took several wrong turns:
full.num_pages > swa.num_pages— a property of the model, true when the full side's per-page byte cost is the lower one. Readentry_bytes_per_page, not the layer counts: gemma-4-E2B is 7 full to 28 sliding but 2:1 in pages.swa.num_pages— a property of the KV budget, since only enough allocation brings an id that high into play.Shrinking the pool makes (2) immediate, which turns a 2B model on one GPU into a one-round repro — this is what the new e2e test runs:
GSM8K over 200, one GPU, 65 s wall clock:
Found in the wild on the shape where the swa table is small by itself —
gemma-4-31b-it, TP2, default budget, GSM8K over 200 alternating a side:For contrast, two hybrid-SWA models that do not reproduce it at their default budgets:
The unit test fails both ways round on
main: the width check on the (1, 5) split, and the churn check with theIndexErrorthe CPU reference path raises where the Triton store would not.test/registered/unit/mem_cache/: 2129 passed (this branch, after rebasing onto #37550 — both changes land in the samemulti_ended_allocator.py).Speed Tests and Profiling
Not a performance change. The extra table is 16.5 MB on the gemma-4-31b shape, allocated once at startup; nothing moves per step.
Since this is the top of the stack, it is also where "does unified cost anything against
the static pool" gets answered for the whole feature. Four hybrid shapes, same build,
only
--enable-unified-memorydiffering -- schedulable KV is identical, and accuracyand throughput are within run-to-run spread:
Inkling matches down to the budget it is derived from (
rest=4.962 GB,max_mamba_cache_size=207on both sides), and at--mem-fraction-static 0.9both sidesboot with
74778tokens, so unified needs no extra headroom. Graph memory moves in bothdirections; #37512 has that column and the breakdown.
Method, because the first pass got this wrong: run a multi-GPU A/B sequentially on the
same cards. The TP pool is sized from the minimum free memory across ranks, so one
foreign 1.2 GB process on one card of one GPU set shrinks that whole side's KV -- it read
as a 22.5% Inkling regression until both sides ran on the same four cards.
Checklist
main.Notes for reviewers
The e2e test's arguments are load-bearing. Three substitutions made it pass on
main, so they are not tuning:--max-total-tokenshides the defect — the small pool is condition (2).main; the 5-shot GSM8K traffic is what reaches it.GSM8KMixin: that path scores this model 0.155 whatever the pool, because its chat template does not fit the model's reasoning config. Callingrun_evalgives 0.87 on a healthy server.Registered at
base-b, not nightly. The lesson from the last regression in this area (#37307) was that the guard existed but was nightly-gated, so it never blocked the PR that broke it.One claim I could not confirm. I described the id space as rotating — ids off the front of the owner's free list, freed ids to the back — and the code does read that way (
torch.cat([free_virtual_ids, free_v_pages])). But probes onalloc()andalloc_with_virtualnever fired during a real server run: the free list is consumed inside the Triton kernel on thealloc_extendpath. So "churn advances the cursor" is the observed behaviour (the trigger is empirically cumulative, and the measured thresholds above bear that out), not a mechanism I have traced end to end.CI States
Latest PR Test (Base): ⏳ Run #33682091141
Latest PR Test (Extra): ❌ Run #33682090790
Latest PR Test (AMD ROCm 7.2): ⏳ Run #33682090991
CI States
Latest PR Test (Base): 🚫 Run #33697315211
Latest PR Test (Extra): 🚫 Run #33697315009
Latest PR Test (AMD ROCm 7.2): 🚫 Run #33697315235