Repository navigation
[Store] Read the DFS shard topology without a global lock - #4770
Merged
Merged
Conversation
ShardAllocator kept its shard topology in a std::shared_ptr and loaded it with std::atomic_load_explicit() in Allocate, Free, UpdateAccess and the eviction paths. libstdc++ implements that free function by locking one of 16 global mutexes chosen by address and then copying the shared_ptr, so every caller takes the same mutex and bounces the same reference count. UpdateAccess runs for every DFS replica on GetReplicaList, BatchGet and PutEnd, so concurrent master RPC workers serialize there even when they touch different shards. Publish each topology snapshot through std::atomic<const ShardList*>, so readers do a plain acquire load. Readers keep no reference, so superseded snapshots are kept until the allocator is destroyed rather than reclaimed. Snapshots are only published by Init and by a growing ExpandShards, under expansion_mutex_, and each retains 16 bytes (one shared_ptr) per shard; lists are reserved to their final size. Shard states, publication order and reader semantics are unchanged. With 16 master RPC threads, allocator throughput no longer collapses under the shared lock.
daegyu94
requested review from
XucSh,
YiXR,
stmatengss and
ykwd
as code owners
October 19, 2026 09:00
ykwd
approved these changes
Oct 26, 2026
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.
Description
ShardAllocatorreads its shard topology withstd::atomic_load_explicit(&shards_)on everyAllocate,FreeandUpdateAccess. libstdc++ implements this with a global mutex pool, so masterRPC workers serialize there even when they touch different shards.
UpdateAccessruns on every DFS read andPutEnd.ShardListis an immutable snapshot that changes rarely (only onInitanda growing
ExpandShardsvia the serialized admin API), and old snapshots canbe kept until the allocator is destroyed. This PR therefore publishes it
through
std::atomic<const ShardList*>, so lookups are a lock-free acquireload, safe without
shared_ptrreference counting. TheShardStateobjectsit points to remain
shared_ptr, and behavior is unchanged.Trade-off: since readers hold no reference, superseded snapshots are not
reclaimed. Each costs 16 bytes per shard, and one is added only when an
operator grows the shard count through the admin API, which sets a new total
(e.g. 64 → 128). Normal expansions therefore retain a few KiB in total; only
an unusual pattern of many single-shard expansions grows it quadratically. A
new master term creates a new allocator, so nothing accumulates across leader
changes. In return, every
Allocate,FreeandUpdateAccessstops takinga process-wide mutex.
std::atomic<std::shared_ptr>was not used because itstill updates the shared reference count on every load.
Microbenchmark (64 shards, median of 3 runs; local benchmark not included in
this PR; AMD Ryzen Threadripper 9960X, GCC 13, Release):
UpdateAccessAllocate+FreeNote: this will conflict textually with #4018 on the
shards_declaration.Suggested Merge Priority
Module
mooncake-store)Type of Change
How Has This Been Tested?
Test commands:
Test results:
Both pass, also under ASan. TSan reports no races, including
ConcurrentExpansionAllocationAndEvictionRestore.Checklist
AI Assistance Disclosure
Claude Code assisted with analysis, implementation, testing and this
description. I reviewed every changed line.