[WIP] cache: remove redundant find() call #33376

pull Raimo33 wants to merge 1 commits into bitcoin:master from Raimo33:batch-write-optimize-find changing 1 files +5 −3

Raimo33 commented at 1:18 PM on September 12, 2025: none

Summary

This PR halves the number of implicit calls to ::find() by using an optimistic approach: insert preemptively, remove in O(1) if already spent.

Motivation

Currently, the ::find() calls that originate from CCoinsViewCache::BatchWrite() account for ~2.9% of total IBD time as shown by this flamegraph from issue #32832

Benchmarks

Benchmarks can't currently measure this change in a realistic manner, because they don't account for cache common related scenarios (see #33375). This change cannot possibly be a regression since ::try_emplace() is a call to ::find() under the hood. And ::erase(it) is constant time in the worst case.

I'm marking this PR as draft because I ultimately want to measure the impact on a real IBD run.
DrahtBot commented at 1:18 PM on September 12, 2025: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33376.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #33018 (coins: remove SetFresh method from CCoinsCacheEntry by andrewtoth)
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.
Raimo33 force-pushed on Sep 12, 2025

cache: remove redundant find() call

This commit reduces the number of implicit calls to ::find() by using an optimistic approach: insert preemptively, remove O(1) if already spent. Benchmarks can't currently measure this change in a realistic manner, because they don't account for cache common related scenarios.
This change cannot possibly be a regression since ::try_emplace() is a call to find() under the hood. And ::erase(it) is constant time in the worst case.

Currently, the ::find() calls account for ~2.9% of total IBD time as shown in issue #32832

40e75eaae8

in src/coins.cpp:194 in 9df8c9d150 outdated

 193 | +
 194 | +        auto [itUs, inserted] = cacheCoins.try_emplace(it->first);
 195 | +        if (inserted) {
 196 | +            // The parent cache did not have an entry, while the child cache does.
 197 | +            // We can ignore it if it's both spent and FRESH in the child.
 198 |              if (!(it->second.IsFresh() && it->second.coin.IsSpent())) {

purpleKarrot commented at 3:27 PM on September 12, 2025:

Can this condition be checked before the insert?

Raimo33 commented at 4:05 PM on September 12, 2025:

you mean as it was before?

Raimo33 force-pushed on Sep 12, 2025
l0rinc commented at 5:56 PM on September 12, 2025: contributor

Multiple people have pushed this already, e.g. 723c49b (#32128) or l0rinc/bitcoin@b23c6a1 (#34). I was about to push my change from my fork after I finished all IBD measurements.
bitcoin deleted a comment on Sep 12, 2025
Raimo33 closed this on Sep 12, 2025
Raimo33 commented at 8:32 PM on September 12, 2025: none

Following up in https://github.com/l0rinc/bitcoin/pull/34/commits/b23c6a1fc8f9a44561add79427d2f3fff4c3718f as reviewer/coauthor
Raimo33 deleted the branch on Sep 12, 2025

Contributors

Linked (view graph)

#32832 [IBD] Raspberry Pi: 90% CPU time for 1.5% of block processing #33375 bench: unrealistic ConnectBlock benchmarks

[WIP] cache: remove redundant find() call #33376

Summary

Motivation

Benchmarks

Code Coverage & Benchmarks

Reviews

Conflicts