Repository navigation
perf(kv-router): port #15611's CRTC writer per-block work onto main - #16134
Merged
Merged
Conversation
Contributor
✅ Dynamo PR CI passed — run 38291821485 (attempt 1) on
|
jthomson04
force-pushed
the
jthomson04/crtc-writer-lookup-port
branch
from
October 12, 2026 15:59
a01050a to
0da6e03
Compare
…ndex CRTC nodes now keep a module-private `CrtcNodeState` instead of the `compressed_radix::NodeState` shared with `RadixTree`. It holds the same worker-keyed coverage, plus an `EdgeIndex` in place of the per-node `FxHashMap` from hash to edge position: - Edges of at most 16 blocks keep no index and scan from the tail. - Longer edges keep an open-addressing table of `u32` positions that reads keys back through the edge. - A split truncates the prefix's table in O(1), leaving tombstones, and rebuilds only when a table half the size would fit. Split coverage handling moves from `Node::split_at_locked` into `CrtcNodeState::split_off_suffix`, unchanged. One intentional difference: when a split prefix repeats a hash that also appears in the suffix, the prefix keeps its own last position of that hash instead of dropping the hash, matching what a scan finds. Ported from #15611 (PeaBrane), without the slot coverage of #15610. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Each CRTC event lane kept a `FxHashMap<hash, Arc<Node>>` per worker, so every stored or removed block was an atomic read-modify-write on its node's strong count, on nodes that other lanes name too. `LaneLookup` replaces it: - `BlockLookup` is an open-addressing table that keeps each key with its value and prefetches a batch's slots ahead of use. - Entries name nodes by a lane-local `NodeId`. The lane holds one `Arc` per distinct node its entries name and counts those entries locally, so only the first entry naming a node and the last leaving it touch the node's strong count. - Stale-leaf cleanup is unchanged: a node any lane names still carries exactly one extra strong reference per lane. Redirects after splits and lookup repair keep #16032's semantics: only entries that still name the old node move. A split also skips the lookup update when the lane names no entry on the split node. `lookup_update.rs` had no other users and is removed. Ported from #15611 (PeaBrane). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
A store with an explicit parent used to resolve the parent through `resolve_lookup`, which probes the named node's edge under its state lock to validate the entry, and then took the shape gate and state lock again to check the worker's coverage of the parent. The coverage check now does both: - `Node::parent_coverage` reports `Covered`, `Uncovered`, or `Missing` when the hash is not in the edge, under one lock. - `Missing` repairs the stale entry (`repair_stale`) and continues from the node the hash moved to, not from the entry. Repair leaves the entry alone when the worker no longer covers the parent there, so re-reading it would loop. Anchor nodes also carry a flag, so `is_anchor_node` no longer probes the `anchor_nodes` DashMap on every store. Debug builds assert that the flag agrees with the map. CRTC's bench counters are now also compiled into tests, so tests can assert repair counts. Ported from #15611 (PeaBrane). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
A CRTC remove used to validate each lookup entry with a state read, count the following hashes the edge still holds with another read, and then take the exclusive gate and state write lock to drop coverage. Hashes the edge no longer held fell back to one resolve and one locked removal each. `Node::remove_worker_for_leading_hashes` now finds and removes a whole run under one exclusive gate: - The run is the following hashes whose lane entries name the node, compared by lane-local id (#16032's grouping rule). - It scans under an upgradable state read and stops at the first hash the edge no longer holds. Readers are blocked only when coverage actually changes, which upgrades to the write lock. - `EdgeIndex` remembers whether it saw a repeated hash, so `position_near` can check the previous block's neighbors before the index. A run usually lists adjacent blocks. - A run whose head moved after a cross-thread split repairs once, toward the head, and retries the run on the resolved node. - The lifecycle check runs once per run instead of once per hash. Coverage still only shrinks, under the state write lock, through the same `remove_worker_at_pos`, so readers see the state before or after a removal and no new overcount path is added. What changes is that a stale run head is now detected under the node's exclusive gate rather than a state read. The two-lane randomized remove test now also checks the lanes' lookup hold invariants after every event. Ported from #15611 (PeaBrane). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Ported from #15611 (PeaBrane). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
jthomson04
force-pushed
the
jthomson04/crtc-writer-lookup-port
branch
from
October 12, 2026 18:34
0da6e03 to
0ab9fde
Compare
Contributor
Author
|
/ok to test 0ab9fde |
jthomson04
marked this pull request as ready for review
October 12, 2026 18:36
Contributor
tedzhouhk
approved these changes
Oct 12, 2026
This was referenced Oct 12, 2026
aylonsk
added a commit
to aylonsk/dynamo
that referenced
this pull request
Oct 14, 2026
ai-dynamo#16134 replaced the CRTC event thread's per-rank lookup map with `LaneLookup`. Counter updates now read `LaneLookup::block_count`, which no longer needs to be test-only, and the backfill reads `block_counts()`. Signed-off-by: Akira Lonske <akiralonske@gmail.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.
Summary
Ports the writer-path changes of #15611 (PeaBrane) onto main, without the two layers it was stacked on:
In Rudy's per-PR e2e breakdown, #15611 carried nearly all of the stack's gain. The lower layers add a lot of mechanism for gains that only show up under heavy overload. Builds on #16032 (merged), whose redirect-only lookup semantics this PR relies on.
Commits:
CrtcNodeStatereplaces thecompressed_radix::NodeStatethat CRTC shared withRadixTree.RadixTreeis unchanged.EdgeIndex. Edges of 16 blocks or fewer keep no index and scan from the tail. Longer edges keep an open-addressingu32position table.LaneLookup. Per-lane block lookups move to an open-addressingBlockLookupthat prefetches a batch's slots.Arcper distinct node, so storing or evicting a block no longer touches the node's shared refcount.lookup_update.rsis removed; fix(kv-router): stop CRTC lookup rewrites from clearing live coverage #16032's redirect rule is kept.parent_coveragereturnsCovered,UncoveredorMissing. A missing parent repairs once and continues from the resolved node.anchor_nodesprobe; debug builds assert it matches the map.position_near. It upgrades to the write lock only when coverage changes.Correctness and locking
remove_worker_at_posunder the state write lock.EdgeIndexdifference. If a split prefix repeats a hash that also appears in its suffix, the prefix keeps its own last position of that hash. The old map dropped the hash, while a scan finds it.random_hole_streams_never_overcounttest isn't ported as-is. It lets every rank evict mid-chain, so on main it fails only through the documented equal-size skip. With the skip disabled locally, 200 seeds had zero overcounts. Instead, fix(kv-router): stop CRTC lookup rewrites from clearing live coverage #16032's two-lane randomized test now also checksLaneLookup's hold invariants after every event.Validation
cargo test -p dynamo-kv-router --all-targets --features metricspasses.--features benchCRTC tests pass.cargo test -p dynamo-bench --test mooncake_tracepasses.cargo clippy --all-targets -D warningsis clean for kv-router (default andbench,metrics) and dynamo-bench.cargo fmt --checkis clean. Every commit builds and passes clippy and the CRTC tests on its own.End to end (tyche, GB200 Grace)
Setup: 2,048 mock workers at speedup 10, AgentX at concurrency 6,144, one frontend per NUMA domain (72 CPUs each), QUIC response plane, KV routing with replica sync, block size 16. Both arms were built and run in one allocation on the same five nodes:
perf record -e cycles:u -F 99 --call-graph dwarfon both frontends from +40 s to +55 s.The workload stores only and never evicts, so e2e covers the store and lookup half of this change.
CPU attributed to CRTC in the frontends, as inclusive
cycles:ushare times measured user CPU, both frontends combined:apply_event, µs/eventfind_matches), µs/eventTiming runs (no profiler) showed no regression beyond run-to-run noise, which is about ±3–6% here:
Main's CRTC cost matches an earlier independent run of this setup (2.76 µs/event).
The e2e arms put #16032 and this PR on
e61319d830. That's because current main's mocker could not start 2,048 workers in this harness; the issue is unrelated and being followed up separately. Apart from #16032, main has no CRTC changes since that commit. The port arm's non-test code is byte-identical to this PR. The main arm is #16032 as ofc8714cdb8c; #16032's laterf71cc95af1only moves the lane-local naming check in removal grouping out of the node lock, and groups the same hashes.Microbench (
mooncake_bench, perlib/bench/kv_router/INDEXER_BENCH.md)Setup:
achieved_block_ops_per_secupdate_accepted_to_finishedp50update_accepted_to_finishedp99query_servicep50query_servicep99Write capacity is up 11% and update latency is down 44% (p50) and 29% (p99). Lookup p50 is unchanged. Lookup p99 shows a small tail increase (+4% at keep-up, +7% overloaded); reader code is unchanged, so this is noted rather than explained.
Indexer memory (
mooncake_bench)Method:
mallinfo2in-use plus mmapped) just before and just after dropping the indexer.INDEXER_BENCH.md(block size 128, 128 workers, with evictions)--num-gpu-blocks 32000000, trace duplication 16)EdgeIndex): −32.9% and −20.1% on its own.LaneLookupadds the rest; the last two commits don't change memory.🤖 Generated with Claude Code
Summary by CodeRabbit