refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls #31490

l0rinc · 2024-12-13T13:52:01Z

UndoWriteToDisk and WriteBlockToDisk were delegating a subset of their functionality to single-use methods that didn't optimally capture a meaningful chunk of the algorithm, resulting in calculating things twice (serialized size, header size).
This change inlines the awkward methods (asserting that all previous behavior was retained), and in separate commits makes the usages less confusing.
Besides making the methods slightly more intuitive, the refactorings reduce duplicate calculations as well.

The speed difference is insignificant for now (~0.5% for the new SaveBlockToDiskBench), but are a cleanup for follow-ups such as #31539

DrahtBot · 2024-12-13T13:52:05Z

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/31490.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, hodlinator, TheCharlatan, andrewtoth
Concept ACK	BrandonOdiwuor, theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31551 (optimization: bulk reads(27%)/writes(290%) in [undo]block [de]serialization, 6% faster IBD by l0rinc)
#31539 (optimization: buffer reads(23%)/writes(290%) in [undo]block [de]serialization, 6% faster IBD by l0rinc)
#31533 (fuzz: Add fuzz target for block index tree and related validation events by mzumsande)
#31144 (optimization: batch XOR operations 12% faster IBD by l0rinc)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#29307 (util: explicitly close all AutoFiles that have been written by vasild)
#26966 (index: initial sync speedup, parallelize process by furszy)

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.

sedited · 2024-12-13T14:04:05Z

Concept ACK

andrewtoth

Concept ACK

Should we not prefer the more modern and explicit uint32_t vs unsigned int?

l0rinc · 2024-12-15T14:30:25Z

prefer the more modern and explicit uint32_t

I was hoping someone will recommend that - done: https://github.com/bitcoin/bitcoin/compare/e913e773926ecb72e327acf60c68655b5611cb7a..d69766164d177707ec7be19c4c188bd79ba3e4a3

Edit: Added block size calculation deduplication as well to the PR

src/node/blockstorage.cpp

maflcko

Not sure about the changes.

What is the goal? Is this an optimization? If yes, how can it be observed?

Also, the changes seem to be based on a misunderstanding of the log and check macros.

src/blockencodings.cpp

ryanofsky

Code review ACK 9a7e1ced7c3bb17193c7401181365c4075d45ec2. This seems like a safer, more efficient approach, but I still think the API is very confusing.

No need to address here, but for a followup I think it would make more sense for WriteBlockToDisk instead of SaveBlockToDisk call FindNextBlockPos, and for UndoWriteToDisk instead of WriteUndoDataForBlock to be call FindUndoPos, so higher level SaveBlockToDisk / WriteUndoDataForBlock functions don't contain any logic dealing with header fields.

I also find names of these functions to be inconsistent, overlong and confusing. Would rename:

SaveBlockToDisk to SaveBlock
WriteBlockToDisk to WriteBlock
WriteUndoDataForBlock to SaveBlockUndo
UndoWriteToDisk to WriteBlockUndo

src/node/blockstorage.h

maflcko

concept ack, if this improves a benchmark. In the future, it would be good to mention a speedup in the pull request description, so that reviewers can see the motivation and goal for the change.

src/util/check.h

We've been surprised multiple times by `Assume` doing heavy computations in `release`: * bitcoin#31178 (comment) * bitcoin#31490 (comment) Since `Assume` is a macro, it could have been written differently to avoid parameter evaluation (similarly to `LogDebug`, which doesn't evaluate the parameters). Co-Authored-By: Anthony Towns <[email protected]>

BrandonOdiwuor

Concept ACK

theuni · 2024-12-18T19:43:44Z

concept ack, if this improves a benchmark. In the future, it would be good to mention a speedup in the pull request description, so that reviewers can see the motivation and goal for the change.

Agree with this. If this actually shows up as significant in real workloads (IBD), concept ACK and we could potentially take this further by caching the size even earlier (as part of deserializing over the wire) to avoid the need for the calculation at all.

But if there's no noticeable speedup, I'm not a fan of muddying the api.

l0rinc · 2024-12-18T19:51:33Z

If this actually shows up as significant in real workloads

Thanks for the reviews, I'm running full IBD benchmarks currently, we'll see the results shortly (can't just do a quick reindex-chainstate since the changes are undo and block writing related).

I have two other changes in queue that will be based on this refactor (#31539 and #31144, which I've drafted until these are sorted).
The 3 changes together seem to result in >5% speedup for IBD (every kind, regardless of dbcache or prunedness) - but those benchmarks are also still running.

l0rinc · 2024-12-19T13:17:13Z

we could potentially take this further by caching the size even earlier

Absolutely, but that's a bigger change (would cache the serialized sizes in CBlock, guarding against any other mutation (which requires better encapsulation), storing GetSerializeSize for TX_NO_WITNESS() and TX_WITH_WITNESS() lazily, similarly to the existing checked* flags)... but that's a big change, affecting a lot of consensus code, I'm still working on that and will push in a separate PR.

Co-authored-by: Ryan Ofsky <[email protected]> Co-authored-by: Hodlinator <[email protected]> -BEGIN VERIFY SCRIPT- grep -r -wE 'WriteBlock|ReadRawBlock|ReadBlock|WriteBlockUndo|ReadBlockUndo' $(git ls-files src/ ':!src/leveldb') && \ echo "Error: One or more target names already exist!" && exit 1 sed -i \ -e 's/\bSaveBlockToDisk/WriteBlock/g' \ -e 's/\bReadRawBlockFromDisk/ReadRawBlock/g' \ -e 's/\bReadBlockFromDisk/ReadBlock/g' \ -e 's/\bWriteUndoDataForBlock/WriteBlockUndo/g' \ -e 's/\bUndoReadFromDisk/ReadBlockUndo/g' \ $(git ls-files src/ ':!src/leveldb') -END VERIFY SCRIPT-

ryanofsky

Code review ACK 223081e. Since last review, "Save" was renamed to "Write", uint32_t references were dropped, some log statements and comments were improved as suggested, and a lot of tweaks made to commits and commit messages which should make this easier to review.

src/node/blockstorage.cpp

ryanofsky · 2025-01-09T16:01:54Z

src/bench/readwriteblock.cpp

+    return block;
+}
+
+static void SaveBlockBench(benchmark::Bench& bench)


In commit "bench: add SaveBlockBench" (86b85bb)

Could rename SaveBlock to WriteBlock here too

Right, if I need to edit, I'll rename this as well

hodlinator

ACK 223081e

Thanks for reorganizing the first commits!
Confirmed that git -c log.showSignature=false log --oneline --follow src/bench/readwriteblock.cpp shows 7 commits.

Cool with the sanity-check in the scripted diff, not sure I've seen that before.

Commit message `dfb2f9d`

Might add some more context:
"Similarly +to UndoWriteToDisk in parent commit+, WriteBlockToDisk wasn't really extracting"

Commit message `42bc491`

(What's the inspiration for all the semicolons? Doesn't appear to be one of the uses described here: https://grammarist.com/punctuation/how-to-use-semicolons-in-a-list/)

Benchmarked with new bench target

₿ build/src/bench/bench_bitcoin -filter=SaveBlockBench -min-time=10000

At second commit (`86b85bb`)

|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|        3,172,375.74 |              315.22 |    0.6% |   20,053,788.28 |    9,071,225.51 |  2.211 |   3,133,287.73 |    0.5% |     11.12 | `SaveBlockBench`

(Median result of 3 runs).

At final commit (`223081e`)

|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|        3,159,241.92 |              316.53 |    1.4% |   19,805,232.51 |    8,963,495.75 |  2.210 |   3,080,238.64 |    0.4% |     11.08 | `SaveBlockBench`

(Median result of 3 runs).

Conclusion

Unfortunately this confirms that serializing blocks is insanely fast, making this PR more of a refactor than an optimization.

src/node/blockstorage.cpp

-            if (!FlushUndoFile(_pos.nFile, true)) {
-                LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning, "Failed to flush undo file %05i\n", _pos.nFile);
+            if (!FlushUndoFile(pos.nFile, true)) {
+                LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning, "Failed to flush undo file %05i\n", pos.


DrahtBot added the Refactoring label Dec 13, 2024

andrewtoth reviewed Dec 13, 2024

View reviewed changes

DrahtBot mentioned this pull request Dec 14, 2024

reduce cs_main scope, guard block index 'nFile' under a local mutex #27006

Closed

l0rinc force-pushed the l0rinc/undo branch from e913e77 to d697661 Compare December 15, 2024 14:29

l0rinc force-pushed the l0rinc/undo branch from d697661 to 5fd6b3f Compare December 15, 2024 20:16

l0rinc changed the title ~~refactor: Cache blockundo serialized size for consecutive calls~~ refactor: Cache block[undo] serialized size for consecutive calls Dec 15, 2024

l0rinc mentioned this pull request Dec 15, 2024

refactor: Cache block[undo] serialized size for consecutive calls bitcoin-dev-tools/benchcoin#66

Closed

ajtowns reviewed Dec 17, 2024

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

maflcko suggested changes Dec 17, 2024

View reviewed changes

src/blockencodings.cpp Outdated Show resolved Hide resolved

l0rinc force-pushed the l0rinc/undo branch from a50f9d7 to 9a7e1ce Compare December 17, 2024 10:37

l0rinc changed the title ~~refactor: Cache block[undo] serialized size for consecutive calls~~ optimization: Cache block[undo] serialized size for consecutive calls Dec 17, 2024

ryanofsky approved these changes Dec 17, 2024

View reviewed changes

src/node/blockstorage.h Outdated Show resolved Hide resolved

DrahtBot requested review from andrewtoth and sedited December 17, 2024 17:32

l0rinc force-pushed the l0rinc/undo branch from 9a7e1ce to a730402 Compare December 18, 2024 12:15

maflcko reviewed Dec 18, 2024

View reviewed changes

src/util/check.h Outdated Show resolved Hide resolved

l0rinc force-pushed the l0rinc/undo branch from a730402 to 3ff1b2c Compare December 18, 2024 13:34

l0rinc mentioned this pull request Dec 18, 2024

doc: emphasize that Assume always evaluates #31528

Closed

BrandonOdiwuor reviewed Dec 18, 2024

View reviewed changes

l0rinc mentioned this pull request Dec 19, 2024

optimization: buffer reads(23%)/writes(290%) in [undo]block [de]serialization, 6% faster IBD #31539

Closed

l0rinc changed the title ~~optimization: Cache block[undo] serialized size for consecutive calls~~ optimization: cache block[undo] serialized size for consecutive calls Dec 19, 2024

DrahtBot mentioned this pull request Dec 19, 2024

util: explicitly close all AutoFiles that have been written #29307

Merged

l0rinc force-pushed the l0rinc/undo branch from 3ff1b2c to 92abdde Compare December 19, 2024 22:13

l0rinc force-pushed the l0rinc/undo branch from e25f029 to 223081e Compare January 9, 2025 14:17

ryanofsky approved these changes Jan 9, 2025

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

ryanofsky reviewed Jan 9, 2025

View reviewed changes

l0rinc requested a review from maflcko January 9, 2025 16:09

hodlinator approved these changes Jan 10, 2025

View reviewed changes

refactor: inline UndoWriteToDisk and WriteBlockToDisk to reduce serialization calls #31490

refactor: inline UndoWriteToDisk and WriteBlockToDisk to reduce serialization calls #31490

Uh oh!

Conversation

l0rinc commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

sedited commented Dec 13, 2024

Uh oh!

andrewtoth left a comment

Choose a reason for hiding this comment

Uh oh!

l0rinc commented Dec 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

theuni commented Dec 18, 2024

Uh oh!

l0rinc commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l0rinc commented Dec 19, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

l0rinc Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Commit message dfb2f9d

Commit message 42bc491

Benchmarked with new bench target

At second commit (86b85bb)

At final commit (223081e)

Conclusion

Uh oh!

refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls #31490

refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls #31490

l0rinc commented Dec 13, 2024 •

edited

Loading

DrahtBot commented Dec 13, 2024 •

edited

Loading

l0rinc commented Dec 15, 2024 •

edited

Loading

l0rinc commented Dec 18, 2024 •

edited

Loading

Commit message `dfb2f9d`

Commit message `42bc491`

At second commit (`86b85bb`)

At final commit (`223081e`)