perf: add in quorumBaseBlockIndexCache to reduce cs_main contention#6422
Conversation
|
Guix Automation has failed due to the HEAD commit not being signed by an authorized core-team member. Please rebase and sign or push a new empty signed commit to allow Guix build to happen. |
PastaPastaPasta
force-pushed
the
refactor-minimize-cs_main-contention-quorum-signing-minimal
branch
from
0034da5 to
f827bfd
Compare
|
Guix Automation has failed due to the HEAD commit not being signed by an authorized core-team member. Please rebase and sign or push a new empty signed commit to allow Guix build to happen. |
PastaPastaPasta
force-pushed
the
refactor-minimize-cs_main-contention-quorum-signing-minimal
branch
from
f827bfd to
3d67771
Compare
|
Guix Automation has began to build this PR tagged as v22.1.0-devpr6422.3d67771f. A new comment will be made when the image is pushed. |
but it doesn't change observable behavior? maybe more of a |
|
It does change internal logic a bit though. |
PastaPastaPasta
changed the title
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v22.1.0-devpr6422.3d67771f. The image should be on dockerhub soon. |
|
As expected; after testnet testing, contention over cs_main related to this line went from 20% -> 0% On rc.1 cs_main contention represents ~30% of contention (although I previously observed it at ~40%), using this PR, cs_main contention was only ~18% of total contention. Still about 10% of this 18% is related to llmq/quorums, so should try to cache the chain tip as well. But this PR appears to be producing the expected results. |
|
|
||
| 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; |
There was a problem hiding this comment.
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
| 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; |
There was a problem hiding this comment.
Yeah; we should probably do a refactor for whole dash headers like this and rename. Thoughts?
There was a problem hiding this comment.
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"
4 participants
Issue being fixed or feature implemented
subset of #6418; only includes the new quorumBaseBlockIndexCache, doesn't include the caching of the chain-tip, as that introduced regressions I'm still debugging.
What was done?
introduce a LRU cache for quorumHash -> const CBlockIndex*; this should significantly reduce cs_main contention during high transaction load.
How Has This Been Tested?
Ran tests locally; let's see CI happy, and I also intend to run this on a testnet MN first and see the level of contention reduction
Breaking Changes
None
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.