mempool: Avoid needless vtx iteration during IBD - #32827
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32827. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
mzumsande
left a comment
mzumsande
left a comment
There was a problem hiding this comment.
"known to be empty" / "always empty" is too strong:
It's quite common for nodes that are not online 24/7 to be in IBD (because they have to catch up a few weeks/months) but have a non-empty mempool. This doesn't affect the approach, just the PR / commit message description.
|
Thanks, updated - though I don't think that's "Initial", just regular Block Download. |
Could make sense to include that change here as well? |
5b94b56 to
028f7e5
Compare
l0rinc
changed the title
removeForBlock during IBD
028f7e5 to
54f9cb8
Compare
|
Just to clarify this is just a refactor/cleanup and doesn't affect end-to-end IBD performance in a measurable way? |
|
That's my expectation, yes. Would you like me to measure it to sure? Edit: measured a full reindex, the effect is <1%, as expected |
|
crACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50 |
|
lgtm ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50 🌌 Show signatureSignature: |
mzumsande
left a comment
mzumsande
left a comment
There was a problem hiding this comment.
Code Review ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50
I think it makes sense not to do these iterations over block txns if these can never result in any action - even if it doesn't increase performance measurably.
ismaelsadeeq
left a comment
ismaelsadeeq
left a comment
There was a problem hiding this comment.
Code review ACK 54f9cb85c4be2c30be0eb89f29b76a4cbf6f1c50
During Initial Block Download, the mempool is usually empty, but `CTxMemPool::removeForBlock` is still called for every connected block where we: * iterate over every transaction in the block even though none will be found in the empty `mapTx`, always leaving `txs_removed_for_block` empty... * which is pre-allocated regardless with 40 bytes * vtx.size(), even though it will always remain empty. This change introduces a minor performance optimization by only executing the loop if any of the core mempool maps have any contents. The call to `MempoolTransactionsRemovedForBlock` and the updates to the rolling fee logic remain unchanged. The `removeForBlock` was also updated stylistically to match the surrounding methods and a clarification was added to clarify that it affects fee estimation as well.
|
Had to rebase after 067365d#diff-e6100361fa0e9e25478f808ca084e5f681d4dddbbee7b3bea0f9d5bcd29db3aaR39-R563, |
|
ACK 249889b |
| @@ -661,26 +661,25 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) | |||
| } | |||
There was a problem hiding this comment.
In "mempool: Avoid expensive loop in removeForBlock during IBD" 41ad2be
nit: change commit title to "mempool: avoid looping blk txs when mempool is empty in removeForBlock"
|
ACK 249889b |
9 participants
During Initial Block Download, the mempool is usually empty, but
CTxMemPool::removeForBlockis still called for every connected block where we:mapTx, always leavingtxs_removed_for_blockempty...40 bytes * vtx.size(), even though it will always remain empty.Similarly to #32730 (comment), this change introduces a minor performance & memory optimization by only executing the loop if any of the affected mempool maps have any contents. The second commit is cherry-picked from there since it's related to this change as well.