Don't empty dbcache on prune flushes: >30% faster IBD #28280
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. |
andrewtoth
force-pushed
the
sync-dirty
branch
3 times, most recently
from
f5b20ca to
97b6bb5
Compare
andrewtoth
force-pushed
the
sync-dirty
branch
5 times, most recently
from
2f78edb to
7bdbf31
Compare
andrewtoth
force-pushed
the
sync-dirty
branch
3 times, most recently
from
d35cea3 to
b8ccdac
Compare
|
Concept ACK! I'll try to repro the bench results in the next week or so. |
|
@andrewtoth Not sure if you have seen #20827 which is a different approach but has a similar effect. |
|
@0xB10C indeed I have seen that one. I believe they complement each other. #20827 reduces IBD time for larger |
At the time I had a spare and otherwise idle machine set up I could use. I don't have the bash script at hand but IIRC I had a merge base (MB) and a pull-request (PR) binary I'd run with different I've uploaded the jupyter nodebooks here https://gist.github.com/0xB10C/89c2903290cfb1840792d41dcd079646. The first was used for a partial IBD from 500k-600k and the second one with a full IBD. Both times syncing from a local node on the same host. The notebooks expect the debug log files with the name |
|
Bench running now, should have some results in the next day or so. $ ( bitcoinperf --no-teardown bench-pr --num-blocks 30_000 --run-id no-flush-on-prune \
--bitcoind-args='-dbcache=4000 -prune=550' --run-count 3 28280 && \
pushover 'Bench for andrewtoth SUCCEEDED!' ) || \
pushover 'Bench for andrewtoth failed' |
|
Local IBD of 30_000 blocks with |
Summary: No behavior change. Prepares for adding the CoinsCachePairs to a linked list when flagged. This is a partial backport of [[bitcoin/bitcoin#28280 | core#28280]] bitcoin/bitcoin@8bd3959 Depends on D18614 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18615
Summary: No behavior change. Prepares for flags adding CCoinsCacheEntrys to a linked list which need to be removed on destruction. This is a partial backport of [[bitcoin/bitcoin#28280 | core#28280]] bitcoin/bitcoin@58b7ed1 Depends on D18615 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18616
Summary: No visible behavior change. This commit tracks the flagged entries internally but the list is not iterated by anything. Co-Authored-By: Pieter Wuille <[email protected]> Co-Authored-By: l0rinc <[email protected]> This is a partial backport of [[bitcoin/bitcoin#28280 | core#28280]] bitcoin/bitcoin@24ce37c bitcoin/bitcoin@a14edad Depends on D18616 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18617
Summary: BatchWrite now iterates through the linked list of flagged entries instead of the entire coinsCache map. Co-Authored-By: l0rinc <[email protected]> This is a partial backport of [[bitcoin/bitcoin#28280 | core#28280]] bitcoin/bitcoin@7825b8b Depends on D18617 Test Plan: `ninja all check-all bitcoin-fuzzers` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18618
Summary: Erase spent cache entries and clear flags of unspent entries inside the BatchWrite loop, instead of an additional loop after BatchWrite. Co-Authored-By: Pieter Wuille <[email protected]> This is a partial backport of [[bitcoin/bitcoin#28280 | core#28280]] bitcoin/bitcoin@05cf4e1 Depends on D18618 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18619
Summary: This is a partial backport of [[bitcoin/bitcoin#28280 | core#28280]] bitcoin/bitcoin@0e89187 Depends on D18619 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18620
Summary: This concludes backport of [[bitcoin/bitcoin#28280 | core#28280]] bitcoin/bitcoin@589db87 Depends on D18620 Test Plan: ``` $ src/bitcoind -datadir=/data0/ecashd_test_pruning_ibd/ -dbcache=16384 -prune=550 2025-09-14T19:53:14Z Bitcoin ABC version v0.31.12-a36e609718a2 (release build) ... 2025-09-14T21:37:21Z Leaving InitialBlockDownload (latching to false) ... 2025-09-14T21:37:48Z UpdateTip: new best=000000000000000015e71b7a821a81f8f35ed3bf946c20ec1cb7722925e07dec height=914517 version=0x2e000000 log2_work=88.533540 tx=299825555 date='2025-09-14T21:37:41Z' progress=1.000000 cache=6914.2MiB(39061252txo) ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18621
Adds `m_dirty_count` member to track the running count of dirty cache entries as follows: * Incremented when entries are marked dirty via `CCoinsCacheEntry::SetDirty` * Decremented when dirty entries are removed or cleaned * Passed through `CoinsViewCacheCursor` and updated during iteration * Validated in `SanityCheck()` by recomputing from scratch The dirty count is needed because after non-wiping flushes (introduced in bitcoin#28280 and bitcoin#28233), the percentage of dirty entries in the cache may be far below 100%. Using total cache size for flush warnings and disk space checks is therefore misleading. Updates all test code to properly initialize and maintain the dirty count. Co-authored-by: l0rinc <[email protected]>
Adds `m_dirty_count` member to track the running count of dirty cache entries as follows: * Incremented when entries are marked dirty via `CCoinsCacheEntry::SetDirty` * Decremented when dirty entries are removed or cleaned * Passed through `CoinsViewCacheCursor` and updated during iteration * Validated in `SanityCheck()` by recomputing from scratch The dirty count is needed because after non-wiping flushes (introduced in bitcoin#28280 and bitcoin#28233), the percentage of dirty entries in the cache may be far below 100%. Using total cache size for flush warnings and disk space checks is therefore misleading. Updates all test code to properly initialize and maintain the dirty count. Co-authored-by: l0rinc <[email protected]>
Since bitcoin#28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful. Split the FlushStateMode::ALWAYS mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in scantxoutset, gettxoutsetinfo, snapshot creation. Co-authored-by: l0rinc <[email protected]> Co-authored-by: cedwies <[email protected]>
Adds `m_dirty_count` member to track the running count of dirty cache entries as follows: * Incremented when entries are marked dirty via `CCoinsCacheEntry::SetDirty` * Decremented when dirty entries are removed or cleaned * Passed through `CoinsViewCacheCursor` and updated during iteration * Validated in `SanityCheck()` by recomputing from scratch The dirty count is needed because after non-wiping flushes (introduced in bitcoin#28280 and bitcoin#28233), the percentage of dirty entries in the cache may be far below 100%. Using total cache size for flush warnings and disk space checks is therefore misleading. Updates all test code to properly initialize and maintain the dirty count. Co-authored-by: l0rinc <[email protected]>
Since bitcoin#28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful. Split the FlushStateMode::ALWAYS mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in scantxoutset, gettxoutsetinfo, snapshot creation. Co-authored-by: l0rinc <[email protected]> Co-authored-by: cedwies <[email protected]>
16 participants
Since #17487 we no longer need to clear the coins cache when syncing to disk. A warm coins cache significantly speeds up block connection, and only needs to be fully flushed when nearing the
dbcachelimit.For frequent pruning flushes there's no need to empty the cache and kill connect block speed. However, simply using
Syncin place ofFlushactually slows down a pruned full IBD with a highdbcachevalue. This is because as the cache grows, sync takes longer since every coin in the cache is scanned to check if it's dirty. For frequent prune flushes and a large cache this constant scanning starts to really slow IBD down, and just emptying the cache on every prune becomes faster.To fix this, we can add two pointers to each cache entry and construct a doubly linked list of dirty entries. We can then only iterate through all dirty entries on each
Sync, and simply clear the pointers after.With this approach a full IBD with
dbcache=16384andprune=550was 32% faster than master. For defaultdbcache=450speedup was ~9%. All benchmarks were run withstopatheight=800000.While the 2 pointers add memory to each cache entry, it did not slow down IBD. For non-pruned IBD results were similar for this branch and master. When I performed the initial IBD, the full UTXO set could be held in memory when using the max
dbcachevalue. For non-pruned IBD with maxdbcacheto tip ended up using 12% more memory, but it was also 2.7% faster somehow. For smallerdbcachevalues thedbcachelimit is respected so does not consume more memory, and the potentially more frequent flushes were not significant enough to cause any slowdown.For reviewers, the commits in order do the following:
First 4 commits encapsulate all accesses to
flagson cache entries, and then the 5th makesflagsprivate.Commits
refactor: add CoinsCachePair aliastocoins: call ClearFlags in CCoinsCacheEntry destructorcreate the linked list head nodes and cache entry self references and pass them intoAddFlags.Commit
coins: track flagged cache entries in linked listactually adds the entries into a linked list when they are flagged DIRTY or FRESH and removes them from the linked list when they are destroyed or the flags are cleared manually. However, the linked list is not yet used anywhere.Commit
test: add cache entry linked list testsadds unit tests for the linked list.Commit
coins: pass linked list of flagged entries to BatchWriteuses the linked list to iterate through DIRTY entries instead of using the entire coins cache.Commit
validation: don't erase coins cache on prune flushesusesSyncinstead ofFlushfor pruning flushes, so the cache is no longer cleared.Inspired by this comment.
Fixes #11315.