Skip to content

perf(kv-router): append decode stores at the leaf tail under one lock - #18108

Merged
jthomson04 merged 3 commits into
mainfrom
jthomson04/crtc-decode-append
Nov 5, 2026
Merged

jthomson04 merged 3 commits into
mainfrom
jthomson04/crtc-decode-append

Conversation

@jthomson04

@jthomson04 jthomson04 commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Most KV events the router applies are one-block decode stores whose parent is the tail of the worker's own leaf: about 80% of events in the selection replay's agentx workload and 87% in its chat workload. Each went through parent planning: an EdgeIndex probe for the parent, then four more shape-gate round trips before try_extend_leaf_with_version appended. Now, when the worker's lookup entry for the parent names a leaf that ends with the parent and the worker covers that tail, the store is checked and appended under one exclusive shape gate, and the new lookup entries reuse the lane's node id. Every other store takes the existing path unchanged. Removal run grouping also prefetches lookup slots ahead, as upserts and deletes already do.

base this PR
Selection replay, agentx at saturation: index cycles per applied event 12,672 9,874 (−22%)
Events applied/s (4 event threads) 957k 1,134k (+18%)
Index backlog at window end 4.83M events 97k events
Selection replay, chat at saturation: index cycles per applied event 13,352 11,740 (−12%)
mooncake_bench write capacity 88.5 M ops/s +1.6% [−1.7, +3.4]
mooncake_bench keep-up find_matches and update p50/p99 inside noise

