refactor: improve unordered_lru_cache #6965
Conversation
…onal Change unordered_lru_cache::get() from returning bool with an output parameter to returning std::optional<Value> by value. This provides a more modern API that is easier to use and less error-prone. - Add <optional> header to unordered_lru_cache.h - Update get() signature: bool get(key, Value&) -> std::optional<Value> get(key) - Update all 21 call sites across 11 files to use the new API - Maintain same copy semantics for performance (no performance impact) Files updated: - src/unordered_lru_cache.h - src/llmq/quorums.cpp (5 call sites) - src/llmq/utils.cpp (2 call sites) - src/llmq/snapshot.cpp, signing.cpp, dkgsessionmgr.cpp, blockprocessor.cpp - src/instantsend/db.cpp (3 call sites) - src/evo/mnhftx.cpp, creditpool.cpp, cbtx.cpp - src/masternode/meta.cpp
Add Boost.Test suite with 17 test cases covering: - Construction and configuration (runtime/compile-time sizing) - Basic API behavior (insert, get, emplace, exists, erase, clear) - LRU and truncation semantics (threshold behavior, recency tracking) - Edge cases (maxSize=1, various threshold combinations) Also optimize truncate_if_needed() to use std::nth_element instead of std::sort for O(N) average complexity instead of O(N log N).
|
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llmq/quorums.cpp (1)
586-591: Don’t drop already cached quorums at genesis boundary.When
vecResultQuorums.back()->m_quorum_base_block_index->pprevisnullptrwe’ve hit the beginning of the chain. Returning{}here discards the quorums we already collected from the cache, so callers get an empty result even though valid quorums exist. Instead of bailing out with an empty vector, return the quorums we have (or otherwise stop scanning without erasing them).- if (vecResultQuorums.back()->m_quorum_base_block_index->pprev == nullptr) return {}; + if (vecResultQuorums.back()->m_quorum_base_block_index->pprev == nullptr) { + return vecResultQuorums; + }
🧹 Nitpick comments (1)
src/unordered_lru_cache.h (1)
60-68: Consider direct return for efficiency.Line 65 uses
std::make_optional(it->second.first)which creates an extra copy. You could return the value directly:- return std::make_optional(it->second.first); + return it->second.first;When returning from a function that returns
std::optional<T>, the compiler will automatically construct the optional from the value, avoiding the intermediate copy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
-
src/Makefile.test.include(1 hunks) -
src/evo/cbtx.cpp(1 hunks) -
src/evo/creditpool.cpp(2 hunks) -
src/evo/mnhftx.cpp(1 hunks) -
src/instantsend/db.cpp(3 hunks) -
src/llmq/blockprocessor.cpp(1 hunks) -
src/llmq/dkgsessionmgr.cpp(1 hunks) -
src/llmq/quorums.cpp(5 hunks) -
src/llmq/signing.cpp(3 hunks) -
src/llmq/snapshot.cpp(1 hunks) -
src/llmq/utils.cpp(2 hunks) -
src/masternode/meta.cpp(1 hunks) -
src/test/unordered_lru_cache_tests.cpp(1 hunks) -
src/unordered_lru_cache.h(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/masternode/meta.cppsrc/evo/cbtx.cppsrc/evo/creditpool.cppsrc/llmq/signing.cppsrc/llmq/utils.cppsrc/llmq/snapshot.cppsrc/unordered_lru_cache.hsrc/test/unordered_lru_cache_tests.cppsrc/llmq/blockprocessor.cppsrc/llmq/quorums.cppsrc/llmq/dkgsessionmgr.cppsrc/evo/mnhftx.cppsrc/instantsend/db.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/meta.cppsrc/evo/cbtx.cppsrc/evo/creditpool.cppsrc/evo/mnhftx.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/unordered_lru_cache_tests.cpp
🧠 Learnings (7)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Applied to files:
src/Makefile.test.includesrc/test/unordered_lru_cache_tests.cpp
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
src/Makefile.test.include
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/evo/cbtx.cppsrc/evo/creditpool.cppsrc/test/unordered_lru_cache_tests.cppsrc/instantsend/db.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/evo/cbtx.cppsrc/llmq/quorums.cppsrc/llmq/dkgsessionmgr.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/evo/creditpool.cppsrc/llmq/blockprocessor.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash uses unordered_lru_cache for efficient caching with LRU eviction
Applied to files:
src/test/unordered_lru_cache_tests.cpp
🧬 Code graph analysis (2)
src/unordered_lru_cache.h (1)
src/script/descriptor.cpp (1)
nullopt(656-656)
src/llmq/dkgsessionmgr.cpp (2)
src/evo/mnhftx.h (1)
quorumHash(39-39)src/llmq/commitment.h (1)
quorumIndex(54-54)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/utils.cpp
[error] 251-257: Clang format differences detected. The step 'git diff -U0 origin/develop --
🔇 Additional comments (10)
src/unordered_lru_cache.h (1)
105-112: LGTM! Excellent performance improvement.The optimization from
std::sorttostd::nth_elementis well-implemented. After partitioning, the firstkeepelements are guaranteed to be the most recently accessed entries, and erasing entries beyond that point is correct. This reduces the average time complexity from O(N log N) to O(N) for the eviction process.src/instantsend/db.cpp (3)
274-278: LGTM! Clean optional-based cache lookup.The early return pattern for cache hits is clean and efficient. The cache is consulted only when
use_cacheis true, and the cached value is returned immediately if present.
298-306: LGTM! Correct cache integration.The cache lookup correctly returns early if a cached value is found. The DB read fallback and cache insertion are properly sequenced.
318-326: LGTM! Proper two-level cache lookup.The function correctly uses the outpoint cache to retrieve the islock hash, then delegates to
GetInstantSendLockByHashInternalfor the full lock retrieval. The DB fallback and cache insertion logic are correct.src/test/unordered_lru_cache_tests.cpp (1)
1-401: Excellent comprehensive test coverage!The test suite thoroughly validates the unordered_lru_cache implementation across multiple dimensions:
- Construction and configuration scenarios
- Basic operations (insert, get, emplace, exists, erase, clear)
- LRU eviction behavior and recency tracking
- Edge cases (maxsize=1, threshold variations, multiple truncation cycles)
- Value semantics (verifying returned values are copies)
The tests properly verify the new optional-based API and ensure correct LRU semantics.
src/Makefile.test.include (1)
199-199: LGTM! Test file properly added to build.The new test file is correctly added to the BITCOIN_TESTS list in alphabetical order.
src/masternode/meta.cpp (1)
142-145: LGTM! Clean optional-based cache lookup.The cache integration correctly uses the new optional API, returning the cached value if present or
std::nulloptotherwise.src/llmq/blockprocessor.cpp (1)
516-527: LGTM! Correct cache integration with proper locking.The optional-based cache lookup is correct. The cache check is properly guarded by the lock, and the DB read fallback occurs outside the lock to minimize lock contention. The cache insertion before returning is correct.
src/llmq/snapshot.cpp (1)
355-357: LGTM! Clean optional-based cache integration.The cache lookup correctly uses the new optional API with an early return on cache hit, followed by DB fallback and cache insertion.
src/evo/mnhftx.cpp (1)
328-330: LGTM! Correct optional-based cache lookup.The cache integration properly uses the new optional API with an early return on cache hit. The multiple DB fallback paths with cache insertions are correctly implemented.
| LOCK(cache_mutex); | ||
| if (creditPoolCache.get(block_hash, pool)) { | ||
| return pool; | ||
| if (auto cached = creditPoolCache.get(block_hash)) { |
There was a problem hiding this comment.
should reduce scope of pool here
| static_cast<size_t>(Params().CreditPoolPeriodBlocks()) * 2}; | ||
| if (LOCK(cache_mutex); block_data_cache.get(block_index->GetBlockHash(), blockData)) { | ||
| return blockData; | ||
| if (LOCK(cache_mutex); auto cached = block_data_cache.get(block_index->GetBlockHash())) { |
There was a problem hiding this comment.
nit(perf): should reduce scope of blockData here to avoid empty object creation
src/instantsend/db.cpp
Outdated
| } | ||
| } | ||
|
|
||
| InstantSendLockPtr ret; |
There was a problem hiding this comment.
- InstantSendLockPtr ret;
- ret = std::make_shared<InstantSendLock>();
+ InstantSendLockPtr ret{std::make_shared<InstantSendLock>()};
| if (!cached.has_value()) { | ||
| return errorHandler("Quorum not found", 0); // Don't bump score because we asked for it | ||
| } | ||
| pQuorum = *cached; |
There was a problem hiding this comment.
use std::optional's feature value_or
There was a problem hiding this comment.
why?
I gave a 2nd look, it just shared_ptr; so, copy is quite fast, no allocation, one atomic is relatively cheap.
Though, it could be:
- CQuorumPtr pQuorum;
- {
- LOCK(cs_map_quorums);
- auto cached = mapQuorumsCache[request.GetLLMQType()].get(request.GetQuorumHash());
- if (!cached.has_value()) {
- return errorHandler("Quorum not found", 0); // Don't bump score because we asked for it
- }
- pQuorum = *cached;
+ CQuorumPtr pQuorum = WITH_LOCK(cs_map_quorums, return mapQuorumsCache[request.GetLLMQType()].get(request.GetQuorumHash())).value_or(nullptr);
+ if (pQuorum == nullptr) {
+ return errorHandler("Quorum not found", 0); // Don't bump score because we asked for it
}
There was a problem hiding this comment.
I believe compiler may optimize away this copy anyway as the obj was not initialized prior.
src/evo/cbtx.cpp
Outdated
|
|
||
| std::pair<uint256, int> qc_hash; | ||
| if (!qc_hashes_cached[llmqType].get(block_hash, qc_hash)) { | ||
| if (auto cached = qc_hashes_cached[llmqType].get(block_hash)) { |
There was a problem hiding this comment.
this particular case is not good anymore; here's 1 extra constructor of uint256 is called now.
once - default constructor.
After that one more in get()
and 3rd one is 'qc_hash=*cached'
Used to be only 2 constructors:
- default for qc_hash
- assignment in get()
std::optional supposed to decrease its amount, not increase!
Consider use std::optional's feature value_or
| bool fCacheExists = cache.get(pindexStore->GetBlockHash(), vecResultQuorums); | ||
| if (fCacheExists) { | ||
| if (auto cached = cache.get(pindexStore->GetBlockHash())) { | ||
| vecResultQuorums = *cached; |
There was a problem hiding this comment.
consider using std::optional's feature value_or
There was a problem hiding this comment.
indeed, this place is not feasible; not easy to do due to if (...) and extra code inside
Consider vecResultQuorums = std::move(*cached)
@coderabbitai, check that std::move is working with std::optional's values?
There was a problem hiding this comment.
uint256 is trivially copyable; there's no gain to move really
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/evo/cbtx.cpp (1)
95-97: Construct the cached pair in-place.
Switching tostd::optional::emplaceavoids creating an extra temporarystd::pair<uint256, int>when we fill a miss, trimming one moreuint256construction at virtually no cost to readability.- qc_hash = {::SerializeHash(pqc), rotation_enabled ? pqc.quorumIndex : 0}; + qc_hash.emplace(::SerializeHash(pqc), rotation_enabled ? pqc.quorumIndex : 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/evo/cbtx.cpp(1 hunks)src/evo/creditpool.cpp(3 hunks)src/instantsend/db.cpp(3 hunks)src/test/unordered_lru_cache_tests.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/unordered_lru_cache_tests.cpp
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/evo/cbtx.cppsrc/evo/creditpool.cppsrc/instantsend/db.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/cbtx.cppsrc/evo/creditpool.cpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/evo/creditpool.cpp
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/instantsend/db.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/instantsend/db.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (1)
src/instantsend/db.cpp (1)
274-291: Good call on caching null hits.
Persisting the miss inislockCachemeans we stop hammering LevelDB for the same absent hash, while write paths still refresh the entry once an IS-lock arrives.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
LGTM overall, one test feels incomplete though
| // Erasing non-existent key is a no-op | ||
| c.erase(999); |
There was a problem hiding this comment.
should probably insert another element e.g. (2, 20) at the start of this test and then check that it wasn't removed neither by erase(1) above nor by erase(999) here
3 participants
What was done?
How Has This Been Tested?
Compiles
Breaking Changes
Checklist: