perf: avoid heavy object copy during ApplyDiff for CDeterministicMNList#6680
Conversation
WalkthroughThe changes update the method CDeterministicMNList::ApplyDiff to perform in-place modifications on the current object instead of returning a new modified copy. The method's signature is changed from returning a CDeterministicMNList to returning void, and it is no longer marked as const. All usages of ApplyDiff throughout the codebase, including in CDeterministicMNManager and related tests, are updated to reflect this new in-place behavior. Additionally, a clarifying comment is added to the method declaration, and variable naming in the tests is corrected for consistency. No other exported or public entity signatures are altered. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between 0de700400af3db43bdfb45ea40942dd86381d34d and 73b68fe. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (2)
✨ 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
🧹 Nitpick comments (2)
src/evo/deterministicmns.h (1)
370-375: Polish the new doc-commentNice to see the explanatory comment, but there are two minor nits that may cause confusion:
- Typo: duplicated words “up to up to”.
{DISK_SNAPSHOT_PERIOD}is a literal placeholder, so anyone reading the header (e.g. in an IDE) will not immediately know the magnitude you’re referring to. Either spell out the constant or add a short note.- * Calculating for old block may take up to up to {DISK_SNAPSHOT_PERIOD} object copying and destroying. + * Calculating for old blocks may take up to DISK_SNAPSHOT_PERIOD object copies and destructions.src/evo/deterministicmns.cpp (1)
1095-1098: Minor optimisation opportunity for empty diffs
GetListForBlockInternalnow always callsApplyDiff, even whendiff.HasChanges()==false.
Given the added assert/early-return proposal inApplyDiff, the overhead is negligible, but if you prefer to avoid the extra function call entirely you could keep the previous short-circuit.Purely optional; feel free to ignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between da8a475 and 0de700400af3db43bdfb45ea40942dd86381d34d.
📒 Files selected for processing (3)
src/evo/deterministicmns.cpp(3 hunks)src/evo/deterministicmns.h(1 hunks)src/test/evo_deterministicmns_tests.cpp(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/evo/deterministicmns.cpp (1)
src/evo/deterministicmns.h (9)
pindex(375-375)pindex(592-592)pindex(594-594)pindex(603-603)pindex(616-616)dmn(304-304)dmn(305-305)dmn(367-367)dmn(377-377)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (2)
src/evo/deterministicmns.cpp (1)
708-710: Verify diff direction in UndoBlock
UndoBlocknow reconstructscurListby copyingprevListand applying the forward diff from prev → cur.
That produces the state being undone, which is what we later use to build the inverse diff, so the logic is sound.Just double-check that the
diffread from disk is always prev → cur; if at some point it’s stored inverted, the new call pattern will silently generate a corrupt list.src/test/evo_deterministicmns_tests.cpp (1)
372-376: Good adaptation to in-placeApplyDiffThe test update mirrors the production change correctly: copying
base_listand mutating it in-place preserves the original object for later assertions.
src/evo/deterministicmns.cpp
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add consistency guards inside ApplyDiff
The in-place variant looks solid, but a few inexpensive assertions would harden it even more:
-
assert(diff.nHeight == pindex->nHeight);
– Helps detect accidental mismatches when we pass the wrong diff/pindex pair. -
Early exit when the diff is empty to skip hash/height updates in the no-op case (micro-optimisation but costs nothing).
void CDeterministicMNList::ApplyDiff(gsl::not_null<const CBlockIndex*> pindex, const CDeterministicMNListDiff& diff)
{
+ // Sanity: the diff we’re applying must belong to the same block.
+ assert(diff.nHeight == -1 || diff.nHeight == pindex->nHeight);
+ if (!diff.HasChanges()) {
+ blockHash = pindex->GetBlockHash();
+ nHeight = pindex->nHeight;
+ return;
+ }Those checks catch logical errors early without any runtime penalty in production builds (asserts are stripped in -DNDEBUG).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void CDeterministicMNList::ApplyDiff(gsl::not_null<const CBlockIndex*> pindex, const CDeterministicMNListDiff& diff) | |
| { | |
| CDeterministicMNList result = *this; | |
| result.blockHash = pindex->GetBlockHash(); | |
| result.nHeight = pindex->nHeight; | |
| blockHash = pindex->GetBlockHash(); | |
| nHeight = pindex->nHeight; | |
| for (const auto& id : diff.removedMns) { | |
| auto dmn = result.GetMNByInternalId(id); | |
| auto dmn = GetMNByInternalId(id); | |
| if (!dmn) { | |
| throw std::runtime_error(strprintf("%s: can't find a removed masternode, id=%d", __func__, id)); | |
| } | |
| result.RemoveMN(dmn->proTxHash); | |
| RemoveMN(dmn->proTxHash); | |
| } | |
| for (const auto& dmn : diff.addedMNs) { | |
| result.AddMN(dmn); | |
| AddMN(dmn); | |
| } | |
| for (const auto& p : diff.updatedMNs) { | |
| auto dmn = result.GetMNByInternalId(p.first); | |
| auto dmn = GetMNByInternalId(p.first); | |
| if (!dmn) { | |
| throw std::runtime_error(strprintf("%s: can't find an updated masternode, id=%d", __func__, p.first)); | |
| } | |
| result.UpdateMN(*dmn, p.second); | |
| UpdateMN(*dmn, p.second); | |
| } | |
| return result; | |
| } | |
| void CDeterministicMNList::ApplyDiff(gsl::not_null<const CBlockIndex*> pindex, const CDeterministicMNListDiff& diff) | |
| { | |
| // Sanity: the diff we’re applying must belong to the same block. | |
| assert(diff.nHeight == -1 || diff.nHeight == pindex->nHeight); | |
| if (!diff.HasChanges()) { | |
| blockHash = pindex->GetBlockHash(); | |
| nHeight = pindex->nHeight; | |
| return; | |
| } | |
| blockHash = pindex->GetBlockHash(); | |
| nHeight = pindex->nHeight; | |
| for (const auto& id : diff.removedMns) { | |
| auto dmn = GetMNByInternalId(id); | |
| if (!dmn) { | |
| throw std::runtime_error(strprintf("%s: can't find a removed masternode, id=%d", __func__, id)); | |
| } | |
| RemoveMN(dmn->proTxHash); | |
| } | |
| for (const auto& dmn : diff.addedMNs) { | |
| AddMN(dmn); | |
| } | |
| for (const auto& p : diff.updatedMNs) { | |
| auto dmn = GetMNByInternalId(p.first); | |
| if (!dmn) { | |
| throw std::runtime_error(strprintf("%s: can't find an updated masternode, id=%d", __func__, p.first)); | |
| } | |
| UpdateMN(*dmn, p.second); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/evo/deterministicmns.cpp around lines 426 to 448, add an assertion at the
start of ApplyDiff to verify that diff.nHeight equals pindex->nHeight to ensure
consistency between the diff and block index. Also, add an early return if the
diff is empty (no removed, added, or updated masternodes) to avoid unnecessary
updates to blockHash and nHeight. These changes improve robustness and
efficiency without affecting production performance due to asserts being
disabled in release builds.
knst
force-pushed
the
perf-GetListForBlockInternal
branch
from
0de7004 to
73b68fe
Compare
| snapshot.SetBlockHash(diffIndex->GetBlockHash()); | ||
| snapshot.SetHeight(diffIndex->nHeight); | ||
| } | ||
| snapshot.ApplyDiff(diffIndex, diff); |
There was a problem hiding this comment.
Am I correct that this is the only place we have a performance improvement right? All other instances, just now have an explicit copy instead of implicit copy. This change does seem worth it, but only this specific one in GetListForBlockInternal has any perf improvement
There was a problem hiding this comment.
not counting unit tests, there are only 2 usages of ApplyDiff: GetListForBlockInternal and in UndoBlock. GetListForBlockInternal will be benefitial from this optimisation; undo block - doesn't become slower either faster.
…r CDeterministicMNList 73b68fe perf: avoid heavy object copy during ApplyDiff for CDeterministicMNList (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented The function `ApplyDiff` for CDeterministicMNList creates copy of current object and return it. It is not efficient in case of applying several diffs one after one. In case if `UndoBlock` and in case of calculating Quorum Members for old blocks (without caches) up to 576 temporary copies of CDeterministicMNList may be created and destroyed. ## What was done? `ApplyDiff` now changes current object, not returning a copy. It speed ups calculation of GetDeterministicMNListInternal for ~10%; this helper is most hot function during: - block undo - RPC `quorum info` for distant quorums (when it's not in cache). - RPC `quorum verify` for distant quorums (when it's not in cache) ## How Has This Been Tested? Run unit / functional tests Collected `perf` stats for block-undo. develop: <img width="772" alt="image" src="https://github.com/user-attachments/assets/a74e5dbb-314e-4067-b0dc-372235d37fb5" /> PR: <img width="772" alt="image" src="https://github.com/user-attachments/assets/a587cb6d-96f1-4932-9143-90b7a08b2751" /> ## Breaking Changes n/a ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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: ACK 73b68fe PastaPastaPasta: utACK 73b68fe Tree-SHA512: cd82e35bdef6793d5bb0ea2189a66b01e50f318dbefd0283dab2973f6df8d99a731cef7b49aed5e8f9ca54c2282ab5570a35224e34c6d10447caeb9e097a31e5
3 participants
Issue being fixed or feature implemented
The function
ApplyDifffor CDeterministicMNList creates copy of current object and return it.It is not efficient in case of applying several diffs one after one.
In case if
UndoBlockand in case of calculating Quorum Members for old blocks (without caches) up to 576 temporary copies of CDeterministicMNList may be created and destroyed.What was done?
ApplyDiffnow changes current object, not returning a copy.It speed ups calculation of GetDeterministicMNListInternal for ~10%; this helper is most hot function during:
quorum infofor distant quorums (when it's not in cache).quorum verifyfor distant quorums (when it's not in cache)How Has This Been Tested?
Run unit / functional tests
Collected
perfstats for block-undo.develop:
PR:
Breaking Changes
n/a
Checklist: