perf: optimize GetQuorumQuarterMembersBySnapshot calculation #6731
Conversation
…r to BuildNewQuorumQuarterMembers
… in ComputeQuorumMembers
… GetMNUsageBySnapshot
Masternodes are already sorted by modifier; no need to re-sort them
WalkthroughThe changes refactor quorum member selection functions to accept a 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/llmq/utils.cpp (1)
416-417: Parameter name inconsistency in function implementation.As mentioned in the function declaration, the parameter name
dmnmanshould be renamed tomn_listor similar to reflect that it's now aCDeterministicMNList.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/llmq/utils.cpp(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (5)
src/llmq/utils.cpp (5)
59-61: Function signature update looks good.The change to accept
CDeterministicMNListdirectly makes the masternode list source explicit and aligns with the performance optimization goals.
272-277: Explicit masternode list retrieval implementation is correct.The logic properly determines the work block index based on V20 activation status and explicitly retrieves the masternode list before passing it to
ComputeQuorumMembers.
284-297: Simplified implementation using direct masternode list.The refactored function correctly uses the provided
CDeterministicMNListdirectly, eliminating the need to fetch it from the manager.
315-316: Consistent pattern of explicit masternode list handling.The code correctly retrieves the masternode list and passes it to dependent functions, maintaining consistency with the refactoring pattern.
Also applies to: 326-327
616-638: Excellent performance optimization by eliminating redundant sorting.The refactored implementation efficiently:
- Sorts masternodes once with the modifier
- Separates them into used/unused while preserving sort order
- Constructs the combined list without additional sorting
This eliminates the redundant sorting that was previously done in
GetMNUsageBySnapshot, achieving the ~25% performance improvement mentioned in the PR objectives.
…unction; and the meaning
…calculation b7b04e5 fix: rename dmnman to allMns to match declaration and definition of function; and the meaning (Konstantin Akimov) e9e0f58 refactor: inline GetMNUsageBySnapshot to simplify code (Konstantin Akimov) 46383b1 perf: optimize GetQuorumQuarterMembersBySnapshot calculation (Konstantin Akimov) d337579 refactor: use CDeterministicMNList instead CDeterministicMNManager in GetMNUsageBySnapshot (Konstantin Akimov) 790ae0f refactor: use CDeterministicMNList instead of CDeterministicMNManager in ComputeQuorumMembers (Konstantin Akimov) ac934eb refactor: pass CDeterministicMNList instead of CDeterministicMNManager to BuildNewQuorumQuarterMembers (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Masternodes already sorted with the same modifier inside `GetMNUsageBySnapshot`, no need to re-sort them inside `GetQuorumQuarterMembersBySnapshot`. ## What was done? Removed extra sort of already sorted vectors; removed duplicated calculation of modifier, simplified `GetMNUsageBySnapshot` by inlining it. Also several methods are changed to accept CDeterministicMNList instead of CDeterministicMNManager. ## How Has This Been Tested? It improves significantly performance of `llmq::utils::GetAllQuorumMembers` which is widely used during block validation; for many RPC calls, for processing some network messages, for masternodes participating in quorums. Despite 25% improvement for performance of `llmq::utils::GetAllQuorumMembers` overall improvement for indexing is less than 1% of total CPU time (thanks to caches). `perf` for PR: <img width="838" alt="image" src="https://github.com/user-attachments/assets/cb98a0ef-3a4b-4d21-a674-80c300764712" /> `perf` for develop: <img width="838" alt="image" src="https://github.com/user-attachments/assets/879acdac-facd-4a11-b37a-607f4cb31e48" /> ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK b7b04e5 Tree-SHA512: 069e2a0a2b6ad7dfb092fdfa14e289ecde5bc25a59c7cb0c2fad10a56c9dbacb207b8f4bdd5be0eb88380fd745e0ff21ffc88fe5f24103d376cd3dc83f089787
…calculation b7b04e5 fix: rename dmnman to allMns to match declaration and definition of function; and the meaning (Konstantin Akimov) e9e0f58 refactor: inline GetMNUsageBySnapshot to simplify code (Konstantin Akimov) 46383b1 perf: optimize GetQuorumQuarterMembersBySnapshot calculation (Konstantin Akimov) d337579 refactor: use CDeterministicMNList instead CDeterministicMNManager in GetMNUsageBySnapshot (Konstantin Akimov) 790ae0f refactor: use CDeterministicMNList instead of CDeterministicMNManager in ComputeQuorumMembers (Konstantin Akimov) ac934eb refactor: pass CDeterministicMNList instead of CDeterministicMNManager to BuildNewQuorumQuarterMembers (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Masternodes already sorted with the same modifier inside `GetMNUsageBySnapshot`, no need to re-sort them inside `GetQuorumQuarterMembersBySnapshot`. ## What was done? Removed extra sort of already sorted vectors; removed duplicated calculation of modifier, simplified `GetMNUsageBySnapshot` by inlining it. Also several methods are changed to accept CDeterministicMNList instead of CDeterministicMNManager. ## How Has This Been Tested? It improves significantly performance of `llmq::utils::GetAllQuorumMembers` which is widely used during block validation; for many RPC calls, for processing some network messages, for masternodes participating in quorums. Despite 25% improvement for performance of `llmq::utils::GetAllQuorumMembers` overall improvement for indexing is less than 1% of total CPU time (thanks to caches). `perf` for PR: <img width="838" alt="image" src="https://github.com/user-attachments/assets/cb98a0ef-3a4b-4d21-a674-80c300764712" /> `perf` for develop: <img width="838" alt="image" src="https://github.com/user-attachments/assets/879acdac-facd-4a11-b37a-607f4cb31e48" /> ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK b7b04e5 Tree-SHA512: 069e2a0a2b6ad7dfb092fdfa14e289ecde5bc25a59c7cb0c2fad10a56c9dbacb207b8f4bdd5be0eb88380fd745e0ff21ffc88fe5f24103d376cd3dc83f089787
3 participants
Issue being fixed or feature implemented
Masternodes already sorted with the same modifier inside
GetMNUsageBySnapshot, no need to re-sort them insideGetQuorumQuarterMembersBySnapshot.What was done?
Removed extra sort of already sorted vectors; removed duplicated calculation of modifier, simplified
GetMNUsageBySnapshotby inlining it.Also several methods are changed to accept CDeterministicMNList instead of CDeterministicMNManager.
How Has This Been Tested?
It improves significantly performance of
llmq::utils::GetAllQuorumMemberswhich is widely used during block validation; for many RPC calls, for processing some network messages, for masternodes participating in quorums.Despite 25% improvement for performance of
llmq::utils::GetAllQuorumMembersoverall improvement for indexing is less than 1% of total CPU time (thanks to caches).perffor PR:perffor develop:Breaking Changes
N/A
Checklist: