Repository navigation
perf(umbp): fix the local-path lock contention that fails the primitive concurrency gate - #637
Merged
TianDi101 merged 3 commits intoSep 3, 2026
Conversation
PageBackend serialized every entry point behind one exclusive `mutex_`,
including the read-only resolve paths. The local fast path this replaced
(DRAMTier) held a `shared_mutex` for its slot map and a separate `lru_mu_`
for the LRU, so the read paths never blocked one another; that property was
lost when the tier was dropped.
The cost shows up in the primitive concurrency gate, which scores the 1->8
rank ratio of `batch_exists` against a 2.00x floor. Resolve is the whole of
that benchmark, so the exclusive lock caps it: throughput peaks at 4 ranks
and then falls, and the gate reads 1.70x.
Restore the split:
- `mutex_` becomes a `std::shared_mutex`. `Resolve`, `BatchResolve` and
`Contains` take a shared lock; the mutating paths take `unique_lock`.
- `read_lease_until` becomes an atomic `read_lease_ns` renewed by a
CAS-max loop, so renewing a lease no longer needs write access. Leases
only ever extend, so a slower thread cannot shorten one. `OwnedSlot`
gains an explicit move ctor/assignment because atomics aren't movable.
- The LRU list and map move behind their own `lru_mutex_`, and
`BatchResolve` collects the keys it touched and splices them under a
single acquisition instead of one per key. Lock order is
`mutex_` -> `lru_mutex_`, never the reverse.
Measured on n09-21, interleaved stock/fixed x 2 rounds, `umbp_prim_ci.py`
with the CI's own arms and rank counts:
nomaster exists scaling@0 1 rank 2 4 8 1->8
stock 1779737 2855396 4324345 3031824 1.70x FAIL
fixed 1720906 2889692 3967708 3970172 2.31x PASS
The gain is confined to where the contention was: +31% at 8 ranks, and the
4->8 knee goes from 0.70x to 1.00x, while 1/2/4 ranks are unchanged. p50 at
8 ranks drops 0.674 -> 0.497 ms. Both rounds pass on their own (2.39x,
2.23x). 256 rows, no correctness failures or mismatches.
Two limits worth stating. At 4 ranks the fixed build is ~0.92x of stock:
`shared_mutex` has a higher uncontended acquire cost, so it gives a little
back where there is no contention to recover. And this does not close the
gap to 094c3c6, which scores 3.86x -- that gap factors into 1.47x
per-operation cost, already visible at 1 rank where nothing contends, times
1.67x lost scaling. This change addresses the second term only; the first is
a separate hunt, concentrated in `put` and `exists`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ExecuteBatchPutPlan ran ExecuteLocalPut once per key from inside a
ParallelFor, and each of those calls takes PeerPool::BatchAllocate and
PeerPool::BatchCommit with a single-element vector. Both take
operation_mutex_ and, before they reach the backend, consult placement and
pending state and build their per-round containers -- a std::map in
allocate, a std::map and a std::set in commit, plus FindOwnerLocked, which
returns a vector by value. Driving that per key made the pool's fixed
per-call cost, taken under an exclusive lock by four copy workers at once,
the dominant term of a local put.
The batch APIs were already there; the caller just wasn't using them. Do
what the ranged path (ExecuteLocalPutRangesBatch) already does: one
BatchAllocate for the whole batch, ParallelFor over the copies alone, then
one BatchCommit and at most one BatchAbort. Only the copy is inherently
per-key, and it stays parallel -- distinct slots, distinct result indices.
The per-key CountMetric pair folds into one update per batch as well;
AddCounter locks and materializes its help text on every call.
ExecuteLocalPut keeps its two genuine single-key callers (the re-cache
worker and MaybeReCacheAfterRemote) and is unchanged.
Measured on n06-21, interleaved against the same commit without this change,
four rounds at each rank count, umbp_prim_ci.py nomaster:
put keys/s 1 rank 2 4 8
before 66,062 71,494 75,285 65,509
after 167,078 168,316 173,108 157,763
2.53x 2.35x 2.30x 2.41x
That is 0.31x -> 0.82x of the pre-regression commit 094c3c6 at 1 rank and
0.37x -> 0.88x at 8. Correctness is unchanged: no failures or mismatches in
any round, and the umbp ctest suite is 30/30 (umbp_ssd_ranged_io failed once
on a gRPC port collision -- "Address already in use" on a fixed port -- and
passes on its own).
This does not move the concurrency gate, which scores exists: 1.46x -> 1.82x,
still under the 2.00x floor. The gate's metric is a different path.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BatchExists asked the pool to resolve every key and then used nothing but
whether each one was found -- `out[i] = holders[i] != nullptr`. A resolve is
much more than that answer: PageBackend::BatchResolve heap-copies the page
vector of every key it finds, renews a read lease on it and touches the LRU.
For a 256-key batch that is 256 vector allocations built and immediately
dropped, and with resolve now running under a shared lock, several reader
threads do it at once and contend on the allocator instead of the mutex.
Add a containment path and use it for the local half of Exists:
- MediumBackend::BatchContains, virtual with a default that loops the
existing Contains(), so no other backend has to change.
- PageBackend overrides it: one shared lock, one hash lookup per key, and
nothing allocated per key.
- PeerPool::BatchContains resolves the read order under the operation lock
and then calls the backends outside it, carrying only the still-unanswered
indices from one backend to the next -- the same shape the resolve paths
use, and for the same reason.
Two deliberate behavior changes, both of which follow from what the call
means: an existence probe no longer renews a read lease, and no longer counts
as a read for recency. Asking whether a key is present is not a read of it,
and a probe should not keep a key alive or promote it over keys that were
actually used. Nothing depended on the old behavior: a get does its own
resolve, and the lease it needs is taken there.
Measured on n06-21, interleaved against the same tree without this change,
four rounds, umbp_prim_ci.py nomaster:
exists keys/s 1 rank 2 4 8 1->8
before 1340243 2803992 3630879 3696662 2.76x
after 2362440 4212537 5405854 7788361 3.30x
1.76x 1.50x 1.49x 2.11x
The 100-pass scaling passes agree at every absent fraction: 3.77M -> 8.14M
keys/s at absent=0.0 and 2.77M -> 5.48M at 0.5. Against 094c3c6, the commit
this series is chasing, exists is now 0.91x at 1 rank and 0.88x at 8, from
0.70x and 0.84x.
The other primitives are unchanged, as expected -- BatchContains is reachable
only from BatchExists. umbp ctest is 30/30 and the harness reports no bad
rows in any round, including the absent-key passes, which are what would
catch a containment answer that wrongly said "missing".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
isytwu
added a commit
that referenced
this pull request
Sep 8, 2026
…NDED] Kept on a side branch as the record of a rejected optimisation, not proposed for merge. A layer-wise reader asks about one key set once per layer group, so seven of every eight resolves rebuild an identical answer. This caches it, keyed on the identity of the caller's key vector (cheaper than hashing 64 KiB of keys, and confirmed by a full comparison so a wrong guess can only cost a re-resolve), validated by an eviction generation, with its own shared_mutex so the hit path stays concurrent and does not undo #637. It works -- the cache hits, and the resolve phase drops from 39.1% to 35.5% of a ranged call. But that is only worth: threads before after 1 1.880 1.839 -2.2% (ranges overlap) 8 1.884 1.828 -3.0% The reason is that resolve is not dominated by the hash and map probe a cache removes. It is dominated by the per-key page-vector copy, and BatchResolve returns by value, so a hit still pays every one of those heap allocations. The cache saves the lookups and nothing else. Getting the rest means MediumBackend, PeerPool and PoolClient passing a borrowed or shared result instead of a copied one -- a real change to the interface #633/#637 have just reworked. That is the follow-up worth doing; this is not worth 3% and a new invariant in concurrent code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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.
What this fixes
The primitive concurrency gate added in #634 fails on this branch. It scores the
1→8 rank ratio of
batch_existsagainst a 2.00x floor and reads 1.96x.The cause is
69607252("Refactor/umbp drop standalone client", #620) — but notin the way its diff suggests. The per-key
default_pool_->BatchAllocate({request})call already existed at
d427be3a, which measures clean. #620 is what activatedit:
WithEmbeddedDefaultsmade the standalone server resolve to the embedded pool,where before it fell back to
StandaloneClient/DRAMTier. That path held ashared_mutexfor its slot map and a separate lock for its LRU; the replacementserializes everything behind one exclusive lock and drives it one key at a time.
It is contention, not per-operation cost. The local put path is internally
parallel (
ParallelFor,UMBP_DRAM_WRITE_THREADS, default 4), so a 1-rankmeasurement is not contention-free. Varying only the thread count at 1 rank:
094c3c64(DRAMTier)The new path is 29% faster single-threaded. The whole regression appears only
under concurrency.
The three changes
PageBackendresolves under a shared lock.mutex_becomes astd::shared_mutex;Resolve/BatchResolve/Containstake shared locks.read_lease_untilbecomes an atomic renewed by a CAS-max loop so renewal needsno write access, and the LRU moves behind its own lock with
BatchResolvesplicing the whole batch under one acquisition instead of one per key.
A local put batch takes one pool call, not one per key.
ExecuteBatchPutPlancalled
ExecuteLocalPutper key from inside aParallelFor, and each call tookPeerPool::BatchAllocateandBatchCommitwith a single-element vector — bothunder
operation_mutex_, each building per-round containers and callingFindOwnerLocked. Now: oneBatchAllocate,ParallelForover the copies alone,one
BatchCommit. This is whatExecuteLocalPutRangesBatchalready did, which iswhy
put_rangesnever regressed whileputdid.Existsanswers by containment instead of resolving.BatchExistsusednothing but
holders[i] != nullptr, while a resolve heap-copies the page vectorof every key it finds, renews a read lease and touches the LRU. Adds
MediumBackend::BatchContains(virtual, with a default that loops the existingContains(), so no other backend changes), aPageBackendoverride doing oneshared lock and one hash lookup per key with nothing allocated, and
PeerPool::BatchContains.Results
n06-21, interleaved A/B, warm rounds only,
umbp_prim_ci.py --arms nomaster:The gate (
existsscaling@0, 1→8, floor 2.00x):094c3c64(pre-regression)1366c0d5)Per primitive, as a fraction of
094c3c64:put@1 / @8exists@1 / @8get@1 / @8get_ranges@8putalone goes 66,062 → 167,078 keys/s at 1 rank (2.53x), and 2.3–2.5x at everyrank count.
Trade-off, stated plainly
get_rangesgives up the improvement the base had over094c3c64: it ends atparity (~654k vs 650k keys/s @8) rather than 1.43x. Mechanism: every local call
site passes
include_descs=false, so the only per-key allocation left inBatchResolveis theentry.pagesvector copy. The exclusive lock admitted onethread at a time; the shared lock admits eight, trading mutex serialization for
allocator contention. That is also why
exists— which discards those pages —gains while
get_ranges, which needs them, does not. Removing that copy forreaders (a lease-scoped view instead of a copy) is the natural follow-up and is
deliberately not in this PR: it carries real lifetime risk and deserves its own
review.
A fourth change — making
PeerPool::operation_mutex_ashared_mutextoo — wasimplemented, measured, and dropped: it bought ~0.1x of gate margin (2.76x vs
2.65x) while costing 22% on
get_ranges@8and 13% onput_ranges@8.Testing
ctest -R umbp: 30/30. (umbp_ssd_ranged_iofails intermittently on a fixedgRPC port collision under load — "Address already in use" — and passes on retry;
unrelated to these changes.)
passes, which are what would catch a containment answer wrongly saying "missing".
Note on semantics
An existence probe no longer renews a read lease or counts toward recency. That is
intentional: asking whether a key is present is not a read of it, and a probe should
not keep a key alive or promote it over keys that were actually used. Nothing
depended on the old behavior — a
getdoes its own resolve and takes the lease itneeds there.
🤖 Generated with Claude Code