Skip to content

[IBD] coins: reduce lookups in dbcache layer propagation #34

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
l0rinc wants to merge 2 commits into from
Closed

[IBD] coins: reduce lookups in dbcache layer propagation #34

l0rinc wants to merge 2 commits into from

Conversation

@l0rinc
Copy link
Owner

@l0rinc l0rinc commented Sep 2, 2025 •
edited
Loading

Previously, when the parent coins cache had no entry and the child did, BatchWrite performed a find followed by try_emplace, which resulted in multiple SipHash computations and bucket traversals on the common insert path.

This change uses a single leading try_emplace and branches on the returned inserted flag. In the FRESH && SPENT case (only exercised by tests), we erase the just-inserted placeholder (which is constant time with no rehash anyway). Semantics are unchanged for all valid parent/child state combinations.

This change is a minimal version of bitcoin/bitcoin@723c49b (#32128) and draws simplification ideas bitcoin/bitcoin@ae76ec7 (#30673) and bitcoin#30326.

Related to parent cache propagation, the second commit makes it possible to avoid destructuring-recreating-destructuring of these short-live parent caches created for each new block.
A few temporary CCoinsViewCache's are destructed right after the Flush(), therefore it is not necessary to call ReallocateCache to recreate them right before they're killed anyway.

This change was based on a subset of bitcoin#28945.

Reindex-chainstate indicates ~1% speedup.

Details 
COMMITS="647cdb4f7e8041affed887e2325ee03a91078bb1 0b0c3293ffd75afb27dadc0b28426b40132a8c6b"; \                       
STOP=909090; DBCACHE=4500; \                                                                                                                                                     
CC=gcc; CXX=g++; \                                                                      
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \                                                                                        
(echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done; echo "") && \
hyperfine \                                                                             
  --sort command \                                                                      
  --runs 2 \                                                                            
  --export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
  --parameter-list COMMIT ${COMMITS// /,} \                                             
  --prepare "killall bitcoind 2>/dev/null; rm -f $DATA_DIR/debug.log; git checkout {COMMIT}; git clean -fxd; git reset --hard && \
    cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DENABLE_IPC=OFF && ninja -C build bitcoind && \
    ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 20" \
  --cleanup "cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \                                                                                                   
  "COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0"

647cdb4f7e Merge bitcoin/bitcoin#33311: net: Quiet down logging when router doesn't support natpmp/pcp
0b0c3293ff validation: don't reallocate cache for short-lived CCoinsViewCache                                                                                                    

Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 647cdb4f7e8041affed887e2325ee03a91078bb1)
  Time (mean ± σ):     16233.508 s ±  9.501 s    [User: 19064.578 s, System: 951.672 s]                                                                                          
  Range (min … max):   16226.790 s … 16240.226 s    2 runs                                                                                                                       
                                                                                        
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 0b0c3293ffd75afb27dadc0b28426b40132a8c6b)
  Time (mean ± σ):     16039.626 s ± 17.284 s    [User: 18870.130 s, System: 950.722 s]
  Range (min … max):   16027.405 s … 16051.848 s    2 runs
  
Relative speed comparison
        1.01 ±  0.00  COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 647cdb4f7e8041affed887e2325ee03a91078bb1)
        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 0b0c3293ffd75afb27dadc0b28426b40132a8c6b)

@l0rinc l0rinc mentioned this pull request Sep 2, 2025
Closed
@l0rinc l0rinc force-pushed the l0rinc/BatchWrite-lookup-optimization branch 2 times, most recently from 499baf5 to f333bdf Compare September 2, 2025 05:29
@l0rinc l0rinc changed the title coins: reduce lookups in dbcache layer propagation [IBD] coins: reduce lookups in dbcache layer propagation Sep 4, 2025
@l0rinc l0rinc force-pushed the l0rinc/BatchWrite-lookup-optimization branch 3 times, most recently from 4412abd to 0b0c329 Compare September 5, 2025 00:06
@l0rinc l0rinc closed this Sep 5, 2025
@l0rinc l0rinc reopened this Sep 5, 2025
@l0rinc l0rinc force-pushed the l0rinc/BatchWrite-lookup-optimization branch from 0b0c329 to 067e314 Compare September 5, 2025 01:05
@l0rinc l0rinc closed this Sep 5, 2025
@l0rinc l0rinc reopened this Sep 5, 2025
l0rinc and others added 2 commits September 5, 2025 09:24
@l0rinc @martinus @andrewtoth
coins: reduce lookups in dbcache layer propagation
b23c6a1 
Previously, when the parent coins cache had no entry and the child did, `BatchWrite` performed a find followed by `try_emplace`, which resulted in multiple `SipHash` computations and bucket traversals on the common insert path.

This change uses a single leading `try_emplace` and branches on the returned `inserted` flag.
In the `FRESH && SPENT` case (only exercised by tests), we erase the just-inserted placeholder (which is constant time with no rehash anyway).
Semantics are unchanged for all valid parent/child state combinations.

This change is a minimal version of bitcoin@723c49b and draws simplification ideas bitcoin@ae76ec7.

Co-authored-by: Martin Ankerl <[email protected]>
Co-authored-by: Andrew Toth <[email protected]>
@l0rinc @martinus
validation: don't reallocate cache for short-lived CCoinsViewCache
195fc2f 
A few temporary `CCoinsViewCache`'s are destructed right after the `Flush()`, therefore it is not necessary to call `ReallocateCache` to recreate them right before they're killed anyway.

* `Flush(/*reallocate_cache=*/true)` - retains existing functionality;
* `Flush(/*reallocate_cache=*/false)` - skips destruction and reallocation of the parent cache since it will soon go out of scope anyway;

This change was based on a subset of bitcoin#28945.

Co-authored-by: Martin Ankerl <[email protected]>
@l0rinc l0rinc force-pushed the l0rinc/BatchWrite-lookup-optimization branch from 067e314 to 195fc2f Compare September 5, 2025 16:24
@Raimo33
Copy link

Raimo33 commented Sep 12, 2025

Concept ACK for try_emplace to reduce find() calls. b23c6a1
Concept ACK for flushing only when needed. 195fc2f

Concept NACK on the rest. why are we setting the release notes for Core 29.1? Shouldn't qt PRs be separate?

I would refactor this into two commits focusing on the BatchWrite and Flush.
Also the diff on b23c6a1 can be made simpler. look at bitcoin#33376

@l0rinc
Copy link
Owner Author

l0rinc commented Sep 12, 2025

This is still on my own fork, i.e. l0rinc/bitcoin :)

Also the diff on b23c6a1 can be made simpler. look at bitcoin#33376

What makes you say that's simpler?

@l0rinc l0rinc closed this Sep 12, 2025
@l0rinc l0rinc reopened this Sep 12, 2025
@Raimo33
Copy link

Raimo33 commented Sep 12, 2025

oops. didn't realize.

What makes you say that's simpler?
I guess comments and the fact that I have the conditions swapped.

@l0rinc
Copy link
Owner Author

l0rinc commented Sep 12, 2025 •
edited
Loading

That's deliberate, it's simpler to mentally parse if-condition-else than if-not_condition-else

@l0rinc l0rinc closed this Oct 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@l0rinc @Raimo33