Tyche Grace node. Selection replay: selection_replay_bench (#17959) at saturation, agentx 3 alternating runs per arm and chat 1 run per arm, with cycles from window-bracketed profiles. mooncake_bench: 6 paired rounds, glibc, 95% bootstrap CIs.

Both arms run at about 12.1k req/s, the scheduler actor's ceiling. With this change the 4 event threads nearly keep up at that rate, instead of falling millions of events behind.

Key Changes

  • Node::try_append_at_tail: the decode-extension checks (not internal, not an anchor, tail is the parent, worker covers the tail) and the append, under one exclusive gate.
  • apply_stored tries it first, through the lane's entry for the parent (LaneLookup::held, borrowed, no Arc clone), and points the new entries at that node id (LaneLookup::upsert_all_held).
  • BlockLookup::leading_holding replaces the unprefetched get loop in LaneLookup::leading_naming.
  • README (Decode Extension) and tests: the anchor and uncovered-tail parents the one-lock path must decline (no existing test covers those guards), and the existing decode-extension test now asserts the stores take that path.

This adds a commit path that relies on the exclusive gate instead of a shape version. The leaf-extension conditions, including sticky internal, are unchanged.

Validation

  • cargo test -p dynamo-kv-router --lib
  • cargo clippy -p dynamo-kv-router --all-targets -- -D warnings, with and without --features bench

Issues and Ongoing Work

Pull Request Checklist
  • I used Dynamo's /pr-checklist skill.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Performance

    • Eligible decode stores can append blocks directly to a leaf’s existing edge, reducing reliance on the general processing path.
    • Successful appends update lookup entries and lifecycle tracking.
  • Bug Fixes

    • Stores with outdated parent references continue through the existing repair path before later appends.
    • Anchors and internal nodes receive new data as child nodes rather than being extended.
    • Stores that cannot find the required parent leave the existing edge unchanged and report the missing parent.

Most KV events are one-block decode stores whose parent is the tail of the
worker's own leaf. Each went through parent planning: an EdgeIndex probe for
the parent, then four more shape-gate round trips before the append, an Arc
clone, and a node-address lookup for the new entries. When the worker's
lookup entry for the parent names a leaf that ends with it and the worker
covers that tail, append there under one exclusive gate and point the new
entries at the held node id. Every other store takes the general path
unchanged.

Removal run grouping now prefetches lookup slots ahead, as upserts and
deletes already do.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jthomson04
jthomson04 requested a review from a team as a code owner November 4, 2026 19:40
@jthomson04 jthomson04 added the perf label Nov 4, 2026
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Nov 4, 2026
@jthomson04

Copy link
Copy Markdown
Contributor Author

/ok to test 51f7b17

@jthomson04

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Stores now attempt to append blocks directly to an eligible compressed leaf tail when the worker’s lookup holds the parent. The change adds lookup support for this path, preserves the existing insertion flow when it does not apply, and adds tests for success and fallback cases.

Changes

Tail append store path

Layer / File(s) Summary
Lookup access and prefix operations
lib/kv-router/src/indexer/concurrent_radix_tree_compressed/block_lookup.rs, lib/kv-router/src/indexer/concurrent_radix_tree_compressed/lane_lookup.rs
BlockLookup::leading_holding counts the matching prefix. LaneLookup::held returns a node ID and borrowed node reference, and upsert_all_held updates entries using that ID. Randomized tests exercise the new lookup operations.
Store tail-append path
lib/kv-router/src/indexer/concurrent_radix_tree_compressed/node.rs, lib/kv-router/src/indexer/concurrent_radix_tree_compressed/store.rs, lib/kv-router/src/indexer/concurrent_radix_tree_compressed/mod.rs, lib/kv-router/src/indexer/concurrent_radix_tree_compressed/tests.rs, lib/kv-router/src/indexer/concurrent_radix_tree_compressed/README.md
apply_stored tries to append blocks to a held parent’s leaf tail. Successful appends update lookup entries and lifecycle hashes. Anchors, internal nodes, uncovered tails, and stale entries use the existing insertion flow. Tests cover these cases, and the README describes the lookup-first path.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: Minimal · up to 51f7b

No actionable merge-blocking risk was found in the tail-append change.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: optimizing decode stores by appending at the leaf tail under one lock.
Docstring Coverage Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description follows the template closely. It has Overview, Key Changes, Validation, and Issues and Ongoing Work sections, plus the checklist. The Overview gives the problem, the approach, and a be…
Full details: Docstring Coverage

Explanation

Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 10 functions across 6 files. (1 skipped: 1 unsupported.)

Full details: Description check

Explanation

The description follows the template closely. It has Overview, Key Changes, Validation, and Issues and Ongoing Work sections, plus the checklist. The Overview gives the problem, the approach, and a benchmark table with cycles, throughput, backlog, and write capacity figures. Key Changes lists each touched function and the test and README updates. Validation lists the cargo test and clippy commands with and without the bench feature. The only gap is the Issues section. It says 'Found with the selection replay benchmark in #17959' but does not contain a 'Closes #XXXX' issue link. The template marks that link as advisory today. The pull request checklist item is unchecked, and the template expects the author to run the /pr-checklist skill. The description is detailed and on-topic. Its missing elements are minor and non-critical.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 9d639c9 and 51f7b17.

Files selected for processing: 7
  • lib/kv-router/src/indexer/concurrent_radix_tree_compressed/README.md
  • lib/kv-router/src/indexer/concurrent_radix_tree_compressed/block_lookup.rs
  • lib/kv-router/src/indexer/concurrent_radix_tree_compressed/lane_lookup.rs
  • lib/kv-router/src/indexer/concurrent_radix_tree_compressed/mod.rs
  • lib/kv-router/src/indexer/concurrent_radix_tree_compressed/node.rs
  • lib/kv-router/src/indexer/concurrent_radix_tree_compressed/store.rs
  • lib/kv-router/src/indexer/concurrent_radix_tree_compressed/tests.rs

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review.

@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor
Action performed

Full review finished.

Main's tests already fail when the internal or tail-hash guard is removed,
or when removal grouping miscounts. Keep the anchor and uncovered-tail
tests, which nothing else covers, and assert the fast path is taken in the
existing decode-extension test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jthomson04

Copy link
Copy Markdown
Contributor Author

/ok to test b5acc50

@jthomson04
jthomson04 enabled auto-merge (squash) November 4, 2026 21:51
@jthomson04

Copy link
Copy Markdown
Contributor Author

/ok to test 28c7f64

@jthomson04
jthomson04 merged commit 3669b24 into main Nov 5, 2026
198 of 201 checks passed
@jthomson04
jthomson04 deleted the jthomson04/crtc-decode-append branch November 5, 2026 03:37
ishandhanani added a commit to ishandhanani/dynamo that referenced this pull request Nov 5, 2026
Fourth upstream merge for the split v2 PR: the five commits past
02331f0. Four conflicts, all from ai-dynamo#18009, placed in the owner crates;
upstream behavior whole, the stack's moves intact, no shims.

- ai-dynamo#18009 keep prepared rows when ranks are past the reject thresholds:
  is_worker_full(dp_size)/skips_full_workers in the kernel's filter.rs;
  the actor keeps prepared rows and the kernel's select_with_policy drops
  the rows of workers whose every rank is full before the policy adopts
  them (PreparedRows::retain in worker-selection; the policy recounts the
  best cached prefix from the rows it adopts and reports whether any is
  left); prepared rows carry dp_size (u16, so the 128-byte row holds) set
  in the kernel's CandidatePreparerExt::prepare; the three new kernel
  queue tests and the relocated test helpers updated.
- ai-dynamo#18108 CRTC decode stores appended at the leaf tail under one lock:
  lib/router/kv-indexer concurrent_radix_tree_compressed (auto-merged).
- ai-dynamo#16258 vLLM backend defaults for omitted token limits: lib/llm
  preprocessor, lib/sidecar/vllm, components, docs, tests (auto-merged).
- ai-dynamo#18152 planner ARIMA forecasts, ai-dynamo#16426 operator crash-loop: outside the
  router (auto-merged).

Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation perf size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants