-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor: improve unordered_lru_cache #6965
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
base: develop
Are you sure you want to change the base?
Changes from all commits
b08eab3
4b85c94
49ee1bd
aafef95
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 |
|---|---|---|
|
|
@@ -65,13 +65,11 @@ static std::optional<CreditPoolDataPerBlock> GetCreditDataFromBlock(const gsl::n | |
| return std::nullopt; | ||
| } | ||
|
|
||
| CreditPoolDataPerBlock blockData; | ||
|
|
||
| static Mutex cache_mutex; | ||
| static Uint256LruHashMap<CreditPoolDataPerBlock> block_data_cache GUARDED_BY(cache_mutex){ | ||
| 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())) { | ||
| return *cached; | ||
| } | ||
|
|
||
| CBlock block; | ||
|
|
@@ -84,7 +82,7 @@ static std::optional<CreditPoolDataPerBlock> GetCreditDataFromBlock(const gsl::n | |
| return std::nullopt; | ||
| } | ||
|
|
||
|
|
||
| CreditPoolDataPerBlock blockData; | ||
| if (const auto opt_cbTx = GetTxPayload<CCbTx>(block.vtx[0]->vExtraPayload); opt_cbTx) { | ||
| blockData.credit_pool = opt_cbTx->creditPoolBalance; | ||
| } else { | ||
|
|
@@ -120,15 +118,14 @@ std::optional<CCreditPool> CCreditPoolManager::GetFromCache(const CBlockIndex& b | |
| if (!DeploymentActiveAt(block_index, Params().GetConsensus(), Consensus::DEPLOYMENT_V20)) return CCreditPool{}; | ||
|
|
||
| const uint256 block_hash = block_index.GetBlockHash(); | ||
| CCreditPool pool; | ||
| { | ||
| LOCK(cache_mutex); | ||
| if (creditPoolCache.get(block_hash, pool)) { | ||
| return pool; | ||
| if (auto cached = creditPoolCache.get(block_hash)) { | ||
|
Collaborator
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. should reduce scope of |
||
| return *cached; | ||
| } | ||
| } | ||
| if (block_index.nHeight % DISK_SNAPSHOT_PERIOD == 0) { | ||
| if (evoDb.Read(std::make_pair(DB_CREDITPOOL_SNAPSHOT, block_hash), pool)) { | ||
| if (CCreditPool pool; evoDb.Read(std::make_pair(DB_CREDITPOOL_SNAPSHOT, block_hash), pool)) { | ||
| LOCK(cache_mutex); | ||
| creditPoolCache.insert(block_hash, pool); | ||
| return pool; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -571,8 +571,8 @@ std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqTyp | |
| } | ||
| } | ||
| auto& cache = scanQuorumsCache[llmqType]; | ||
| bool fCacheExists = cache.get(pindexStore->GetBlockHash(), vecResultQuorums); | ||
| if (fCacheExists) { | ||
| if (auto cached = cache.get(pindexStore->GetBlockHash())) { | ||
| vecResultQuorums = *cached; | ||
|
Collaborator
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. consider using std::optional's feature
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. why? / how?
Collaborator
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. indeed, this place is not feasible; not easy to do due to if (...) and extra code inside Consider @coderabbitai, check that std::move is working with std::optional's values?
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. uint256 is trivially copyable; there's no gain to move really |
||
| // We have exactly what requested so just return it | ||
| if (vecResultQuorums.size() == nCountRequested) { | ||
| return vecResultQuorums; | ||
|
|
@@ -642,12 +642,15 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint25 | |
| // We cannot hold cs_quorumBaseBlockIndexCache the whole time as that creates lock-order inversion with cs_main; | ||
| // We cannot acquire cs_main if we have cs_quorumBaseBlockIndexCache held | ||
| const CBlockIndex* pindex; | ||
| if (!WITH_LOCK(cs_quorumBaseBlockIndexCache, return quorumBaseBlockIndexCache.get(quorumHash, pindex))) { | ||
| auto cached = WITH_LOCK(cs_quorumBaseBlockIndexCache, return quorumBaseBlockIndexCache.get(quorumHash)); | ||
| if (!cached.has_value()) { | ||
| pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(quorumHash)); | ||
| if (pindex) { | ||
| LOCK(cs_quorumBaseBlockIndexCache); | ||
| quorumBaseBlockIndexCache.insert(quorumHash, pindex); | ||
| } | ||
| } else { | ||
| pindex = *cached; | ||
| } | ||
| return pindex; | ||
| }(); | ||
|
|
@@ -668,9 +671,8 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, gsl::not_nul | |
| return nullptr; | ||
| } | ||
|
|
||
| CQuorumPtr pQuorum; | ||
| if (LOCK(cs_map_quorums); mapQuorumsCache[llmqType].get(quorumHash, pQuorum)) { | ||
| return pQuorum; | ||
| if (LOCK(cs_map_quorums); auto cached = mapQuorumsCache[llmqType].get(quorumHash)) { | ||
| return *cached; | ||
| } | ||
|
|
||
| return BuildQuorumFromCommitment(llmqType, pQuorumBaseBlockIndex, populate_cache); | ||
|
|
@@ -833,9 +835,12 @@ MessageProcessingResult CQuorumManager::ProcessMessage(CNode& pfrom, CConnman& c | |
|
|
||
| CQuorumPtr pQuorum; | ||
| { | ||
| if (LOCK(cs_map_quorums); !mapQuorumsCache[request.GetLLMQType()].get(request.GetQuorumHash(), 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; | ||
|
Collaborator
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. use std::optional's feature
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. why / how?
Collaborator
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.
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:
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. I believe compiler may optimize away this copy anyway as the obj was not initialized prior. |
||
| } | ||
|
|
||
| // Check if request has QUORUM_VERIFICATION_VECTOR data | ||
|
|
@@ -1118,9 +1123,8 @@ void CQuorumManager::StartCleanupOldQuorumDataThread(const CBlockIndex* pIndex) | |
| const CBlockIndex* pindex_loop{pIndex}; | ||
| std::set<uint256> quorum_keys; | ||
| while (pindex_loop != nullptr && pIndex->nHeight - pindex_loop->nHeight < params.max_store_depth()) { | ||
| uint256 quorum_key; | ||
| if (cache.get(pindex_loop->GetBlockHash(), quorum_key)) { | ||
| quorum_keys.insert(quorum_key); | ||
| if (auto quorum_key = cache.get(pindex_loop->GetBlockHash())) { | ||
| quorum_keys.insert(*quorum_key); | ||
| if (quorum_keys.size() >= static_cast<size_t>(params.keepOldKeys)) break; // extra safety belt | ||
| } | ||
| pindex_loop = pindex_loop->pprev; | ||
|
|
||
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.
nit(perf): should reduce scope of
blockDatahere to avoid empty object creation