[IBD] specialize block serialization#31868
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/31868. ReviewsSee the guideline for information on the review process. 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. LLM Linter (✨ experimental)Possible places where named args for integral literals may be used (e.g.
2026-01-15 |
a7db42f to
c5bbf57
Compare
|
Drafting until #31519 is merged, as recommended in #31868 (comment) |
c5bbf57 to
19d20ce
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
f98fb61 to
a9d311c
Compare
…addata16be` 0431a69 cleanup: remove unused `ser_writedata16be` and `ser_readdata16be` (Lőrinc) Pull request description: Remove dead code from serialization logic - extracted from #31868 ACKs for top commit: maflcko: lgtm ACK 0431a69 achow101: ACK 0431a69 jlest01: ACK 0431a69 Tree-SHA512: 1881a164b2a91bb6033770db625f10b845bfb17a9898efb7612ca29ba113175b29e345beb84488f388b4639ade98df98e3411e663149bcbaec6e3abeffe1cbef
a84f12e to
f4cb559
Compare
|
Are you still working on this? |
|
Of course, paused it since I'm not exactly sure how do handle single-byte entries: they measurably speed up the scenarios, but the resulting code is quite ugly. Do you have a suggestion? Should I attempt that in a separate PR and ignore it here? Edit: draft because of the requests in #31868 (comment) which I will need to remeasure |
|
What was the speedup? I can only see your last comment, which says:
|
Measure both full block serialization and size computation via `SizeComputer`. `SizeComputer` returns the exact final size of the serialized content without writing any bytes. > cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/bin/bench_bitcoin -filter='SizeComputerBlock|SerializeBlock' --min-time=10000 > C compiler ............................ AppleClang 16.0.0.16000026 | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 195,610.62 | 5,112.20 | 0.3% | 11.00 | `SerializeBlock` | 12,061.83 | 82,906.19 | 0.1% | 11.01 | `SizeComputerBlock` > C++ compiler .......................... GNU 13.3.0 | ns/block | block/s | err% | ins/block | cyc/block | IPC | bra/block | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 867,857.55 | 1,152.26 | 0.0% | 8,015,883.90 | 3,116,099.08 | 2.572 | 1,517,035.87 | 0.5% | 10.81 | `SerializeBlock` | 30,928.27 | 32,332.88 | 0.0% | 221,683.03 | 111,055.84 | 1.996 | 53,037.03 | 0.8% | 11.03 | `SizeComputerBlock`
Merged multiple template methods into single constexpr-delimited implementation to reduce template bloat (i.e. related functionality is grouped into a single method, but can be optimized because of C++20 constexpr conditions). This unifies related methods that were only bound before by similar signatures - and enables `SizeComputer` optimizations later
Endianness doesn’t affect final size, so skip it in `SizeComputer`. Fold existing overloads into one implementation, short‑circuiting logic when only the serialized size is needed. > cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/src/bench/bench_bitcoin -filter='SizeComputerBlock|SerializeBlock' --min-time=10000 > C compiler ............................ AppleClang 16.0.0.16000026 | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 191,652.29 | 5,217.78 | 0.4% | 10.96 | `SerializeBlock` | 10,323.55 | 96,865.92 | 0.2% | 11.01 | `SizeComputerBlock` > C++ compiler .......................... GNU 13.3.0 | ns/block | block/s | err% | ins/block | cyc/block | IPC | bra/block | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 614,847.32 | 1,626.42 | 0.0% | 8,015,883.64 | 2,207,628.07 | 3.631 | 1,517,035.62 | 0.5% | 10.56 | `SerializeBlock` | 26,020.31 | 38,431.52 | 0.0% | 159,390.03 | 93,438.33 | 1.706 | 42,131.03 | 0.9% | 11.00 | `SizeComputerBlock`
f4cb559 to
0068641
Compare
Single byte writes are used very often (used for every (u)int8_t or std::byte or bool and for every VarInt's first byte which is also needed for every (pre)Vector). It makes sense to avoid the generalized serialization infrastructure that isn't needed: * AutoFile write doesn't need to allocate 4k buffer for a single byte now; * `VectorWriter` and `DataStream` avoids memcpy/insert calls. > cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/bin/bench_bitcoin -filter='SizeComputerBlock|SerializeBlock' --min-time=10000 > C compiler ............................ AppleClang 16.0.0.16000026 | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 174,569.19 | 5,728.39 | 0.6% | 10.89 | `SerializeBlock` | 10,241.16 | 97,645.21 | 0.0% | 11.00 | `SizeComputerBlock` > C++ compiler .......................... GNU 13.3.0 | ns/block | block/s | err% | ins/block | cyc/block | IPC | bra/block | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 615,000.56 | 1,626.01 | 0.0% | 8,015,883.64 | 2,208,340.88 | 3.630 | 1,517,035.62 | 0.5% | 10.56 | `SerializeBlock` | 25,676.76 | 38,945.72 | 0.0% | 159,390.03 | 92,202.10 | 1.729 | 42,131.03 | 0.9% | 11.00 | `SizeComputerBlock`
0068641 to
41ef25f
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Just checking in here. This still needs rebase, and I still wonder about the speedup, and the scope: #31868 (comment) In any case, I read https://purplekarrot.net/blog/serialization.html and it introduces something like this behind a compilation boundary (to get faster compilation and the blog also claims an architectural cleanup): void serialize(CBlock const& block, writer_ref writer)
{
auto encode = encode_block(witness::allow);
auto w = BufferedWriter<writer_ref>{writer};
encode(w, block);
}I wonder if this is an alternative to this pull request and if this will also result in possible runtime speedups as the 1-byte reads/writes don't hit a read/write syscall one-by-one in a hot loop, and instead go through a buffer. I can understand that compilation speed improvements or architectural cleanup is nice, but when there is no visible speedup or other benefit for external users, the motivation is weaker. Just wondering what the greater picture here is and if there is any visible speedup. |
…rns using helper aliases (C++14/C++17) 0a3d4cd refactor: modernize remaining outdated trait patterns (Lőrinc) 1ca1b1f scripted-diff: modernize outdated trait patterns - values (Lőrinc) 66b251d scripted-diff: modernize outdated trait patterns - types (Lőrinc) Pull request description: The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11). The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly. I've modified the instances I found in the codebase one-by-one (noticed them while investigating bitcoin/bitcoin#31868), and afterwards extracted scripted diff commits to do the trivial ones automatically. The last commit contains the values that were easier done manually. I've excluded changes from `src/bench/nanobench.h`, `src/leveldb`, `src/minisketch`, `src/span.h` and `src/sync.h` - let me know if you think they should be included instead. A few of the code changes can also be reproduced by clang-tidy (but not all of them): ```bash cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc) run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,modernize-type-traits' -fix $(git grep -lE '::(value|type)' ./src ':(exclude)src/bench/nanobench.h' ':(exclude)src/leveldb' ':(exclude)src/minisketch' ':(exclude)src/span.h' ':(exclude)src/sync.h') ``` ACKs for top commit: laanwj: Concept and code review ACK 0a3d4cd Tree-SHA512: a4bcf0f267c0f4e02983b4d548ed6f58d464ec379ac5cd1f998b9ec0cf698b53a9f2557a05a342b661f1d94adefc9a0ce2dc8f764d49453aaea95451e2c4c581
…be` and `ser_readdata16be` 3b25bcb cleanup: remove unused `ser_writedata16be` and `ser_readdata16be` (Lőrinc) Pull request description: Remove dead code from serialization logic - extracted from bitcoin/bitcoin#31868 ACKs for top commit: maflcko: lgtm ACK 3b25bcb achow101: ACK 3b25bcb jlest01: ACK bitcoin/bitcoin@3b25bcb Tree-SHA512: 1881a164b2a91bb6033770db625f10b845bfb17a9898efb7612ca29ba113175b29e345beb84488f388b4639ade98df98e3411e663149bcbaec6e3abeffe1cbef
…rns using helper aliases (C++14/C++17) 0929abe refactor: modernize remaining outdated trait patterns (Lőrinc) 2237482 scripted-diff: modernize outdated trait patterns - values (Lőrinc) 2881c6e scripted-diff: modernize outdated trait patterns - types (Lőrinc) Pull request description: The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11). The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly. I've modified the instances I found in the codebase one-by-one (noticed them while investigating bitcoin/bitcoin#31868), and afterwards extracted scripted diff commits to do the trivial ones automatically. The last commit contains the values that were easier done manually. I've excluded changes from `src/bench/nanobench.h`, `src/leveldb`, `src/minisketch`, `src/span.h` and `src/sync.h` - let me know if you think they should be included instead. A few of the code changes can also be reproduced by clang-tidy (but not all of them): ```bash cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc) run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,modernize-type-traits' -fix $(git grep -lE '::(value|type)' ./src ':(exclude)src/bench/nanobench.h' ':(exclude)src/leveldb' ':(exclude)src/minisketch' ':(exclude)src/span.h' ':(exclude)src/sync.h') ``` ACKs for top commit: laanwj: Concept and code review ACK 0929abe Tree-SHA512: a4bcf0f267c0f4e02983b4d548ed6f58d464ec379ac5cd1f998b9ec0cf698b53a9f2557a05a342b661f1d94adefc9a0ce2dc8f764d49453aaea95451e2c4c581
…be` and `ser_readdata16be` 1004801 cleanup: remove unused `ser_writedata16be` and `ser_readdata16be` (Lőrinc) Pull request description: Remove dead code from serialization logic - extracted from bitcoin/bitcoin#31868 ACKs for top commit: maflcko: lgtm ACK 1004801 achow101: ACK 1004801 jlest01: ACK bitcoin/bitcoin@1004801 Tree-SHA512: 1881a164b2a91bb6033770db625f10b845bfb17a9898efb7612ca29ba113175b29e345beb84488f388b4639ade98df98e3411e663149bcbaec6e3abeffe1cbef
Thanks, valid concerns. I have a rebased and updated branch locally; once I finish the reviews and PRs I'm working on, I'll retest and adjust this one - see l0rinc#201 |
This change is part of [IBD] - Tracking PR for speeding up Initial Block Download
This PR is drafted until I remeasure everything after the recent merges and I need to find a way to simplify the 1 byte writes more nicely, I don't like all the specializations.
Summary: This PR contains a few different optimizations found by IBD profiling and via the newly added block serialization benchmarks. It also takes advantage of the recently merged
std::spanchanges enabling propagating static extents.The commits merge similar (de)serialization methods and separate them internally with
if constexpr, similarly to how it has been done here before. This enables furtherSizeComputeroptimizations as well.Context: Since single byte writes are used very often (for every
(u)int8_t,std::byte, orbool, and for everyVarInt's first byte, which is also needed for every(pre)Vector), it makes sense to avoid the generalized serialization infrastructure that isn't needed:AutoFilewrite doesn't need to allocate a 4k buffer for a single byte now;VectorWriterandDataStreamavoidmemcpy/insertcalls;CSHA256::Writecan avoidmemcpy.DeserializeBlockis dominated by hash calculations, so the optimizations barely affect it.Measurements:
C compiler ............................ AppleClang 16.0.0.16000026
C++ compiler .......................... GNU 13.3.0
While this wasn't the main motivation for the change, IBD on Ubuntu/GCC on SSD with i9 indicates a 2% speedup as well:
Details
```bash COMMITS="05314bde0b06b820225f10c6529b5afae128ff81 1cd94ec2511874ec68b92db34ad7ec7d9534fed1"; \ STOP_HEIGHT=880000; DBCACHE=10000; \ C_COMPILER=gcc; CXX_COMPILER=g++; \ hyperfine \ --export-json "/mnt/my_storage/ibd-${COMMITS// /-}-${STOP_HEIGHT}-${DBCACHE}-${C_COMPILER}.json" \ --runs 3 \ --parameter-list COMMIT ${COMMITS// /,} \ --prepare "killall bitcoind || true; rm -rf /mnt/my_storage/BitcoinData/*; git checkout {COMMIT}; git clean -fxd; git reset --hard; cmake -B build -DCMAKE_BUILD_TYPE=Release -DENABLE_WALLET=OFF -DCMAKE_C_COMPILER=$C_COMPILER -DCMAKE_CXX_COMPILER=$CXX_COMPILER && cmake --build build -j$(nproc) --target bitcoind && ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=1 -printtoconsole=0 || true" \ --cleanup "cp /mnt/my_storage/BitcoinData/debug.log /mnt/my_storage/logs/debug-{COMMIT}-$(date +%s).log || true" \ "COMPILER=$C_COMPILER COMMIT={COMMIT} ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=$STOP_HEIGHT -dbcache=$DBCACHE -prune=550 -printtoconsole=0" Benchmark 1: COMPILER=gcc COMMIT=05314bde0b06b820225f10c6529b5afae128ff81 ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -prune=550 -printtoconsole=0 Time (mean ± σ): 33647.918 s ± 508.655 s [User: 71503.409 s, System: 4404.899 s] Range (min … max): 33283.439 s … 34229.026 s 3 runsBenchmark 2: COMPILER=gcc COMMIT=1cd94ec2511874ec68b92db34ad7ec7d9534fed1 ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -prune=550 -printtoconsole=0
Time (mean ± σ): 33062.491 s ± 183.335 s [User: 71246.532 s, System: 4318.490 s]
Range (min … max): 32888.211 s … 33253.706 s 3 runs
Summary
COMPILER=gcc COMMIT=1cd94ec2511874ec68b92db34ad7ec7d9534fed1 ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -prune=550 -printtoconsole=0 ran
1.02 ± 0.02 times faster than COMPILER=gcc COMMIT=05314bde0b06b820225f10c6529b5afae128ff81 ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=10000 -prune=550 -printtoconsole=0