-
Notifications
You must be signed in to change notification settings - Fork 1.2k
perf: use std::nth_element instead of full sort when pruning bounded caches #7494
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,180 @@ | ||
| // Copyright (c) 2026 The Dash Core developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #include <unordered_lru_cache.h> | ||
|
|
||
| #include <test/util/setup_common.h> | ||
|
|
||
| #include <boost/test/unit_test.hpp> | ||
|
|
||
| using IntCache = unordered_lru_cache<int, int, std::hash<int>>; | ||
|
|
||
| BOOST_FIXTURE_TEST_SUITE(unordered_lru_cache_tests, BasicTestingSetup) | ||
|
|
||
| BOOST_AUTO_TEST_CASE(no_truncation_below_threshold) | ||
| { | ||
| // the default truncate threshold is twice the max size | ||
| IntCache cache(10); | ||
| BOOST_CHECK_EQUAL(cache.max_size(), 10U); | ||
|
|
||
| for (int i = 0; i < 20; i++) { | ||
| cache.insert(i, i); | ||
| } | ||
|
|
||
| // reaching the threshold is not enough to trigger truncation | ||
| for (int i = 0; i < 20; i++) { | ||
| BOOST_CHECK(cache.exists(i)); | ||
| } | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(truncation_keeps_most_recent) | ||
| { | ||
| IntCache cache(10); | ||
|
|
||
| // exceeding the threshold truncates down to the max size | ||
| for (int i = 0; i < 21; i++) { | ||
| cache.insert(i, i); | ||
| } | ||
|
|
||
| for (int i = 0; i < 21; i++) { | ||
| BOOST_CHECK_EQUAL(cache.exists(i), i >= 11); | ||
| } | ||
|
|
||
| // the retained values are intact | ||
| for (int i = 11; i < 21; i++) { | ||
| int value{0}; | ||
| BOOST_CHECK(cache.get(i, value)); | ||
| BOOST_CHECK_EQUAL(value, i); | ||
| } | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(truncation_honors_explicit_threshold) | ||
| { | ||
| IntCache cache(5, 6); | ||
| BOOST_CHECK_EQUAL(cache.max_size(), 5U); | ||
|
|
||
| for (int i = 0; i < 6; i++) { | ||
| cache.insert(i, i); | ||
| } | ||
| for (int i = 0; i < 6; i++) { | ||
| BOOST_CHECK(cache.exists(i)); | ||
| } | ||
|
|
||
| cache.insert(6, 6); | ||
| for (int i = 0; i < 7; i++) { | ||
| BOOST_CHECK_EQUAL(cache.exists(i), i >= 2); | ||
| } | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(get_refreshes_recency) | ||
| { | ||
| IntCache cache(4); | ||
|
|
||
| // fill up to the threshold without triggering truncation | ||
| for (int i = 0; i < 8; i++) { | ||
| cache.insert(i, i); | ||
| } | ||
|
|
||
| // make the oldest entry the most recently used one | ||
| int value{0}; | ||
| BOOST_CHECK(cache.get(0, value)); | ||
| BOOST_CHECK_EQUAL(value, 0); | ||
|
|
||
| // this insert exceeds the threshold and truncates | ||
| cache.insert(8, 8); | ||
|
|
||
| // the refreshed entry survives, the entries it outranks do not | ||
| for (int i = 0; i < 9; i++) { | ||
| const bool expected = i == 0 || i >= 6; | ||
| BOOST_CHECK_EQUAL(cache.exists(i), expected); | ||
| } | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(exists_refreshes_recency) | ||
| { | ||
| IntCache cache(4); | ||
|
|
||
| for (int i = 0; i < 8; i++) { | ||
| cache.insert(i, i); | ||
| } | ||
|
|
||
| BOOST_CHECK(cache.exists(1)); | ||
|
|
||
| cache.insert(8, 8); | ||
|
|
||
| for (int i = 0; i < 9; i++) { | ||
| const bool expected = i == 1 || i >= 6; | ||
| BOOST_CHECK_EQUAL(cache.exists(i), expected); | ||
| } | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(erase_and_clear) | ||
| { | ||
| IntCache cache(10); | ||
|
|
||
| for (int i = 0; i < 5; i++) { | ||
| cache.insert(i, i); | ||
| } | ||
|
|
||
| cache.erase(2); | ||
| BOOST_CHECK(!cache.exists(2)); | ||
| BOOST_CHECK(cache.exists(1)); | ||
|
|
||
| // erasing an absent key is a no-op | ||
| cache.erase(2); | ||
| cache.erase(100); | ||
| BOOST_CHECK(cache.exists(1)); | ||
|
|
||
| cache.clear(); | ||
| for (int i = 0; i < 5; i++) { | ||
| BOOST_CHECK(!cache.exists(i)); | ||
| } | ||
|
|
||
| // the cache is still usable afterwards | ||
| cache.insert(7, 7); | ||
| int value{0}; | ||
| BOOST_CHECK(cache.get(7, value)); | ||
| BOOST_CHECK_EQUAL(value, 7); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(emplace_inserts_and_overwrites) | ||
| { | ||
| IntCache cache(10); | ||
| int value{0}; | ||
|
|
||
| cache.emplace(1, 10); | ||
| BOOST_CHECK(cache.get(1, value)); | ||
| BOOST_CHECK_EQUAL(value, 10); | ||
|
|
||
| // emplacing a key that is already present replaces its value | ||
| cache.emplace(1, 20); | ||
| BOOST_CHECK(cache.get(1, value)); | ||
| BOOST_CHECK_EQUAL(value, 20); | ||
|
|
||
| cache.insert(1, 30); | ||
| BOOST_CHECK(cache.get(1, value)); | ||
| BOOST_CHECK_EQUAL(value, 30); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(overwrite_does_not_grow_map) | ||
| { | ||
| IntCache cache(4); | ||
|
|
||
| // fill up to the threshold without triggering truncation | ||
| for (int i = 0; i < 8; i++) { | ||
| cache.insert(i, i); | ||
| } | ||
|
|
||
| // overwriting an existing key must not grow the map, so this stays at the threshold rather than exceeding it | ||
| cache.insert(0, 100); | ||
| for (int i = 0; i < 8; i++) { | ||
| BOOST_CHECK(cache.exists(i)); | ||
| } | ||
|
|
||
| int value{0}; | ||
| BOOST_CHECK(cache.get(0, value)); | ||
| BOOST_CHECK_EQUAL(value, 100); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_SUITE_END() |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -29,6 +29,8 @@ class unordered_lru_cache | |||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| // 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); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
29
to
34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking: Preserve smaller truncation thresholds The constructor previously accepted configurations such as
Suggested change
source: ['codex']
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is required because; 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved in Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread. |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| size_t max_size() const { return maxSize; } | ||||||||||||||||||||||||||
|
|
@@ -101,8 +103,8 @@ class unordered_lru_cache | |||||||||||||||||||||||||
| for (auto it = cacheMap.begin(); it != cacheMap.end(); ++it) { | ||||||||||||||||||||||||||
| vec.emplace_back(it); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For constructor calls such as Useful? React with 👍 / 👎. |
||||||||||||||||||||||||||
| return it1->second.second > it2->second.second; | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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::sortplus an erase loop starting atmaxSizemeant a configuration likecache(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 reachesstd::nth_element(vec.begin(), vec.begin() + maxSize, vec.end(), ...)withvec.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 ofassert(). Verified: no call site insrc/currently constructsunordered_lru_cache/Uint256LruHashMapwith a threshold belowmaxSize(checked all instantiations in creditpool.cpp, mnhftx.h, instantsend/db.h, llmq/*.h, saltedhasher.h). Verified: this repository's ownCMakeLists.txtsets no explicitCMAKE_BUILD_TYPE/NDEBUGhandling, so a standard-DCMAKE_BUILD_TYPE=Releaseconfiguration relies on CMake's built-in default flags, which do defineNDEBUGand disableassert()— the risk is not purely theoretical. That said, this exactly mirrors the pre-existing, previously-unflaggedassert(nPruneAfterSize >= nMaxSize)contract inlimitedmap.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.source: ['claude', 'codex']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.