Skip to content

perf: add in quorumBaseBlockIndexCache to reduce cs_main contention #6422

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.

Sign up for GitHub

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

Merged
PastaPastaPasta merged 1 commit into from
Nov 22, 2024
Merged

perf: add in quorumBaseBlockIndexCache to reduce cs_main contention #6422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/llmq/quorums.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,20 @@ std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqTyp

CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash) const
{
const CBlockIndex* pQuorumBaseBlockIndex = WITH_LOCK(cs_main, return m_chainstate.m_blockman.LookupBlockIndex(quorumHash));
const CBlockIndex* pQuorumBaseBlockIndex = [&]() {
// Lock contention may still be high here; consider using a shared lock
// We cannot hold cs_quorumBaseBlockIndexCache the whole time as that creates lock-order inversion with cs_main;
// We cannot aquire cs_main if we have cs_quorumBaseBlockIndexCache held
const CBlockIndex* pindex;
if (!WITH_LOCK(cs_quorumBaseBlockIndexCache, return quorumBaseBlockIndexCache.get(quorumHash, pindex))) {
pindex = WITH_LOCK(cs_main, return m_chainstate.m_blockman.LookupBlockIndex(quorumHash));
if (pindex) {
LOCK(cs_quorumBaseBlockIndexCache);
quorumBaseBlockIndexCache.insert(quorumHash, pindex);
}
}
return pindex;
}();
if (!pQuorumBaseBlockIndex) {
LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- block %s not found\n", __func__, quorumHash.ToString());
return nullptr;
Expand Down
4 changes: 4 additions & 0 deletions src/llmq/quorums.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,10 @@ class CQuorumManager
mutable Mutex cs_cleanup;
mutable std::map<Consensus::LLMQType, unordered_lru_cache<uint256, uint256, StaticSaltedHasher>> cleanupQuorumsCache GUARDED_BY(cs_cleanup);

mutable Mutex cs_quorumBaseBlockIndexCache;
// On mainnet, we have around 62 quorums active at any point; let's cache a little more than double that to be safe.
mutable unordered_lru_cache<uint256 /*quorum_hash*/, const CBlockIndex* /*pindex*/, StaticSaltedHasher, 128 /*max_size*/> quorumBaseBlockIndexCache;
Copy link
Copy Markdown
Collaborator

@knst knst Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: accordingly code-style, members should start for m_

Class member variables have a m_ prefix.
https://github.com/dashpay/dash/blob/master/doc/developer-notes.md?plain=1#L88-L90

Suggested change
mutable unordered_lru_cache<uint256 /*quorum_hash*/, const CBlockIndex* /*pindex*/, StaticSaltedHasher, 128 /*max_size*/> quorumBaseBlockIndexCache;
mutable unordered_lru_cache<uint256 /*quorum_hash*/, const CBlockIndex* /*pindex*/, StaticSaltedHasher, 128 /*max_size*/> m_baseblock_index_cache;

Copy link
Copy Markdown
Member Author

@PastaPastaPasta PastaPastaPasta Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah; we should probably do a refactor for whole dash headers like this and rename. Thoughts?

Copy link
Copy Markdown
Collaborator

@knst knst Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for whole dash headers

I would not do it. Adding new one - add them in new style. Renaming all old one just for sack of renaming doesn't worth IMO. But in case of any refactoring around it is a good cause to do it.

For example, bitcoin does it everytime when code is refactored, for example bitcoin#21148 - created new class, moved data there and all members got proper nice names such as "mapOrphanTransactionsByPrev" -> "m_outpoint_to_orphan_it"


mutable ctpl::thread_pool workerPool;
mutable CThreadInterrupt quorumThreadInterrupt;

Expand Down