Repository navigation
perf(kv-router): allocate CRTC child storage only on a node's first child - #17259
jthomson04 merged 2 commits into
Conversation
…hild Every CRTC node held its children in an ArcSwap, and dropping an ArcSwap runs arc_swap's Debt::pay_all, which walks one debt node per thread that has ever loaded one. Most nodes the indexer frees are dead leaves that never had a child, so reclaiming each one paid that walk. NodeChildren now keeps its ArcSwap in a OnceLock that stays unset until the first child is inserted; readers see an unset cell as Empty. A node that never had a child drops without the walk and without allocating an Arc<ChildrenState>. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
github-actions
Bot
added
perf
documentation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Lazy state representation and reads lib/kv-router/src/indexer/concurrent_radix_tree_compressed/children.rs |
NodeChildren stores its ArcSwap in a OnceLock. Empty construction leaves the cell unset, and read methods use a shared empty state when it is unset. |
Mutation paths and validation lib/kv-router/src/indexer/concurrent_radix_tree_compressed/children.rs, lib/kv-router/src/indexer/concurrent_radix_tree_compressed/README.md |
Insertion initializes the cell on demand. Removal, clearing, and split transfer handle unset state. A test checks unset and initialized states. The README describes allocation and cleanup. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Merge Risk: 
68f04
-
The lazy child-state change has no identified issue that needs to be resolved before merge.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 1 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. Description check The description includes the required Overview, Details, reviewer starting point, benchmark data, reproduction steps, and implementation context. However, the required Related Issues section contains … Add the implemented issue reference in the Related Issues section, such as “Closes #1234” or “Closes DYN-1234”. Use “Relates to” if the pull request does not fully resolve the issue.-
Check name Status Explanation Title check The title clearly and concisely describes the main change: lazy allocation of CRTC child storage on first child insertion. 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.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 1 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Full details: Description check
-
Explanation
The description includes the required Overview, Details, reviewer starting point, benchmark data, reproduction steps, and implementation context. However, the required Related Issues section contains “None” and does not reference an issue.
Resolution
Add the implemented issue reference in the Related Issues section, such as “Closes
#1234” or “Closes DYN-1234”. Use “Relates to” if the pull request does not fully resolve the issue.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
The first insert created the cell holding `Empty`, then swapped in the real state, paying an `Arc` allocation and a full debt walk for a value nobody reads. Seed the cell with the first child instead. Also pass the cell into `compare_and_swap` so nothing but an insert can create it, return early from `from_map` on an empty map, and fold `values_snapshot`, `extend_values` and `entries_snapshot` into `for_each_child`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
|
/ok to test 27157a9 |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
✅ Dynamo PR CI passed — run 40159577891 (attempt 1) on
|
Overview:
Every CRTC node holds its children in an
ArcSwap, and dropping one runs arc_swap'sDebt::pay_all, which walks a debt node for every thread that ever loaded anArcSwap(about 140 here). Most freed nodes are dead leaves that never had a child, so reclaiming each cost about 5 µs in that walk. This PR allocates theArcSwaponly when a node gets its first child, and seeds it with that child. That raises CRTC write capacity by 15% under glibc and 49% under jemalloc.Write capacity ranged from 69.8–73.0 M block ops/s for base to 80.9–83.0 for this PR under glibc, and from 94.3–114.8 to 154.1–163.3 under jemalloc.
Keep-up runs on this node land in one of two states whatever the arm: fast (find_matches p50 about 2.5 µs) or slow (about 3.9 µs). Slow runs were 3/10 for base and 2/10 for this PR under glibc, and 3/10 and 5/10 under jemalloc. A median across both states mostly reflects that split, so this table compares medians within the fast state:
Slow-state runs show the same thing: find_matches is unchanged or up to 3% faster, and update p50 falls 7–9% under both allocators.
Benchmark:
mooncake_benchreplays the full Mooncake trace (128 workers, duplication factor 20, length factor 4: 472k requests, 2.0M KV events, 320M block operations) throughThreadPoolIndexerover CRTC with 8 event workers, on one 144-core Grace node. Overload replays the trace in 750 ms, so the achieved rate is the write capacity. Keep-up replays it in 12 s and measures find_matches and update latency. Each allocator ran 10 rounds, as fresh processes in rotated order. The overload Δ is the median per-round ratio with a bootstrap 95% CI. The keep-up Δ compares unpaired medians, so it has no CI.Details:
NodeChildren::stateis now aOnceLock<ArcSwap<ChildrenState>>that stays unset until the first insert, which seeds it with that child. Readers see an unset cell asEmpty.remove,clearandtransfer_for_splitreturn early on an unset cell, so a split that starts from a leaf saves one swap as well.Arc<ChildrenState>. In the overload profile,pay_allfalls from 9.8% to 3.3% of CPU under glibc and from 21.6% to 5.9% under jemalloc.Emptystate it replaces, so a reader racing the first insert sees what it saw before. Shape gates, versions and unlink rules are untouched. Keep-up node splits (23.8k) and lookup repairs (87.2k) match between arms.OnceLock, and each node grows by 8 bytes, while each leaf saves one heap allocation.68f04e3043. The second commit was not re-measured:Emptyand then swap in the real state. It now seeds the cell with the child directly, which saves oneArcallocation and one debt walk per leaf that gains a child. That is about 24k walks in this trace, so the expected gain is under 1%.values_snapshotandextend_valuesintofor_each_child.a_node_without_children_holds_no_state_celland a README note on why the cell is lazy. The existing concurrent-insert tests race the first insert into an unset cell.Reproduce
mooncake_bench(lib/bench/kv_router) on the full Mooncake trace,-n 128 --trace-duplication-factor 20 --trace-length-factor 4, CRTC with 8 event workers, fresh processes in rotated order on one Grace node:A keep-up run is in the fast state when its find_matches p50 is under 3.2 µs.
Where should the reviewer start?
concurrent_radix_tree_compressed/children.rs:NodeChildren,Loadedandcell_or_seed().Related Issues
None.
🤖 Generated with Claude Code