Repository navigation
Conversation
The SGLang scheduler published a Stored event when it allocated prompt pages, and it inserted each decode page into the radix tree as soon as the page filled. At page size 1 a request therefore produced about one Stored event per output token, plus one Removed event for every decode step that evicted. SGLang v0.5.21 behaves differently: - it inserts a request's prompt into its radix tree after the prefill forward (`cache_unfinished_req`, once per chunk for chunked prefill). It does so whether or not the request's first decode slot then fits, and for a disaggregated prefill while its KV transfers. It inserts the rest when it releases the finished request (`release_kv_cache`), including a running request that an abort finished. Decoded tokens are neither matchable nor published before then, and retraction releases them without inserting them (`release_kv_cache(is_insert=False)`); - it queues events in the tree cache. A removal merges into a removal at the tail of the queue, and a store whose parent is the last block of the tail store extends that store; - it publishes the queue after every prefill batch, after every `--decode-log-interval` (default 40) decode batches, and when idle. The SGLang KV manager and scheduler now do the same. Allocation no longer publishes, and decode no longer inserts per token. The post-prefill insert covers the prompt only. It runs even when the first decode slots cannot be reserved, and also for requests that finish with their first token, which include disaggregated prefill sources. Cancelling a running request finishes it, so the KV it decoded is inserted. Events go through a queue that merges like SGLang's and assigns event IDs at publish time, and the queue is flushed at SGLang's publish points. Commands flush the queue when no request is running. Behavior changes in SGLang mode: - KV events are fewer and larger, and are published at the next publish point (up to 40 decode passes later) instead of immediately. Stored and removed block totals are unchanged. Pages that never enter the radix tree are no longer announced as stored or removed. - KV-aware routing sees a request's decoded blocks only when the request finishes or is cancelled, as with SGLang. - A disaggregated prefill source's prompt is inserted and published after its prefill pass. Before, it was published when its pages were allocated, and it entered the radix tree only when the source was released. - Decoded tokens of in-flight requests are no longer prefix-matchable by the simulator's own radix cache. - A retracted request discards its decoded KV and recomputes it when it is readmitted; only its prompt stays cached. - Simulated TTFT and throughput can therefore change under KV pressure. vLLM and TRT-LLM modes are unchanged. We recorded a multi-turn coding-agent workload on SGLang 0.5.21 at page size 1 (16 closed-loop lanes, 6,262 requests). Real SGLang emitted 2.00 Stored events per request at 370 blocks per event (median 98), and 0.49 Removed events per request at 1,375 blocks per event. Replaying the same schedule, the simulator went from 112.2 to 2.00 Stored events per request (6.6 to 371 blocks per event, median 98), and from 11.4 to 1.98 Removed events per request (59 to 341 blocks per event). A 4-lane run with a quarter of the KV capacity agrees on stores (real and simulated: 2.00 per request at 383 and 384 blocks per event). At page size 16, Stored events per request fell from 7.7 to 1.9. The removal gap that remains comes from eviction granularity. SGLang evicts whole radix leaves, while this cache evicts only the pages it needs. Evicting whole leaves in a local experiment brought the simulator to 0.47 Removed events per request at 1,437 blocks per event. That change alters cache retention, so it is left for a follow-up. Signed-off-by: PeaBrane <yanrpei@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (25)
🧰 Additional context used📚 Code guidelines (2)📓 Path-based instructions (4)Review scheduler state transitions and event ordering, including cancel, preempt, drain, retry, duplicate, empty, and terminal paths.⚙️ CodeRabbit configuration file Files:
Read REVIEW.md before commenting.⚙️ CodeRabbit configuration file Files:
Before making any change under: `python/aisimulate/src/aisimulate/generator/**` MUST read: `python/aisimulate/.claude/rules/generator-development.md` Source excerpt: Before making any change under `python/aisimulate/collector/**` MUST read:...📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: Only workflows under the repository-root `.github/workflows/` run for this repository.📄 CodeRabbit inference engine (REVIEW.md) Files:
🔀 Multi-repo context ai-dynamo/dynamo, ai-dynamo/aiconfiguratorLinked repositories findingsai-dynamo/dynamo
ai-dynamo/aiconfigurator
🔇 Additional comments (5)
📝 SummaryRisk level: Medium. Three areas need human attention:
Behavior and contracts SGLang mode now queues KV events and publishes them at SGLang insert points. Evidence supplied The PR objectives report that workspace tests passed (2,057 passed, 1 ignored) and formatting checks passed. Clippy reported 158 existing findings, with none added. Named regression tests cover prefill caching, held disaggregated prefill sources, cancellation, event placement, and batched removals. The author also reports cadence replays and a synthetic workload check. The objectives report fewer Stored and Removed events in a 16-lane, page-size-1 replay, with unchanged stored and removed block totals and simulated run time across eight runs. They also report that TTFT and throughput can change under KV pressure. These are author-reported results; no independent measurements are supplied here. Evidence missing No current review findings or severity counts were supplied. The actual test output, replay data, and source diff were not supplied for independent verification. Technical quality and merge readiness The supplied test and replay claims support the intended behavior, but they do not establish independent verification. Assess merge readiness against the full diff and test evidence, with particular attention to flush timing and cancellation/retraction behavior. Do not treat automated review as approval. WalkthroughSGLang KV events are queued and published at scheduler-controlled points. The scheduler now handles running-request cancellation through completed-request cleanup. Prefill caches prompt prefixes before decode-page reservation, while generated tokens enter the radix tree when a request finishes or is cancelled. ChangesSGLang KV event flow
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to No actionable issue remains from this review; the change is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 6 | ❌ 1 | ❓ 1❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (6 passed)
Full details: Cross-Layer ContractExplanation The PR adds a new compatible- Resolution Add a focused backend test that queues two adjacent Full details: Review EvidenceExplanation The supplied PR description is truncated before the validation section. The available metadata reports
Comment |
|
Superseded by #583, which now contains this change together with the related SGLang cache lifecycle, eviction, reporting, and KV-event corrections. The consolidated branch has passed local core and public-API validation. |
Summary
SGLang mode published KV events at the wrong points and far too often. It published a Stored event as soon as it allocated prompt pages, before the prompt was computed. It also inserted each decode page into the radix tree as soon as the page filled. At page size 1, a request therefore produced about one Stored event per output token, plus one Removed event for every decode step that evicted. That is roughly 50 times more events than a real SGLang worker sends to a KV-aware router.
This PR changes SGLang mode to behave like SGLang v0.5.21:
cache_unfinished_req, once per chunk for chunked prefill). This happens even when the first decode slots cannot be reserved, and for a disaggregated prefill whose KV is still transferring. The rest of the sequence is inserted when the finished request is released (release_kv_cache). Cancelling a running request finishes it the same way, as an SGLang abort does. Retraction releases KV that was never inserted without inserting it (release_kv_cache(is_insert=False)).--decode-log-interval(40) decode passes. It is also flushed when the scheduler is idle, including after a command when no request is running.Behavior changes (SGLang mode only)
Measured cadence
Real SGLang setup:
--page-size 1, radix cache, chunked prefill 8192, ZMQ KV-event publisher.Simulator setup:
Page size 1, 16 lanes, 393,216-token KV, 6,262 requests
Page size 1, 4 lanes, 98,304-token KV, 6,258 requests
Page size 16 (simulator only; there is no real SGLang capture at this page size)
In all eight simulator runs, stored and removed block totals and total simulated run time were identical before and after.
Known follow-up
Removed events are still about four times too frequent:
The cause is eviction granularity. SGLang evicts whole radix leaves, while this cache evicts only the pages it needs. In a local experiment, evicting whole leaves brought the simulator to 0.47 Removed events per request at 1,437 blocks per event. That change alters cache retention, and with it hit rates and timing, so it is not part of this PR.
#363 moves SGLang mode onto SGLang's native radix core with whole-leaf LRU eviction, so it may close this gap. The two PRs touch the same files, and whichever lands second will need a rebase.
Validation
cargo test --workspace: 2,057 passed, 1 ignored.cargo fmt --all -- --check: passed.cargo clippy --workspace --all-targets -- -D warnings(Rust 1.96.1): reports the same 158 existing findings asmain. This PR adds none.prompt_is_cached_after_prefill_when_first_output_slots_do_not_fit: after a prefill whose first decode slots do not fit, the prompts are matchable, their Stored events are queued, and a request retracted at the next decode step keeps its prompt cached.held_source_prompt_is_cached_and_published_after_prefill: a disaggregated prefill source's prompt is published with its prefill pass and stays matchable while the source is held; releasing it publishes nothing more.cancelled_running_request_caches_its_decoded_tokens: cancelling a decoding request caches and publishes its full decoded pages.kv_events_store_prompt_after_prefill_and_outputs_at_finish(page sizes 1 and 4) andkv_event_removals_are_batched_like_sglangfix the expected events for each pass. Existing KV-manager tests now flush the event queue where they read events.crates/tests/public-api. This PR changes no public API.