Repository navigation
perf: skip repeated speculative signals in SGLang committed requests - #638
Conversation
…ted requests A speculative decode burst emits consecutive token signals for one request, so the pass extended committed_requests with one UUID per token and sorted ~2.5x more entries than it had requests under MTP. Skip a signal whose UUID matches the last pushed entry, as vLLM does; the final sort and dedup keep the result sorted and unique. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Signal scanning and regression test crates/core/src/engine/scheduler/sglang/core.rs, crates/core/src/engine/scheduler/sglang/tests.rs |
Decode signal scanning avoids appending a UUID that matches the last committed UUID. The regression test checks that three requests emit three tokens each in submission order and that committed requests are sorted by UUID. |
Priority:
Estimated code review effort: 2 (Simple) | ~8 minutes
Merge Risk: 
1253d
-
The change is mergeable: committed-request behavior remains unchanged while repeated adjacent signals avoid redundant collection work.
Pre-merge checks: 
-
Check name Status Explanation Title check The title precisely states that repeated speculative signals are skipped when collecting SGLang committed requests. Description check The description explains the problem, behavioral change, risk, affected code, tests, benchmark results, identity validation, and related issue. The template fields for Fast CI, Full CI, reviewed commi… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Cross-Layer Contract PASS: The PR changes only internal SGLang collection before the existing sort_unstable/dedup; the finalEnginePassResult.committed_requestsoutput remains sorted and unique.rank::split_passf…Modeling And Data Evidence The PR provides provenance and reproducible before/after evidence. The description identifies the Tyche Grace node, DeepSeek-V3.2, SGLang 0.5.14, topology, workload, concurrency, pinned cores, and thr… Compatibility Boundaries The PR changes only crates/core/src/engine/scheduler/sglang/core.rsand its Rust tests. The publicPassStartEffects::committed_requests: Vec<Uuid>surface is unchanged. The decode path still sorts…Review Evidence The description names the relevant command, cargo test -p aisimulate-core --lib --test engine --test runtime_contract, and states the expected committed-request result. It includes boundary coverage…Full details: Description check
-
Explanation
The description explains the problem, behavioral change, risk, affected code, tests, benchmark results, identity validation, and related issue. The template fields for Fast CI, Full CI, reviewed commits, negative or boundary cases, and compatibility or rollback concern are not explicitly completed, but the description is otherwise substantially complete.
Full details: Cross-Layer Contract
-
Explanation
PASS: The PR changes only internal SGLang collection before the existing
sort_unstable/dedup; the finalEnginePassResult.committed_requestsoutput remains sorted and unique.rank::split_passforwards that unchanged toPassStartEffects(crates/core/src/engine/scheduler/rank.rs:634-670), so no public field, schema, default, or serialization shape changed. Repository-wide search found no Python, CLI, documentation, or serialization consumer forcommitted_requests. The added speculative-burst test exercises repeated signals and checks the public result (crates/core/src/engine/scheduler/sglang/tests.rs:382-412). The linked Dynamo dispatcher consumes admissions and KV events, not this field (ai-dynamo/dynamo commit 8fe088dc54f8667ad69aaeee6e6cdd1a26ad8972, lib/mocker/src/grouped_scheduler/dispatch.rs:84-87).
Full details: Modeling And Data Evidence
-
Explanation
The PR provides provenance and reproducible before/after evidence. The description identifies the Tyche Grace node, DeepSeek-V3.2, SGLang 0.5.14, topology, workload, concurrency, pinned cores, and three rotated rounds. It reports before/after replay medians, a no-speculation control, and prediction.json identity checks across both workloads and every round. The changed selection logic also has a deterministic regression oracle in
crates/core/src/engine/scheduler/sglang/tests.rs:387-413, which checks the emitted burst sequence and the sorted unique committed UUIDs. The implementation retains final sort and deduplication atcrates/core/src/engine/scheduler/sglang/core.rs:1212-1221. No unexplained model-data refresh or binary-only validation is present.
Full details: Compatibility Boundaries
-
Explanation
The PR changes only
crates/core/src/engine/scheduler/sglang/core.rsand its Rust tests. The publicPassStartEffects::committed_requests: Vec<Uuid>surface is unchanged. The decode path still sorts and deduplicates the same UUID set after collecting signals, so the optimization does not change the public result. No Python, schema, binding, version, package, wheel, or crate artifact files changed. The SGLang code remains within AISimulate, with no Dynamo dependency introduced.
Full details: Review Evidence
-
Explanation
The description names the relevant command,
cargo test -p aisimulate-core --lib --test engine --test runtime_contract, and states the expected committed-request result. It includes boundary coverage for reverse UUID order and three-token speculative bursts, plus a no-speculation control. It separates local tests, the Tyche Grace production benchmark, andprediction.jsonparity evidence; it makes no fake-runner or hosted-CI claim. The diff confirms the targeted test and decode-path change incrates/core/src/engine/scheduler/sglang/{tests.rs,core.rs}. No inactive nested workflow is cited as hosted CI evidence.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Use literal UUIDs and expected vectors instead of building them with iterator chains. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tedzhouhk
left a comment
tedzhouhk
left a comment
There was a problem hiding this comment.
Reviewed committed-request collection and downstream semantics. Clean local builds passed 118 SGLang scheduler tests and 104 engine/runtime-contract tests. An independent 168-case replay comparison matched the parent exactly, including per-request results and timing calls; an exhaustive 727,831-case deduplication probe also preserved results. No actionable correctness findings; current-head Full CI is green. Author-reported performance benchmarks were not rerun.
…i-dynamo#638) * perf: skip repeated speculative signals when collecting SGLang committed requests A speculative decode burst emits consecutive token signals for one request, so the pass extended committed_requests with one UUID per token and sorted ~2.5x more entries than it had requests under MTP. Skip a signal whose UUID matches the last pushed entry, as vLLM does; the final sort and dedup keep the result sorted and unique. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: shorten the SGLang speculative committed-requests test Use literal UUIDs and expected vectors instead of building them with iterator chains. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Why and what changed
In SGLang speculative replay, collecting
committed_requestsinSglangCore::try_execute_pass_internalcost ~18% of replay. On every decode pass, the code added one UUID per emitted token, then sorted and deduplicated the list. A speculative burst emits consecutive signals for the same request. Under MTP (~2.5 tokens per request per round), the pass therefore sorted ~2.5x more entries than it had requests.The pass now skips a signal whose UUID equals the last pushed entry, as vLLM does. The decode step emits each request's burst contiguously, so this leaves one entry per request. The final sort and dedup remain, so the result is still sorted and unique.
Review map
try_execute_pass_internalincrates/core/src/engine/scheduler/sglang/core.rs.Evidence
cargo test -p aisimulate-core --lib --test engine --test runtime_contract. The newspeculative_decode_commits_each_request_once_in_uuid_ordertest runs three requests in reverse UUID order, each emitting a 3-token burst. It checks for one sorted entry per request.wall_time_msof 3 rotated rounds.prediction.jsonmatches main, excluding host timing fields, for both workloads in every round.Modeling or data provenance
N/A
Tracking
🤖 Generated with Claude Code