perf: use std::nth_element instead of full sort when pruning bounded caches - #7494
perf: use std::nth_element instead of full sort when pruning bounded caches#7494PastaPastaPasta wants to merge 2 commits into
Conversation
…caches unordered_limitedmap::prune() and unordered_lru_cache::truncate_if_needed() both fully sorted a vector of every entry in the container just to find the ones below the eviction boundary. Only a partition around that boundary is required, which reduces the work from O(n log n) to O(n). The set of evicted entries is unchanged. Ties between equal values were already broken arbitrarily because std::sort is not stable.
|
⛔ Blockers found — Sonnet deferred (commit 94b5c42) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe LRU cache constructor now asserts that the truncation threshold is at least the maximum size, and cache truncation uses Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The bounded-cache eviction changes are behavior-preserving and the added unit tests cover threshold and recency semantics. One repository-maintenance omission leaves the new Dash-specific test source outside the file set used by Dash cppcheck and differential formatting checks; there are no blocking correctness issues.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:68-69: Register the new Dash-specific test for lint checks
`src/test/unordered_lru_cache_tests.cpp` is newly authored Dash-specific code, but this list does not include it. Both `test/lint/lint-cppcheck-dash.py` and the differential clang-format workflow derive their Dash-specific file sets from this list, so the new test is silently excluded from those checks. Add its exact path alongside the other unit-test entries.
The cache had no coverage at all. Pin down the truncation behaviour that the eviction path relies on: nothing is dropped until the size exceeds the truncate threshold, truncation leaves exactly max_size entries, and both get() and exists() refresh an entry's recency.
cbd1200 to
94b5c42
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94b5c42c22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // sort by last access time (descending order) | ||
| std::sort(vec.begin(), vec.end(), [](const Iterator& it1, const Iterator& it2) { | ||
| // partition by last access time (descending order), the entries to keep end up in the first maxSize slots | ||
| std::nth_element(vec.begin(), vec.begin() + maxSize, vec.end(), [](const Iterator& it1, const Iterator& it2) { |
There was a problem hiding this comment.
For constructor calls such as cache(10, 5), which the previous implementation handled, the sixth insertion builds a six-element vector and then forms vec.begin() + 10, an out-of-range iterator that causes undefined behavior in release builds; debug builds instead abort at the new assertion. The previous sort-and-erase path retained entries until the size exceeded maxSize, so skip partitioning while cacheMap.size() <= maxSize or otherwise preserve this previously accepted configuration.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta resolves prior-1 by registering the new unit test in test/util/data/non-backported.txt, and it introduces no new issue. Cumulative revalidation confirms one newly identified correctness regression: explicit truncation thresholds smaller than the maximum size, which the previous implementation handled safely, can now produce an out-of-range std::nth_element iterator in release builds.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/unordered_lru_cache.h`:
- [BLOCKING] src/unordered_lru_cache.h:26-33: Preserve smaller truncation thresholds
The constructor previously accepted configurations such as `cache(10, 5)`. Although the old implementation began sorting after the sixth insertion, it did not erase anything until the size exceeded `maxSize`, so the configuration remained safe and effectively behaved as if the threshold were 10. The new assertion rejects it only in assertion-enabled builds; with `NDEBUG`, the sixth insertion reaches `std::nth_element(vec.begin(), vec.begin() + 10, vec.end(), ...)` with a six-element vector, forming an out-of-range iterator and invoking undefined behavior. Clamp the effective threshold to `maxSize` instead of relying on a debug-only assertion, and add a regression test for a threshold below the maximum.
| { | ||
| // either specify maxSize through template arguments or the constructor and fail otherwise | ||
| assert(_maxSize != 0); | ||
| assert(truncateThreshold >= maxSize); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Preserve smaller truncation thresholds
The constructor previously accepted configurations such as cache(10, 5). Although the old implementation began sorting after the sixth insertion, it did not erase anything until the size exceeded maxSize, so the configuration remained safe and effectively behaved as if the threshold were 10. The new assertion rejects it only in assertion-enabled builds; with NDEBUG, the sixth insertion reaches std::nth_element(vec.begin(), vec.begin() + 10, vec.end(), ...) with a six-element vector, forming an out-of-range iterator and invoking undefined behavior. Clamp the effective threshold to maxSize instead of relying on a debug-only assertion, and add a regression test for a threshold below the maximum.
| { | |
| // either specify maxSize through template arguments or the constructor and fail otherwise | |
| assert(_maxSize != 0); | |
| assert(truncateThreshold >= maxSize); | |
| } | |
| explicit unordered_lru_cache(size_t _maxSize = MaxSize, size_t _truncateThreshold = TruncateThreshold) : | |
| maxSize(_maxSize), | |
| truncateThreshold(std::max(_truncateThreshold == 0 ? _maxSize * 2 : _truncateThreshold, _maxSize)) | |
| { | |
| // either specify maxSize through template arguments or the constructor and fail otherwise | |
| assert(_maxSize != 0); | |
| } |
source: ['codex']
There was a problem hiding this comment.
this is required because;
std::nth_element requires its "nth" iterator to satisfy first <= nth <= last. If truncateThreshold < maxSize, it's possible for truncate_if_needed() to be entered (cacheMap.size() > truncateThreshold) while cacheMap.size() < maxSize — meaning vec.begin() + maxSize lands past vec.end(). That's undefined behavior (out-of-bounds iterator arithmetic fed to nth_element), not just a logic bug.
Under the old std::sort-based code this misconfiguration was harmless: sorting doesn't care about maxSize at all, and the eviction loop for (i = maxSize; i < vec.size(); i++) would simply not execute if maxSize > vec.size() — a silent no-op, not memory-unsafe.
weirdly setting the truncate threshold to a max of various things makes no sense. Better to assert imo
2 participants
Issue being fixed or feature implemented
Two bounded caches do strictly more work than they need to when they evict:
unordered_limitedmap::prune()(src/limitedmap.h) collects an iterator to every entry in the map and fully sorts that vector ascending by mapped value, then throws away everything except thetooMuch = size - nMaxSizesmallest entries.unordered_lru_cache::truncate_if_needed()(src/unordered_lru_cache.h) does the same thing with a full descending sort by access counter, then erases everything past indexmaxSize.In both cases the total order is discarded immediately. All that is actually needed is a partition around the eviction boundary, so the sort is
O(n log n)work whereO(n)suffices. This matters because these caches are not small:unordered_lru_cacheinstances in the tree are sized at 10000 (islockCache,txidCache,outpointCache) and 30000 (hasSigForIdCache,hasSigForSessionCache,hasSigForHashCache), and truncation runs on the insert path.This came out of review of #7482, which also touches
src/limitedmap.h. This PR is deliberately scoped so the two can merge in either order without conflicting: it only rewrites the body ofprune(), leaves the constructor and themax_size()/ prune-threshold accessors alone, and does not touchsrc/test/limitedmap_tests.cppat all. The existing limitedmap tests already pin the prune membership semantics that must be preserved.What was done?
Replaced
std::sortwithstd::nth_elementin both eviction paths, using the same comparator in each case:unordered_limitedmap::prune():tooMuchis now computed before the partition, andstd::nth_element(begin, begin + tooMuch, end, cmp)places thetooMuchsmallest entries in the leading range. The subsequentresize(tooMuch)and erase loop are unchanged.unordered_lru_cache::truncate_if_needed():std::nth_element(vec.begin(), vec.begin() + maxSize, vec.end(), cmp)leaves themaxSizemost recently accessed entries in the leading range, and the existing loop erases everything frommaxSizeonward.This is behaviour preserving for the set of evicted entries. The relative order within the evicted group and within the retained group was never observable, and ties between equal values were already broken arbitrarily because
std::sortis not a stable sort.Two supporting changes:
assert(truncateThreshold >= maxSize)to theunordered_lru_cacheconstructor.vec.begin() + maxSizeis only a valid iterator because truncation runs only whensize > truncateThreshold, so that invariant now has to hold rather than merely happening to hold. Every instantiation insrc/passes onlymaxSizeand therefore gets the defaulttruncateThreshold = 2 * maxSize, so no caller is affected.unordered_limitedmapalready has the equivalentassert(nPruneAfterSize >= nMaxSize).src/test/unordered_lru_cache_tests.cpp. The class previously had no unit test coverage at all.How Has This Been Tested?
Built on macOS arm64 (aarch64-apple-darwin, clang,
--enable-debug --enable-crash-hooks --enable-stacktraces --without-gui --enable-tests), against thedependsprefix.make -C src test/test_dashat both commits of this branch, to confirm each commit builds on its own../src/test/test_dash --run_test=limitedmap_tests,unordered_lru_cache_tests— 6 test cases, no errors../src/test/test_dash(full unit test suite) — 770 test cases, no errors. This is the check that the new constructor assert does not fire for any existing cache instantiation.test/lint/lint-whitespace.py,test/lint/lint-git-commit-check.py(withCOMMIT_RANGEset to this branch),test/lint/lint-tests.py,test/lint/lint-includes.py,test/lint/lint-include-guards.py— all clean.The new tests cover: no truncation while the size is at or below the truncate threshold (which also pins the default threshold at
2 * max_size); truncation down to exactlymax_size, keeping the most recently accessed entries and leaving their values intact; an explicitly supplied truncate threshold; and that bothget()andexists()refresh an entry's recency so an otherwise-oldest entry survives truncation.Not run: functional tests, since no behaviour reachable from RPC or P2P changes.
Breaking Changes
None. Both changes preserve the set of entries evicted, and no consensus, network, or RPC behaviour is affected.
Checklist: