Repository navigation
perf(router): reduce scheduler and dispatch overhead - #14666
Conversation
9829153 to
be5c46c
Compare
baf3a8c to
e3bc02d
Compare
be5c46c to
7c21112
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (13)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe scheduler now applies lifecycle changes synchronously, coalesces capacity rechecks, and reuses worker-load projection maps. Overlap data uses ChangesScheduler capacity and projection
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to No actionable correctness, availability, or integration risk was established, so the PR is ready for normal merge checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed overview, implementation details, reviewer entry points, and validation results. It does not include the required Related Issues section or confirm that no related issue exists.
Comment |
jthomson04
left a comment
There was a problem hiding this comment.
Code review
Found 15 issues (each expanded in an inline comment with its failure scenario and suggested fix). Reviewed at head 7c211122549949e84f0742a29f7ed1519a945f17 against base ae5ca99fba. Comments 11 and 12 are anchored to the nearest changed line because the flagged line is outside the diff.
- Admission ordering: the invariant that let
handle_enqueuedskip the blocked-lane recheck no longer holds
dynamo/lib/kv-router/src/scheduling/queue.rs
Lines 983 to 987 in 7c21112
ensure_running()gates a mutation that no longer needs the actor, making lifecycle accounting inconsistent if the actor dies
dynamo/lib/kv-router/src/scheduling/local.rs
Lines 642 to 644 in 7c21112
- Output-block updates now run the registry write path on every request task at every block boundary, and the PR states this was not benchmarked
dynamo/lib/kv-router/src/scheduling/local.rs
Lines 708 to 710 in 7c21112
handle_capacity_updateshas nohas_ready()early return, unlikehandle_update/handle_enqueued
dynamo/lib/kv-router/src/scheduling/queue.rs
Lines 1313 to 1316 in 7c21112
drain_cleanup()runs unconditionally on every capacity wake, bypassing the burst gate the command path uses
dynamo/lib/kv-router/src/scheduling/queue.rs
Lines 1319 to 1321 in 7c21112
- Unknown-worker ->
recheck_all_workersis a symptom patch; worker removal is not handled at its source
dynamo/lib/kv-router/src/scheduling/queue.rs
Lines 1323 to 1331 in 7c21112
- Reuse
worker_config_for_rankinstead of re-implementing the dp-rank range check
dynamo/lib/kv-router/src/scheduling/queue.rs
Lines 1323 to 1328 in 7c21112
lib/kv-router/src/sequences/AGENTS.mdrequires before/after benchmark numbers for prompt-registry projection changes
dynamo/lib/kv-router/src/sequences/prompt_registry.rs
Lines 301 to 308 in 7c21112
- The legacy
schedule*path now allocates twoFxHashMaps per call where it previously moved the caller's maps
dynamo/lib/kv-router/src/scheduling/local.rs
Lines 484 to 486 in 7c21112
- Dead indirection: three names now do the identical synchronous thing
dynamo/lib/kv-router/src/scheduling/local.rs
Lines 732 to 741 in 7c21112
- Redundant state:
capacity_updates.is_some()andqueueing_enabled(line 519) encode the same fact
dynamo/lib/kv-router/src/scheduling/queue.rs
Lines 506 to 508 in 7c21112
Update { worker: Some(..) }is now only reachable from#[cfg(test)] update_worker
dynamo/lib/kv-router/src/scheduling/queue.rs
Lines 155 to 158 in 7c21112
update_queue()is documented as waiting for a recheck but is a no-op when queueing is disabled, and has no production caller
dynamo/lib/kv-router/src/scheduling/local.rs
Lines 668 to 672 in 7c21112
#[inline(always)]added to already-inlinable private/generic fns with no selection-bench delta cited
dynamo/lib/kv-router/src/scheduling/selector/default.rs
Lines 253 to 255 in 7c21112
- The reused projection table never shrinks;
clear()is O(bucket count)
dynamo/lib/kv-router/src/sequences/prompt_registry.rs
Lines 307 to 310 in 7c21112
🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
55d56ab to
3cce7cb
Compare
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Incorporate the final change from PR #14545. Borrow the live discovery snapshot and clone only the selected instance, preserving transport fallback behavior and the existing lookup regression test. Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
3cce7cb to
f1ffe12
Compare
|
/ok to test f1ffe12 |
Summary
Reduce scheduler and dispatch work for requests served by large worker pools, with admission queueing disabled, enabled, or mixed across policy classes. Reuse load-projection storage, apply lifecycle state updates directly, and clone only the selected discovered instance during transport lookup.
Client::instance_by_id()borrows the current discovery snapshot and clones only the matching instance.PushRouter::resolve_transport()uses that lookup and preserves worker-removal, fallback, and error behavior.Selection, booking, queue order, fairness, and overlap refresh remain inside the actor. Attempt checks, replica publication, reservation cleanup, and booking ownership remain intact. Destructor cleanup, expiry, admission handoff, and failed-admission rollback retain their actor paths. Successful lifecycle calls confirm the state change and notification.
LocalScheduler::update_queue()waits for an explicit recheck when queueing is enabled and the actor is running; with queueing disabled it returns immediately without an admission barrier.No new dependencies, tasks, runtime settings, HTTP/Python interfaces, or wire changes. The current base already includes the numeric-map optimization and builtin worker-selection refactor. This PR preserves that implementation; it does not retain changes that only tuned the old reference selector.
Reviewer entry points
lib/kv-router/src/scheduling/local.rs: direct lifecycle updates andupdate_queue()timing.lib/kv-router/src/scheduling/queue.rs: serialized selection/booking, reusable projection storage, and capacity rechecks before draining admissions.lib/kv-router/src/scheduling/queue/capacity.rs: bounded worker hints, overflow fallback, and retained wakeups.lib/runtime/src/component/client.rsandlib/runtime/src/pipeline/network/egress/push_router.rs: selected-instance lookup and transport resolution.Validation
Rebased onto main
0fac8e85d0c1a27bbd6b6d75d386b757e7f6f322. Current combined head:3cce7cb547.standalone-selection,bench: 1,286 passed, 2 ignored.dynamo-llm --no-default-features --lib kv_router::): 389 passed.No new performance benchmark was run for this rebase or the combined change. The results below describe earlier scheduler revisions, before the instance-lookup change and the current main refactor. They do not measure the current combined head.
Review-fix benchmark on the same base
Control
7c21112254versus candidate55d56abf0c, both onae5ca99fba. Five Grace nodes, 16 workers with actual backlog, AgentX, concurrency 6,144, 120-second load, identical warmup/drain/policy and frozen mock-worker/client binaries. Separate frontend builds, verified live binary identities, and fresh processes each run.One control/candidate pair, followed by the permitted reverse-order pair to check the initial CPU increase:
The candidate had higher throughput and lower TTFT and post-first-token latency in both pairs. CPU per completion was higher in both pairs: +2.0% and +3.1%. This is a measured tradeoff, not a CPU saving.
The size of a sustained speedup and the cause of the CPU increase remain inconclusive. The two control runs differed by 25.9% in throughput. Request size and worker distribution also changed. Those observations do not prove the cause of the differences, and no slowdown is assigned to an external cause.
All 426,472 records have complete client coverage, with zero errors. All 16 workers were used in every run; long-queue peaks ranged from 1,365 to 1,503 per frontend. Cleanup passed on all five nodes after each run. Output tracking was disabled; no cancellation-heavy benchmark was added. The setup attempt that stopped before load is excluded from these results.
Earlier optimization benchmark evidence
The benchmark used base
4067d08f, step 12c483e0059, and both stepsda44ae41e1, before the rebases onto #14149 and main. These are measurements of the original implementation, not a new benchmark of the rebased head.Matched isolated builds and fresh processes on five Grace nodes: two frontend processes, AgentX workload, 2,048 workers with queueing off, and 16 workers with actual backlog for queueing on/mixed. One clean run per build/mode, separate diagnostic runs, and one extra queue-on baseline/full pair to check the initial regression. Binary identities and complete client-record coverage were verified; rare transport errors were accepted and retained in the artifacts.
The diagnostic runs show 11–27% less actor service time per enqueue on both frontends across all three modes, and roughly half the admission-channel traffic. Actor service is elapsed processing time, including overlap-refresh waits, rather than CPU time.
With queueing off, both steps improved throughput by 9.3%, reduced TTFT p95 by 62.7%, and reduced CPU per completed request by 1.9%; post-first-token p95 increased by 10.5%. The first queue-on slowdown did not repeat in the control pair. The results support the reduction in scheduler work, but run variation prevents a claim of a stable throughput gain in all modes or a confirmed external cause for the first slowdown. Output tracking was disabled in these workloads, so its direct-update performance was not measured.