perf: use std::nth_element instead of full sort when pruning bounded caches - #7494
Conversation
|
🔍 Review in progress — actively reviewing now (commit b498b67) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe LRU cache now validates its truncation threshold and uses Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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.
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
There was a problem hiding this comment.
Resolved in bccccf1 — Preserve smaller truncation thresholds no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
commits applied; @thepastaclaw ; please track this flake: feature_governance.py --legacy-wallet in UBSAN run |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This Sonnet-final pass covers the full delta cbd1200..bccccf1 (test coverage additions plus a doc/rename refactor of the bounded-cache prune paths) reconciled against two carried-forward prior findings. Prior-sonnet-1 (register the new unit test in non-backported.txt) is FIXED and confirmed present at the current head. Prior-codex-1 (unordered_lru_cache constructor precondition enforced only by assert, not clamped) is STILL VALID as a technical matter: independent verification confirms this PR's own nth_element commit (99beec4) changed a previously well-defined-but-degenerate edge case (truncateThreshold < maxSize) into a real out-of-bounds-iterator risk when assertions are compiled out, and this repo's CMakeLists.txt does not itself suppress CMake's default -DNDEBUG under Release builds, so the risk is real rather than purely theoretical. It is downgraded from blocking to a suggestion, however, because zero call sites in the tree currently violate the precondition, the same assert-only-precondition idiom is already an established, unflagged pattern in the adjacent limitedmap.h, and the maintainer made a considered, documented decision to assert rather than silently clamp. No new issues were introduced by the delta itself beyond this carried-forward item.
Source: Codex reviewers gpt-5.6-sol; Sonnet reviewers and final verifier claude-sonnet-5; harness claude-code; coordinator openclaw-agent/cliproxy/gpt-5.6-sol (orchestration only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— verifier - Sonnet reviewers:
claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
🟡 1 suggestion(s)
🤖 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`:
- [SUGGESTION] src/unordered_lru_cache.h:26-33: [prior-codex-1] Constructor precondition (truncateThreshold >= maxSize) enforced only via debug-only assert
This PR's std::nth_element swap (commit 99beec4bc4e) genuinely changed the safety characteristics of misusing this constructor. Before the swap, `std::sort` plus an erase loop starting at `maxSize` meant a configuration like `cache(10, 5)` was memory-safe even though it never evicted correctly (the vector reaches size 6, the loop starting at i=10 never executes). After the swap, the same misuse reaches `std::nth_element(vec.begin(), vec.begin() + maxSize, vec.end(), ...)` with `vec.size() < maxSize`, forming an out-of-range middle iterator — undefined behavior. The follow-up commit at this head (bccccf176a5) only adds a comment explaining the precondition; it does not clamp or otherwise enforce it outside of `assert()`. Verified: no call site in `src/` currently constructs `unordered_lru_cache`/`Uint256LruHashMap` with a threshold below `maxSize` (checked all instantiations in creditpool.cpp, mnhftx.h, instantsend/db.h, llmq/*.h, saltedhasher.h). Verified: this repository's own `CMakeLists.txt` sets no explicit `CMAKE_BUILD_TYPE`/`NDEBUG` handling, so a standard `-DCMAKE_BUILD_TYPE=Release` configuration relies on CMake's built-in default flags, which do define `NDEBUG` and disable `assert()` — the risk is not purely theoretical. That said, this exactly mirrors the pre-existing, previously-unflagged `assert(nPruneAfterSize >= nMaxSize)` contract in `limitedmap.h`, and the maintainer explicitly weighed and rejected clamping in favor of a hard-fail-on-misconfiguration precondition (PR discussion, comment 3679365777). Given no live caller trips this and it matches established codebase convention, this doesn't block the PR, but it's worth clamping or adding a regression test (`cache(10, 5)` insert-past-threshold) so a future caller who mismatches the two constructor arguments fails safely in both debug and release builds rather than only in debug builds.
| { | ||
| // either specify maxSize through template arguments or the constructor and fail otherwise | ||
| assert(_maxSize != 0); | ||
| // truncate_if_needed() only runs past truncateThreshold, so this is what keeps maxSize inside the vector | ||
| assert(truncateThreshold >= maxSize); |
There was a problem hiding this comment.
🟡 Suggestion: [prior-codex-1] Constructor precondition (truncateThreshold >= maxSize) enforced only via debug-only assert
This PR's std::nth_element swap (commit 99beec4) genuinely changed the safety characteristics of misusing this constructor. Before the swap, std::sort plus an erase loop starting at maxSize meant a configuration like cache(10, 5) was memory-safe even though it never evicted correctly (the vector reaches size 6, the loop starting at i=10 never executes). After the swap, the same misuse reaches std::nth_element(vec.begin(), vec.begin() + maxSize, vec.end(), ...) with vec.size() < maxSize, forming an out-of-range middle iterator — undefined behavior. The follow-up commit at this head (bccccf1) only adds a comment explaining the precondition; it does not clamp or otherwise enforce it outside of assert(). Verified: no call site in src/ currently constructs unordered_lru_cache/Uint256LruHashMap with a threshold below maxSize (checked all instantiations in creditpool.cpp, mnhftx.h, instantsend/db.h, llmq/*.h, saltedhasher.h). Verified: this repository's own CMakeLists.txt sets no explicit CMAKE_BUILD_TYPE/NDEBUG handling, so a standard -DCMAKE_BUILD_TYPE=Release configuration relies on CMake's built-in default flags, which do define NDEBUG and disable assert() — the risk is not purely theoretical. That said, this exactly mirrors the pre-existing, previously-unflagged assert(nPruneAfterSize >= nMaxSize) contract in limitedmap.h, and the maintainer explicitly weighed and rejected clamping in favor of a hard-fail-on-misconfiguration precondition (PR discussion, comment 3679365777). Given no live caller trips this and it matches established codebase convention, this doesn't block the PR, but it's worth clamping or adding a regression test (cache(10, 5) insert-past-threshold) so a future caller who mismatches the two constructor arguments fails safely in both debug and release builds rather than only in debug builds.
| { | |
| // either specify maxSize through template arguments or the constructor and fail otherwise | |
| assert(_maxSize != 0); | |
| // truncate_if_needed() only runs past truncateThreshold, so this is what keeps maxSize inside the vector | |
| 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: ['claude', 'codex']
There was a problem hiding this comment.
asserts are not disabled in release. This assert WILL trigger in release if there is an issue. Maybe you can argue this could still open us to a DoS etc if we write buggy code for this. But the assert will trigger and we won't get to undefined behavior.
There was a problem hiding this comment.
Resolved in this update — [prior-codex-1] Constructor precondition (truncateThreshold >= maxSize) enforced only via debug-only assert no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
bccccf1 to
e843d3f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/unordered_lru_cache_tests.cpp (1)
52-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the equality boundary for
truncateThreshold.Line 54 only tests a threshold greater than
maxSize. Add a focused test withIntCache cache(5, 5). Insert six entries and verify that five recent entries remain. This verifies the allowedtruncateThreshold == maxSizeboundary.Proposed test
+BOOST_AUTO_TEST_CASE(truncation_accepts_threshold_equal_to_max_size) +{ + IntCache cache(5, 5); + + for (int i = 0; i < 6; ++i) { + cache.insert(i, i); + } + + for (int i = 0; i < 6; ++i) { + BOOST_CHECK_EQUAL(cache.exists(i), i >= 1); + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/unordered_lru_cache_tests.cpp` around lines 52 - 68, Add a focused test case alongside truncation_honors_explicit_threshold using IntCache(5, 5). Insert six entries, then verify the cache retains exactly the five most recent entries and evicts the oldest, covering the allowed truncateThreshold == max_size() boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/unordered_lru_cache_tests.cpp`:
- Around line 52-68: Add a focused test case alongside
truncation_honors_explicit_threshold using IntCache(5, 5). Insert six entries,
then verify the cache retains exactly the five most recent entries and evicts
the oldest, covering the allowed truncateThreshold == max_size() boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c81ccbdf-9614-4cc7-b7d1-d43582607e09
📒 Files selected for processing (6)
src/Makefile.test.includesrc/chainlock/handler.hsrc/limitedmap.hsrc/test/unordered_lru_cache_tests.cppsrc/unordered_lru_cache.htest/util/data/non-backported.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Makefile.test.include
- test/util/data/non-backported.txt
- src/limitedmap.h
- src/unordered_lru_cache.h
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Independent verification confirms the src/unordered_lru_cache.h and src/limitedmap.h nth_element pruning logic is correct, and the new/broadened unit tests in src/test/unordered_lru_cache_tests.cpp and src/test/limitedmap_tests.cpp adequately exercise truncation, recency, and threshold behavior. Carried-forward prior-codex-1 (constructor precondition truncateThreshold >= maxSize enforced only via a 'debug-only' assert) is OUTDATED: direct inspection of src/util/check.h shows #if defined(NDEBUG) #error "Cannot compile without assertions!" #endif, util/check.cpp is unconditionally listed in both libbitcoin_util and libbitcoinkernel in src/Makefile.am (lines 1028, 1358), and configure.ac never defines NDEBUG for the standard (non-debug) build flags -- so C assertions, including this one, are active in every supported release build of this codebase, matching the maintainer's reply (comment 3696299393) exactly. New for this delta (bccccf1..e843d3f, and confirmed within the current PR range 627f82f..e843d3f): the final commit e843d3f correctly updates src/chainlock/handler.h's sizing-rationale comment from 'sorts' to 'partitions,' but the analogous constructor-doc comment in src/limitedmap.h:34-38 (and the matching test comment in src/test/limitedmap_tests.cpp:105) still describes unordered_limitedmap::prune() as performing a full sort, even though prune() was changed in an earlier commit in this same stack to use std::nth_element (confirmed at src/limitedmap.h:109). This is a real, currently-unaddressed documentation nit kept as a nitpick finding. No backport-prerequisite claims were raised by any agent for this PR (it is an internal Dash algorithmic/perf change, not a Bitcoin Core backport), so prerequisite_adjudications is empty. Source: Sol/Codex reviewers gpt-5.6-sol; Sonnet reviewers and final verifier claude-sonnet-5; harness claude-code; coordinator openclaw-agent/cliproxy/gpt-5.6-sol (orchestration only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— backport-reviewer (completed)
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
|
pls see b586d1c |
…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. Since the vector is no longer sorted, sortedIterators is renamed to iterators. The prune() assert is tightened to cover the bound nth_element actually relies on: tooMuch > 0 alone does not catch an unsigned underflow of map.size() - nMaxSize. The unordered_lru_cache constructor gains the equivalent assert, whose connection to the iterator arithmetic in truncate_if_needed() is otherwise only visible by reading both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Also cover erase(), clear() and emplace(), and the case where overwriting an existing key must not grow the map. The class exposes no size(), so the last one asserts that indirectly: it sits exactly at the truncate threshold and checks that nothing was evicted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
prune() now partitions with std::nth_element instead of fully sorting, so update the remaining sorting references: the seenChainLocks sizing rationale, the unordered_limitedmap constructor docs and limitedmap_prune_after_size_test. Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
e843d3f to
b498b67
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/unordered_lru_cache_tests.cpp (1)
52-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the invalid threshold contract.
IntCache cache(5, 6)covers the valid threshold path only. Add a test forIntCache cache(5, 4)that exercises thetruncateThreshold >= maxSizeconstructor assertion, so removing or reversing that guard fails the suite. Use the project assertion-test helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/unordered_lru_cache_tests.cpp` around lines 52 - 68, Add a test case near truncation_honors_explicit_threshold that constructs IntCache with max size 5 and threshold 4 using the project’s assertion-test helper, and verify construction triggers the expected assertion for truncateThreshold >= maxSize. Keep the existing valid-threshold test unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/unordered_lru_cache_tests.cpp`:
- Around line 52-68: Add a test case near truncation_honors_explicit_threshold
that constructs IntCache with max size 5 and threshold 4 using the project’s
assertion-test helper, and verify construction triggers the expected assertion
for truncateThreshold >= maxSize. Keep the existing valid-threshold test
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93104011-e5c3-4a0f-bd17-d303343f7533
📒 Files selected for processing (7)
src/Makefile.test.includesrc/chainlock/handler.hsrc/limitedmap.hsrc/test/limitedmap_tests.cppsrc/test/unordered_lru_cache_tests.cppsrc/unordered_lru_cache.htest/util/data/non-backported.txt
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Makefile.test.include
- test/util/data/non-backported.txt
- src/limitedmap.h
- src/unordered_lru_cache.h
- src/chainlock/handler.h
|
that change should be included now; sadly the agent also did a rebase, when I wanted it to just squash down changes... range diff looks right and below is empty too. should be right. |
3 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: