perf: Cap the number of threads TaggedCache sweep starts - #8243
pratikmankawde wants to merge 4 commits into
Conversation
The default partition count was the host's core count, and TaggedCache::sweep() starts one thread per partition, so a sweep created as many threads as the host has cores. Fix the default at 2, exposed as the kDefaultPartitions constant. Both arms of the constructor's initializer are now non-zero, so the non-zero assertion can no longer fire and is removed. The header no longer needs <thread> or instrumentation.h. Add a GTest suite covering the default, the zero fallback, an explicit count, and the key split across partitions.
pratikmankawde
added
the
DraftRunCI
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
sweep() started one thread per cache partition, and the partition count defaults to the host's core count, so a sweep of one cache started one thread per core. Several caches are swept per timer tick, so the churn multiplied by the number of caches. Cap the workers instead of the partitions. sweep() now starts min(partitions, kMaxSweepThreads) of them, and worker w takes partitions w, w + workerCount, w + 2 * workerCount and so on, which covers every partition exactly once. sweepHelper decided what one partition's sweep does and also wrapped it in a std::thread. Split those: sweepPartition returns void and runs in the calling thread, and sweep() alone owns thread creation. Both overloads change the same way; the sweeping logic is untouched. Fewer workers means the exclusive cache lock is held for longer, since sweep() joins inside it. That trade is unmeasured. Add gtest cases for the cap, and for eviction reaching every partition on both the key/value and key-only caches.
pratikmankawde
changed the title
A cap of 4 measured slower than the uncapped sweep on 8-core hosts, so raise the cap to 8 and take the question of the right value to review. Trim the sweep comments to the fact and its consequence.
|
@vlntb @nbougalis — requesting you both to look at this one. The open question is the value of the cap, not the mechanism. On an 8 core aws-cluster, a cap of 4 measured ~10% slower on the sweep job than no cap at all, because @vlntb, you have been here before and I would value the reasons: RIPD-2602's branch (#5442) ended back at all hardware threads, and #5486 was merged then reverted with no stated cause. If the conclusion was that this knob is the wrong one and the cache-wide mutex is the real lever, I would rather close this and put the effort behind #6048 and #5874. |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
The diff caps sweep() worker threads at min(partitions, kMaxSweepThreads) and converts sweepHelper (which returned std::thread) into sweepPartition (void, run synchronously). Each worker now strides over multiple partitions (w, w+workerCount, w+2*workerCount, ...), covering every partition exactly once with no overlap, and removal counts are still accumulated into the shared std::atomic allRemovals, so the accounting stays correct under the new striping. The sweepPartition bodies are otherwise a straight re-indent of the prior lambda bodies with no logic changes, and allStuffToSweep is still indexed per-partition so eviction/destruction-outside-lock behavior is preserved. Lambda captures (by reference for whenExpire/now/allStuffToSweep/allRemovals/lock, by value for this/w) stay valid for the whole worker lifetime since sweep() joins all workers before returning. I did not find a correctness or security issue in the changed lines; the open question the author raises (right cap value, lock-hold duration under fewer workers) is a performance/design tradeoff already called out explicitly in the description, not a code defect.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Add coverage that verifies the worker cap and exercises sweeping with fewer workers than partitions.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR caps TaggedCache::sweep() at eight workers while preserving partition coverage and adds eviction tests.
Changes:
- Implements capped, strided worker scheduling.
- Refactors sweep helpers to run within worker threads.
- Adds cap and eviction tests for value and key-only caches.
| File | Summary |
|---|---|
src/tests/libxrpl/basics/TaggedCache.cpp |
Adds cap and eviction tests; coverage does not observe actual worker creation. |
include/xrpl/basics/TaggedCache.ipp |
Implements capped scheduling and partition sweeping; reduced-worker execution is not deterministically exercised. |
include/xrpl/basics/TaggedCache.h |
Defines the worker cap and sweep helper declarations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // Without the cap, sweep() runs one worker per partition, and the | ||
| // partition count follows the host's core count. | ||
| EXPECT_EQ(Cache::kMaxSweepThreads, 8u); |
High Level Overview of Change
TaggedCache::sweep()starts onestd::threadper cache partition, and the partition count defaults tostd::thread::hardware_concurrency(). So a sweep of one cache starts one thread per core, andApplicationImp::doSweepsweeps several caches per timer tick.This caps the worker count at
TaggedCache::kMaxSweepThreads, currently 8, and leaves the partition count alone.The right value for the cap is the open question — that is what this PR is for review on. A cap of 4 was measured slower than no cap at all (below).
Context of Change
Nothing constructs a
TaggedCachewith an explicit partition count, so every cache uses the default.Capping workers rather than partitions matters: the partition count also sets the container's sharding for every user of
PartitionedUnorderedMap, and onlysweep()turns partitions into threads.There is a cost in the other direction.
sweep()joins its workers inside onestd::scoped_lockon the cache mutex, so the lock is held for the whole sweep. Fewer workers means the same entries are visited by fewer threads, so that exclusive hold gets longer. That is why a cap of 4 regressed, and it is the reason to doubt any cap below the core count.Related work on the same mutex, which a reviewer may consider the better lever: #6048 replaces the cache-wide mutex with per-partition locks (#5874 is its prerequisite), and #8240 bounds the online-delete freshen's hold on it. Earlier attempts at this specific knob did not stick — #5442 ended back at all hardware threads, and #5486 was merged then reverted.
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)TaggedCachegains a publickMaxSweepThreads.sweepHelperbecomessweepPartition, returningvoidinstead ofstd::thread; both are private.Before / After
sweep()startsmin(partitions, kMaxSweepThreads)workers instead of one per partition. Workerwtakes partitionsw,w + workerCount,w + 2 * workerCount, covering each exactly once with nothing shared between workers.sweepHelperoverloads becomevoid sweepPartition(...), running in the calling thread, sosweep()alone owns thread creation. The sweeping logic is unchanged; the diff is large only because removing the lambda re-indents both bodies.<thread>moves fromTaggedCache.htoTaggedCache.ipp, which is what builds the worker vector.allStuffToSweepis still sized per partition, so swept objects are still destroyed after the lock is released.Test Plan
Three gtest cases in
src/tests/libxrpl/basics/TaggedCache.cpp: the cap's value, full eviction on a key/value cache, and the same on a key-only cache (a separatesweepPartitionoverload). Full eviction is the guard that matters — a worker that skipped a partition would leave its keys behind.A perf run at this cap will be a no-op. The perf-iac AWS cluster uses
z1d.2xlargevalidators, 8 cores each (measured, not assumed). Sohardware_concurrency()is 8, partitions are 8, andmin(8, 8)is 8 — identical to the uncapped baseline. Testing any cap at or above 8 needs larger instances.The measured run was therefore at a cap of 4, 8 workers down to 4, against this branch's merge base with no other difference. One run, no spread:
rippled_jobq_timer{job_type="sweep"}sweep_q(dequeue wait)The dequeue wait is flat, so the extra time is inside the sweep rather than in scheduling.
Future Tasks
Measure the sweep's lock-hold duration directly rather than inferring it from client latency;
sweep()already logs it.