[IBD] batch block reads/writes during AutoFile serialization #31551

This change is part of [IBD] - Tracking PR for speeding up Initial Block Download

Summary

We can serialize the blocks and undos to any Stream which implements the appropriate read/write methods. AutoFile is one of these, writing the results “directly” to disk (through the OS file cache). Batching these in memory first and reading/writing these to disk is measurably faster (likely because of fewer native fread calls or less locking, as observed by Martinus in a similar change).

Unlocking new optimization opportunities

Buffered writes will also enable batched obfuscation calculations (implemented in #31144) - especially since currently we need to copy the write input’s std::span to do the obfuscation on it, and batching enables doing the operations on the internal buffer directly.

Measurements (micro benchmarks, full IBDs and reindexes)

Microbenchmarks for [Read|Write]BlockBench show a ~30%/168% speedup with macOS/Clang, and ~19%/24% with Linux/GCC (the follow-up XOR batching improves these further):

Before:

After:

Before:

After:

2 full IBD runs against master (compiled with GCC where the gains seem more modest) for 888888 blocks (seeded from real nodes) indicates a ~7% total speedup.

 0COMMITS="d2b72b13699cf460ffbcb1028bcf5f3b07d3b73a 652b4e3de5c5e09fb812abe265f4a8946fa96b54"; \
 1STOP_HEIGHT=888888; DBCACHE=1000; \
 2C_COMPILER=gcc; CXX_COMPILER=g++; \
 3BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
 4(for c in $COMMITS; do git fetch origin $c -q && git log -1 --pretty=format:'%h %s' $c || exit 1; done) && \
 5hyperfine \
 6  --sort 'command' \
 7  --runs 2 \
 8  --export-json "$BASE_DIR/ibd-${COMMITS// /-}-$STOP_HEIGHT-$DBCACHE-$C_COMPILER.json" \
 9  --parameter-list COMMIT ${COMMITS// /,} \
10  --prepare "killall bitcoind; rm -rf $DATA_DIR/*; git checkout {COMMIT}; git clean -fxd; git reset --hard; \
11    cmake -B build -DCMAKE_BUILD_TYPE=Release -DENABLE_WALLET=OFF -DCMAKE_C_COMPILER=$C_COMPILER -DCMAKE_CXX_COMPILER=$CXX_COMPILER && \
12    cmake --build build -j$(nproc) --target bitcoind && \
13    ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=1 -printtoconsole=0; sleep 100" \
14  --cleanup "cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
15  "COMPILER=$C_COMPILER COMMIT=${COMMIT:0:10} ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP_HEIGHT -dbcache=$DBCACHE -blocksonly -printtoconsole=0"
16d2b72b1369 refactor: rename leftover WriteBlockBench
17652b4e3de5 optimization: Bulk serialization writes in `WriteBlockUndo` and `WriteBlock`
18Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=888888 -dbcache=1000 -blocksonly -printtoconsole=0 (COMMIT = d2b72b13699cf460ffbcb1028bcf5f3b07d3b73a)
19  Time (mean ± σ):     41528.104 s ± 354.003 s    [User: 44324.407 s, System: 3074.829 s]
20  Range (min … max):   41277.786 s … 41778.421 s    2 runs
21 
22Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=888888 -dbcache=1000 -blocksonly -printtoconsole=0 (COMMIT = 652b4e3de5c5e09fb812abe265f4a8946fa96b54)
23  Time (mean ± σ):     38771.457 s ± 441.941 s    [User: 41930.651 s, System: 3222.664 s]
24  Range (min … max):   38458.957 s … 39083.957 s    2 runs
25 
26Relative speed comparison
27        1.07 ±  0.02  COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=888888 -dbcache=1000 -blocksonly -printtoconsole=0 (COMMIT = d2b72b13699cf460ffbcb1028bcf5f3b07d3b73a)
28        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=888888 -dbcache=1000 -blocksonly -printtoconsole=0 (COMMIT = 652b4e3de5c5e09fb812abe265f4a8946fa96b54)

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

Reviews

See 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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #32043 ([IBD] Tracking PR for speeding up Initial Block Download by l0rinc)
• #31144 ([IBD] multi-byte block obfuscation by l0rinc)
• #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
• #29307 (util: explicitly close all AutoFiles that have been written by vasild)

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.

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36027778098

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

2 full IBD runs against master for 870k blocks (seeded from real nodes) indicates a 6% total speedup. Note that this is only measurable with full IBD, not with reindex(-chainstate) since the gain comes from block reading/writing.

I don’t understand this. -reindex(-chainstate) requires the read of blocks, so why would it not be measurable? See also #28226 which this pull is inspired by(?) and which claims a speedup in reindex-chainstate.

which claims a speedup in reindex-chainstate

So the 9% improvement in unserializing seems to translate to roughly 1.2% total runtime improvement in -reindex-chainstate on my machine.

I can measure several runs to see if it makes any difference (to be able to claim anything reliable about a ~1% potential speedup), but the reads are only 27% faster, while the writes are 2.9x faster, so I’m not surprised that reindex doesn’t shine compared to IBD. But I’m currently rerunning them with the rebased PR to make sure my measurements were realistic. If you have any concrete measurement you’d like me to do, feel free to specify them.

which this pull is inspired by(?)

I did bump into it, but basically reinvented it (not the first time) while investigating why XOR is slow. I’ll add @martinus as a coauthor as well, thanks for the hint @maflcko, he deserves it.

-reindex(-chainstate) requires the read of blocks, so why would it not be measurable?

Thanks for the push, I have redone all the measurements: reindex-chainstate is very stable (see the updated PR description for details), two runs indicate a 4% speedup (with GCC, will try with Clang as well since it indicated a bigger speedup in the microbenchmarks):

0Summary
1  COMMIT=97b4a50c714c801e08bc02648a9c259f284069c2 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=30000 -printtoconsole=0 -reindex-chainstate -connect=0 ran
2    1.04 ± 0.00 times faster than COMMIT=5acf12bafeb126f2190b3f401f95199e0eea90c9 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=30000 -printtoconsole=0 -reindex-chainstate -connect=0

While IBD is closer to the user’s experience (having actual block- and undo writes, which is important since they were modified here), it’s also less stable, so I ran 3 rounds of each, indicating a 6% speedup (bringing reindex and IBD speeds closer together):

0Summary
1  COMMIT=0e9fb2ed330960c0e4cd36b077c64ac7d0f84240 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=30000 -printtoconsole=0 ran
2    1.06 ± 0.02 times faster than COMMIT=5acf12bafeb126f2190b3f401f95199e0eea90c9 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=30000 -printtoconsole=0

Code review d700b14491a27a6f4f9069dde86fe914784346ca

This approach seems reaonable but maybe it would be good to have a more general solution to providing buffered reads and writes instead of having these ad-hoc std::vector<uint8_t> / SpanReader / DataStream / write_large calls.

Ideally instead of:

0std::vector<uint8_t> mem(blk_size);
1filein >> Span{mem};
2SpanReader(mem) >> TX_WITH_WITNESS(block);
3// ...
4fileout.write_large(DataStream(block_size) << TX_WITH_WITNESS(block));

Code could look more like:

0BufferedReader{fileIn, blk_size} >> TX_WITH_WITNESS(block);
1// ...
2BufferedWriter{fileout, block_size} << TX_WITH_WITNESS(block);

But I guess as long as the buffers are only going to be used once and thrown away in this case (and not be fixed-size ring buffers) having buffer classes wouldn’t substantively improve this code much aside from making it look nicer. Buffer stream classes would seem more useful in cases where more fields are serialized and sizes are not known in advance.

Just wanted to suggest this in case there are other places where serialization is slow and buffering could speed it up. We might want a more general solution if that is the case.

Thanks for the review @ryanofsky

would be good to have a more general solution to providing buffered reads and writes

That’s what I had originally (with a fixed-size buffer, so was even more general than your suggestion), making the call-site-diff trivial: #31539. It was suggested by @theuni that it may be simpler to serialize to buffer directly. It does seem simpler to reason about this one, especially since they basically had the same speed. What do you think?

What other places are candidates for similar buffering?

No strong opinion. I guess I’d be a little disappointed if you initially had a general solution that made it easy to enable buffering any place and then were told that instead of using that, you should add kludges to this code and implement ad-hoc buffering here. I don’t think buffering is difficult to reason about and don’t think buffering is the type of thing that needs to be deployed on a case-by-case basis, especially since performance of buffering should mostly depend on where data is being written, not what data is being written. So if there is evidence that XORed file i/o is faster with buffering one place, probably we should expect XORed file i/o with buffering to be faster other places as well.

I have no problems with current version of this PR though, and I can understand reasons why there might be pushback against implementation in #31539 since since the BufferedReadOnlyFile class implemented there duplicated functionality of AutoFile, didn’t support writing, and didn’t work with any type of stream. So it could be faulted not just for introducing a new abstraction, but for the abstraction not being usable in more places and not being compatible with current stream classes.

Would maybe suggest adding a little “Design considerations” or “Alternatives considered” section to end of PR description to link to #31539 and say how current design was chosen, because it would have been useful to see and know about the alternative when reviewing this PR.

since the BufferedReadOnlyFile class implemented there duplicated functionality of AutoFile, didn’t support writing, and didn’t work with any type of stream

There was a writer as well, but it resulted in some weird CI failures and given this alternative I just closed the PR.

disappointed if you initially had a general solution that made it easy to enable buffering any place and then were told that instead of using that

No, the suggested approach was better, that’s why I kept this one. I’d be curious what other reviewers think, but I don’t mind re-generalizing it (now or in a follow-up, when we find other usages).

XORed file i/o is faster with buffering one place, probably we should expect XORed file i/o with buffering to be faster other places as well

It’s not just the xor though, many small writes seemed to be slower in general, it seems faster if we buffer instead of the OS (likely because of locality, the file is a global resource that needs locking).

don’t think buffering is the type of thing that needs to be deployed on a case-by-case basis

I can get behind that, can you point me to other usages where we could use this?

I can get behind that, can you point me to other usages where we could use this?

I don’t know, but if I were looking I’d just grep for places where AutoFile class is used. I don’t have any real concerns here and I’m fine with the current approach. To the extent I do have concerns they are about readability and maintainability of code not performance, so I like an approach that makes it trivial to turn buffering on and off, which is I why thought BufferedReader / BufferedWriter stream adapters suggested in #31551 (comment) might be a good idea. But when I posted that I didn’t know a similar idea had already been tried before, so wouldn’t suggest changing the approach again at this point without feedback from the other reviewers.

Thanks again for the hint @ryanofsky, I went through all changes again and reworked the batching - it’s so much better this way and it includes your idea, @theuni’s and @maflcko’s.

Additionally I fixed a few things after rebase:

• There was a leftover SaveBlock from the previous refactor;
• Commits and docs still contained build/src/benc/bench_bitcoin - changed them to build/bin/bench_bitcoin;
• BLOCK_SERIALIZATION_HEADER_SIZE is used inline in the examples now and is too long - simplified it in a separate commit;
• Added BufferedFileW and BufferedFileR, encapsulating the buffering (and the new AutoFile::write_large) so that we can have a similar usage to what you suggested;
• I have also extracted the low-risk refactorings (which are needed for the batching later) to a separate commit to minimize the diffs in the risky commits.

The performance is same as before, the commits should be a lot simpler to review.

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/39110927855

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

• Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

• A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

• An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Code review 2c073b90ae977af1ac01164a894edd978529fdc6. I think the new BufferedFile approach implemented is reasonable and a little nicer than the previous approach which made lower-level calls in block storage code to allocate and pass buffers. But the need to hardcode and compute sizes in 8782d6ed09bbb947426c1ed54ad4d5188326764d and 2c073b90ae977af1ac01164a894edd978529fdc6 still seems to make the storage code more awkward and fragile, and require earlier refactoring changes that make the PR more complicated overall.

If it would be possible, and if it would ok with you and other reviewers, I would prefer if it we could make minimal changes to the blockstorage code and instead add BufferedReader/BufferedWriter streams that only add buffering without making other changes. A diff of what this could look like is:

  0--- a/src/node/blockstorage.cpp
  1+++ b/src/node/blockstorage.cpp
  2@@ -660,11 +660,12 @@ bool BlockManager::ReadBlockUndo(CBlockUndo& blockundo, const CBlockIndex& index
  3     const FlatFilePos pos{WITH_LOCK(::cs_main, return index.GetUndoPos())};
  4 
  5     // Open history file to read
  6-    AutoFile filein{OpenUndoFile(pos, true)};
  7-    if (filein.IsNull()) {
  8+    AutoFile file{OpenUndoFile(pos, true)};
  9+    if (file.IsNull()) {
 10         LogError("OpenUndoFile failed for %s", pos.ToString());
 11         return false;
 12     }
 13+    BufferedReader filein{file};
 14 
 15     // Read block
 16     uint256 hashChecksum;
 17@@ -937,11 +938,12 @@ bool BlockManager::WriteBlockUndo(const CBlockUndo& blockundo, BlockValidationSt
 18             return false;
 19         }
 20         // Open history file to append
 21-        AutoFile fileout{OpenUndoFile(pos)};
 22-        if (fileout.IsNull()) {
 23+        AutoFile file{OpenUndoFile(pos)};
 24+        if (file.IsNull()) {
 25             LogError("OpenUndoFile failed");
 26             return FatalError(m_opts.notifications, state, _("Failed to write undo data."));
 27         }
 28+        BufferedWriter fileout{file};
 29 
 30         // Write index header
 31         fileout << GetParams().MessageStart() << blockundo_size;
 32@@ -986,11 +988,12 @@ bool BlockManager::ReadBlock(CBlock& block, const FlatFilePos& pos) const
 33     block.SetNull();
 34 
 35     // Open history file to read
 36-    AutoFile filein{OpenBlockFile(pos, true)};
 37-    if (filein.IsNull()) {
 38+    AutoFile file{OpenBlockFile(pos, true)};
 39+    if (file.IsNull()) {
 40         LogError("%s: OpenBlockFile failed for %s\n", __func__, pos.ToString());
 41         return false;
 42     }
 43+    BufferedReader filein{file};
 44 
 45     // Read block
 46     try {
 47@@ -1039,11 +1042,12 @@ bool BlockManager::ReadRawBlock(std::vector<uint8_t>& block, const FlatFilePos&
 48         return false;
 49     }
 50     hpos.nPos -= 8; // Seek back 8 bytes for meta header
 51-    AutoFile filein{OpenBlockFile(hpos, true)};
 52-    if (filein.IsNull()) {
 53+    AutoFile file{OpenBlockFile(hpos, true)};
 54+    if (file.IsNull()) {
 55         LogError("%s: OpenBlockFile failed for %s\n", __func__, pos.ToString());
 56         return false;
 57     }
 58+    BufferedReader filein{file};
 59 
 60     try {
 61         MessageStartChars blk_start;
 62@@ -1082,12 +1086,13 @@ FlatFilePos BlockManager::WriteBlock(const CBlock& block, int nHeight)
 63         LogError("FindNextBlockPos failed");
 64         return FlatFilePos();
 65     }
 66-    AutoFile fileout{OpenBlockFile(pos)};
 67-    if (fileout.IsNull()) {
 68+    AutoFile file{OpenBlockFile(pos)};
 69+    if (file.IsNull()) {
 70         LogError("OpenBlockFile failed");
 71         m_opts.notifications.fatalError(_("Failed to write block."));
 72         return FlatFilePos();
 73     }
 74+    BufferedWriter fileout{file};
 75 
 76     // Write index header
 77     fileout << GetParams().MessageStart() << block_size;
 78--- a/src/streams.cpp
 79+++ b/src/streams.cpp
 80@@ -64,6 +64,15 @@ void AutoFile::read(std::span<std::byte> dst)
 81     }
 82 }
 83 
 84+void AutoFile::read_buffer(std::span<std::byte>& dst)
 85+{
 86+    size_t size{detail_fread(dst)};
 87+    if (size != dst.size() && !feof()) {
 88+        throw std::ios_base::failure("AutoFile::read_buffer: fread failed");
 89+    }
 90+    dst = dst.first(size);
 91+}
 92+
 93 void AutoFile::ignore(size_t nSize)
 94 {
 95     if (!m_file) throw std::ios_base::failure("AutoFile::ignore: file handle is nullptr");
 96@@ -102,6 +111,16 @@ void AutoFile::write(std::span<const std::byte> src)
 97     }
 98 }
 99 
100+void AutoFile::write_buffer(std::span<std::byte> src)
101+{
102+    if (!m_file) throw std::ios_base::failure("AutoFile::write_buffer: file handle is nullptr");
103+    util::Xor(src, m_xor, *m_position); // obfuscate in-place
104+    if (std::fwrite(src.data(), 1, src.size(), m_file) != src.size()) {
105+        throw std::ios_base::failure("AutoFile::write_buffer: write failed");
106+    }
107+    if (m_position) *m_position += src.size();
108+}
109+
110 bool AutoFile::Commit()
111 {
112     return ::FileCommit(m_file);
113--- a/src/streams.h
114+++ b/src/streams.h
115@@ -465,6 +465,11 @@ public:
116         ::Unserialize(*this, obj);
117         return *this;
118     }
119+
120+    //! Read into a mutable buffer more flexibly than read(), shortening the buffer if reached EOF.
121+    void read_buffer(std::span<std::byte>& dst);
122+    //! Write a mutable buffer more efficiently than write(), XORing the buffer in-place.
123+    void write_buffer(std::span<std::byte> src);
124 };
125 
126 /** Wrapper around an AutoFile& that implements a ring buffer to
127@@ -614,4 +619,72 @@ public:
128     }
129 };
130 
131+template<typename S>
132+class BufferedReader
133+{
134+    S& m_stream;
135+    std::vector<std::byte> m_buffer;
136+    size_t m_pos; //!< Next m_buffer position to read.
137+public:
138+    BufferedReader(S& stream, size_t size=65536) : m_stream{stream}, m_buffer{size}, m_pos{size} {}
139+    void read(std::span<std::byte> dst)
140+    {
141+        while (!dst.empty()) {
142+            if (m_pos == m_buffer.size()) {
143+                std::span<std::byte> data{m_buffer};
144+                m_stream.read_buffer(data);
145+                m_buffer.resize(data.size()); // shrink on eof
146+                m_pos = 0;
147+            }
148+            if (m_buffer.size() == 0) throw std::ios_base::failure("BufferedReader::read: end of file");
149+            size_t n = std::min(dst.size(), m_buffer.size() - m_pos);
150+            std::copy(m_buffer.begin() + m_pos, m_buffer.begin() + m_pos + n, dst.begin());
151+            m_pos += n;
152+            dst = dst.subspan(n);
153+        }
154+    }
155+
156+    template <typename T>
157+    BufferedReader& operator>>(T&& obj)
158+    {
159+        ::Unserialize(*this, obj);
160+        return *this;
161+    }
162+};
163+
164+template<typename S>
165+class BufferedWriter
166+{
167+    S& m_stream;
168+    std::vector<std::byte> m_buffer;
169+    size_t m_pos; //!< Next m_buffer position to write.
170+public:
171+    BufferedWriter(S& stream, size_t size=65536) : m_stream{stream}, m_buffer{size}, m_pos{0} {}
172+    ~BufferedWriter() { flush(); }
173+
174+    void write(std::span<const std::byte> src)
175+    {
176+        while (!src.empty()) {
177+            size_t n = std::min(src.size(), m_buffer.size() - m_pos);
178+            std::copy(src.begin(), src.begin() + n, m_buffer.begin() + m_pos);
179+            m_pos += n;
180+            if (m_pos == m_buffer.size()) flush();
181+            src = src.subspan(n);
182+        }
183+    }
184+
185+    void flush()
186+    {
187+         m_stream.write_buffer(std::span{m_buffer}.first(m_pos));
188+         m_pos = 0;
189+    }
190+
191+    template <typename T>
192+    BufferedWriter& operator<<(const T& obj)
193+    {
194+        ::Serialize(*this, obj);
195+        return *this;
196+    }
197+};
198+
199 #endif // BITCOIN_STREAMS_H

This is untested but compiles. If it’s workable I think it would be a better direction to go in, both to keep blockstorage code simpler, but also to provide better support for buffered reads and writes in the stream API in case there are other parts of code that could benefit from them.

I am also ok with with current approach and happy to review if there are issues with this or it’s just not the direction we want to go.

@ryanofsky, thanks again for the review. Your suggestion seems to point back to the initial implementation of this PR, i.e. https://github.com/bitcoin/bitcoin/pull/31539/commits

Absolutely yes. There are a few things I would change about that PR but overall it seems a lot cleaner and simpler to me than this PR. If it doesn’t perform as well as this one that is surprising, because I thought the performance improvement here came from buffering, not from choosing buffers with magic sizes. But if you and other reviewers agree the current approach is better than the previous approach implemented in #31539, I guess this is the way to go, and I don’t mind reviewing it as-is.

@ryanofsky, I’ve experimented with your version, turns out I’m mostly fine with both solutions. https://github.com/bitcoin/bitcoin/compare/2c073b90ae977af1ac01164a894edd978529fdc6..06f37689a3b79f2f65ac95d8ae54a407ed2d4502

I have based this version on the patch you’ve posted - with minor differences.

• Removed all introduced undo_size seek backs - except in ReadRawBlock, which already steps back to get access to the size. I have cleaned it up in a separate commit - we don’t need buffering there since we’re not serializing anything there, just reading the whole block directly already.
• read_buffer/write_buffer introduces non-trivial Duck Typing (i.e. now users have to know if the want to call void write(std::span<const std::byte> src); or void write_buffer(std::span<std::byte> src);), I don’t particularly like it, but we can live with it.
• std::copy can be simplified to std::copy_n
• the new AutoFile::write_buffer can now be used in AutoFile::write to reduce duplication
• Changed read_buffer to return the size instead of mutating the parameter, this enabled us to avoid resizing the buffer for eof in BufferedReader (the benchmarks caught the slowdown)
• Used DataStream as buffers
• Added tests for the buffered writers - for the read/write methods and separately for the serialization part

The ReadBlockBench/WriteBlockBench speedups changed slightly from 24%/31% to 19%/24% on Linux/GCC, and from 32%/298% on Mac to 30%/ 194% To see how real performance is affected, I’m rerunning a few full IBDs to measure the performance change after the buffering changes (reindex won’t suffice here).

Does CI pass locally before and after? Measuring locally, what is the memory usage of the CI before and after?

It seems odd that using exact block sizes (this pull initially) passes, whereas using a 1<<20 approximation fails due to OOM.

It seems odd that using exact block sizes (this pull initially) passes, whereas using a 1«20 approximation fails due to OOM

Yes, I thought of the same, but the other PR had the same issue. I’m not familiar enough with the intricacies of ASan + LSan + UBSan + integer, no depends, USDT to have a good enough explanation - besides the instrumentation overhead causing it to overflow.

What is the memory usage of the CI before and after?

I have measured a valgrind/massif IBD until the be benchmarked block #413567, indicating that there isn’t a significant memory usage change - not sure about the CI usage, that seems messy to measure.

 0COMMITS="06f37689a3b79f2f65ac95d8ae54a407ed2d4502 8fe73a102f5b8406f166d7ac4b1a36bba84de7c4"; \
 1STOP_HEIGHT=413567; DBCACHE=1000; \
 2C_COMPILER=gcc; CXX_COMPILER=g++; \
 3BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
 4for COMMIT in $COMMITS; do \
 5  git fetch --all -q && git fetch origin $COMMIT && git log -1 --oneline $COMMIT && \
 6  killall bitcoind; rm -rf $DATA_DIR/* && git checkout $COMMIT && git clean -fxd && git reset --hard && \
 7  cmake -B build -DCMAKE_BUILD_TYPE=Release -DENABLE_WALLET=OFF -DCMAKE_C_COMPILER=$C_COMPILER -DCMAKE_CXX_COMPILER=$CXX_COMPILER && \
 8  cmake --build build -j$(nproc) --target bitcoind && \
 9  ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=1 -printtoconsole=0 && \
10  valgrind --tool=massif --time-unit=ms \
11    --massif-out-file="$LOG_DIR/massif-$COMMIT-$STOP_HEIGHT-$DBCACHE.out" \
12    ./build/bin/bitcoind -datadir=$DATA_DIR \
13    -stopatheight=$STOP_HEIGHT \
14    -dbcache=$DBCACHE \
15    -blocksonly=1 \
16    -printtoconsole=0 && \
17  cp $DATA_DIR/debug.log "$LOG_DIR/debug-$COMMIT-$STOP_HEIGHT-$DBCACHE.log" && \
18  ms_print "$LOG_DIR/massif-$COMMIT-$STOP_HEIGHT-$DBCACHE.out" | tee "$LOG_DIR/massif-$COMMIT-$STOP_HEIGHT-$DBCACHE.txt"; \
19done

06f37689a3 - size_t size = 1 « 20

 0    GB
 11.322^              #
 2     |              #     :     :               @      :    :               :
 3     |             @#     :     :    :          @:     :    :    :     :    :
 4     |            :@#    ::    ::    :    ::    @:     :   ::    :    ::   ::
 5     |            :@#   @::    ::   ::    :     @:    ::   ::    :    ::   @:
 6     |            :@#   @::    ::   ::    :     @:    ::   ::   ::    ::   @:
 7     |            :@#   @::    ::   ::   ::     @:   :::   ::   ::   :::   @:
 8     |            :@#   @::  ::::  :::   ::     @:  @:::  :::  :::   :::  :@:
 9     |          :@:@#  :@::  : ::  :::  :::     @:  @::: ::::  :::   :::  :@:
10     |         ::@:@#  :@::  : ::  :::  :::     @:  @::: ::::  :::   ::: ::@:
11     |        @::@:@#  :@::  : :: @:::  :::    :@:  @::: ::::  :::  @::: ::@:
12     |        @::@:@# ::@:: :: :: @:::  :::    :@:  @::: :::: :::: :@::: ::@::
13     |       @@::@:@# ::@:: :: :: @:::  :::    :@: :@::::::::@:::: :@::: ::@::
14     |     ::@@::@:@# ::@:: :: :::@::: :::: @  :@: :@::::::::@:::: :@::::::@::
15     |    :: @@::@:@#:::@::::: :::@::: :::: @  :@: :@::::::::@:::: :@::::::@::
16     |    :: @@::@:@#:::@::::: :::@:::::::: @:::@:::@::::::::@::::::@::::::@::
17     |   ::: @@::@:@#:::@::::: :::@:::::::: @: :@:::@::::::::@::::::@::::::@::
18     | ::::: @@::@:@#:::@::::: :::@:::::::: @: :@:::@::::::::@::::::@::::::@::
19     | ::::: @@::@:@#:::@::::: :::@:::::::: @: :@:::@::::::::@::::::@::::::@::
20     | ::::: @@::@:@#:::@::::: :::@:::::::: @: :@:::@::::::::@::::::@::::::@::
21   0 +----------------------------------------------------------------------->h
22     0                                                                   4.449

vs

8fe73a102f - size_t size = 1 « 16

 0    GB
 11.336^                 #
 2     |           @     #                   :   :           :     :
 3     |           @     #    :     :        :   :     :     :     :    :    :
 4     |          @@     #    :     :     :  :   :     :    ::    @:    :    :
 5     |          @@    :#    :    ::     :  :  ::     :   :::   :@:   ::   ::
 6     |          @@    :#    :    ::     :  :  ::    @:   :::   :@:   ::   ::
 7     |          @@    :#   @:    ::     :  :  ::    @:   :::  ::@:  :::   ::
 8     |        @@@@   ::#   @:   :::   :::  : :::  ::@:  @:::  ::@:  :::   ::
 9     |        @ @@   ::#  :@:   :::   : :  : :::  ::@:  @:::  ::@:  :::  :::
10     |      @@@ @@   ::#  :@:  ::::   : : :: :::  ::@:  @::: :::@: ::::  :::
11     |      @@@ @@ ::::#  :@:  ::::   : : :: :::  ::@: :@::: :::@: :::: :::: :
12     |      @@@ @@ : ::# ::@:  ::::  @: : ::@:::  ::@: :@:::::::@:::::: :::: :
13     |    ::@@@ @@ : ::# ::@:  ::::  @: : ::@::: :::@:::@:::::::@::::::@:::: :
14     |   :: @@@ @@:: ::#:::@:  ::::  @: ::::@::: :::@:::@:::::::@::::::@:::: :
15     |   :: @@@ @@:: ::#:::@:  ::::  @: ::::@:::@:::@:::@:::::::@::::::@::::::
16     |  ::: @@@ @@:: ::#:::@:@@::::::@: ::::@:::@:::@:::@:::::::@::::::@::::::
17     | :::: @@@ @@:: ::#:::@:@ ::::: @: ::::@:::@:::@:::@:::::::@::::::@::::::
18     | :::: @@@ @@:: ::#:::@:@ ::::: @: ::::@:::@:::@:::@:::::::@::::::@::::::
19     | :::: @@@ @@:: ::#:::@:@ ::::: @: ::::@:::@:::@:::@:::::::@::::::@::::::
20     | :::: @@@ @@:: ::#:::@:@ ::::: @: ::::@:::@:::@:::@:::::::@::::::@::::::
21   0 +----------------------------------------------------------------------->h
22     0                                                                   4.111

Does CI pass locally before and after?

It’s passing here as well now with 1 << 16 instead of 1 << 20.

Code review ACK 8fe73a102f5b8406f166d7ac4b1a36bba84de7c4, since the code seems right and I didn’t see any issues, but IIUC something unexpected happens if (1«20) buffer sizes are used as described #31551 (comment) so that sounds like something that needs to be figured out.

Would be curious to know if other reviewers like the current approach, since a number of approaches have been tried, and this moves closer to some earlier approaches that maybe other reviewers didn’t like. I like the current approach because I was pushing for it, but I only suggested it because I didn’t know something like it had been tried before, so maybe there are drawbacks I don’t see here or this is just not appealing. I’d be happy with whatever approach can get accepted if not this one.

re: #31551 (comment)

• read_buffer/write_buffer introduces non-trivial Duck Typing (i.e. now users have to know if the want to call void write(std::span<const std::byte> src); or void write_buffer(std::span<std::byte> src);), I don’t particularly like it, but we can live with it.

Would be curious what this means. The two write methods have different behavior because one of them mutates the buffer and the other one doesn’t. It would seem both write methods have different use-cases, and same for the read methods.

but IIUC something unexpected happens if (1«20) buffer sizes are used

Seems like a simple OOM caused by instrumentation overhead - @maflcko, can you help us out here, does this need more investigation?

read_buffer/write_buffer introduces non-trivial Duck Typing

would be curious what this means. The two write methods have different behavior because one of them mutates the buffer and the other one doesn’t. It would seem both write methods have different use-cases, and same for the read methods.

Yes, I mean we don’t have segregated interfaces which predestine the usages that are allowed (e.g. how a bufferedfile could have a buffered-reader and a buffered-writer interface to only allow a subset of the functionality). Here AutoFile doesn’t implement a Bufferable interface, and the BufferedReader doesn’t accept a explicit BufferedReader(Bufferable& stream, so there’s an implicit contract (incidental interface), that you can only use BufferedReader for a stream if they happen to implement read_buffer, because it’s used inside (i.e. compile-time duck typing). So if somebody wants to make any other stream BufferedReader compatible, they will have to look at the actual implementation details since there isn’t an interface guiding them. It’s fine, I don’t mind, just mentioned that it’s a slight complication in my opinion.

Rebased the PR in a separate push to hopefully resolve the unrelated Windows CI failure.

but IIUC something unexpected happens if (1«20) buffer sizes are used

Seems like a simple OOM caused by instrumentation overhead - @maflcko, can you help us out here, does this need more investigation?

It is not a blocker, it just makes me curious that static buffer sizes cause it and dynamic ones don’t. If you want to look more into this, you could try IBD (or AU-IBD) again with asan enabled?

Code review ACK 71610707a8b69eeef0c5345f78aac40c38fc3b88. Mostly small changes since last review, biggest one was changing the read method to be simpler and avoid looping.

re: #31551 (comment)

Yes, I mean we don’t have segregated interfaces which predestine the usages that are allowed

Understood now thanks. IMO, using duck typing and not defining formal interfaces here is a more of a benefit than a drawback, since it can let this code evolve easily without requiring wider refactoring, and let it be generic without implementing functionality that’s not actually used. But this is a tradeoff.

• BufferedReader takes ownership of the underlying stream now;
• read_buffer was removed;
• In the tests made sure file_size and max_read_length are never empty and that test state doesn’t propagate between comparisons;
• Fixed writeer typo;
• Added back BufferedWriter size = 1 << 20 - I have a hunch of why it failed, let’s see if it passes now; I have to figure this out before it’s merged

https://github.com/bitcoin/bitcoin/compare/71610707a8b69eeef0c5345f78aac40c38fc3b88..85b0c6d309cea981fda75e8cf268a144ae472d8d

https://github.com/bitcoin/bitcoin/compare/aef99ea38d85d7e56e64bbd0809a7298f21e7b8a..652b4e3de5c5e09fb812abe265f4a8946fa96b54

Rebased again in separate commit since QT5 wasn’t found after the QT6 merge.

Redid the IBDs (on an HDD this time) - also showing a ~7% average speedup

 0COMMITS="d2b72b13699cf460ffbcb1028bcf5f3b07d3b73a 652b4e3de5c5e09fb812abe265f4a8946fa96b54"; \
 1STOP_HEIGHT=888888; DBCACHE=1000; \
 2C_COMPILER=gcc; CXX_COMPILER=g++; \
 3BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
 4(for c in $COMMITS; do git fetch origin $c -q && git log -1 --pretty=format:'%h %s' $c || exit 1; done) && \
 5hyperfine \
 6  --sort 'command' \
 7  --runs 2 \
 8  --export-json "$BASE_DIR/ibd-${COMMITS// /-}-$STOP_HEIGHT-$DBCACHE-$C_COMPILER.json" \
 9  --parameter-list COMMIT ${COMMITS// /,} \
10  --prepare "killall bitcoind; rm -rf $DATA_DIR/*; git checkout {COMMIT}; git clean -fxd; git reset --hard; \
11    cmake -B build -DCMAKE_BUILD_TYPE=Release -DENABLE_WALLET=OFF -DCMAKE_C_COMPILER=$C_COMPILER -DCMAKE_CXX_COMPILER=$CXX_COMPILER && \
12    cmake --build build -j$(nproc) --target bitcoind && \
13    ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=1 -printtoconsole=0; sleep 100" \
14  --cleanup "cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
15  "COMPILER=$C_COMPILER COMMIT=${COMMIT:0:10} ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP_HEIGHT -dbcache=$DBCACHE -blocksonly -printtoconsole=0"
16
17d2b72b1369 refactor: rename leftover WriteBlockBench
18652b4e3de5 optimization: Bulk serialization writes in `WriteBlockUndo` and `WriteBlock`
19
20Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=888888 -dbcache=1000 -blocksonly -printtoconsole=0 (COMMIT = d2b72b13699cf460ffbcb1028bcf5f3b07d3b73a)
21  Time (mean ± σ):     41528.104 s ± 354.003 s    [User: 44324.407 s, System: 3074.829 s]
22  Range (min … max):   41277.786 s … 41778.421 s    2 runs
23 
24Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=888888 -dbcache=1000 -blocksonly -printtoconsole=0 (COMMIT = 652b4e3de5c5e09fb812abe265f4a8946fa96b54)
25  Time (mean ± σ):     38771.457 s ± 441.941 s    [User: 41930.651 s, System: 3222.664 s]
26  Range (min … max):   38458.957 s … 39083.957 s    2 runs
27 
28Relative speed comparison
29        1.07 ±  0.02  COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=888888 -dbcache=1000 -blocksonly -printtoconsole=0 (COMMIT = d2b72b13699cf460ffbcb1028bcf5f3b07d3b73a)
30        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=888888 -dbcache=1000 -blocksonly -printtoconsole=0 (COMMIT = 652b4e3de5c5e09fb812abe265f4a8946fa96b54)

Pushed a new version (with rebase because of recent CI failures):

• AutoFile/BufferedWriter scope reduction in WriteBlockUndo to make sure the file is closed before it’s reopened - as observed by @hodlinator. Based on previous similar test failures this would especially be problematic on Windows;
• Guard m_position dereference in new AutoFile::write_buffer (current usages have always had it, but it’s more consistent this way);
• BufferData to DataBuffer renamed I have;
• Primitive parameters in new tests now have comments.

Thanks for the comments, ready for further reviews.

Code review ACK c28221545bf23492ee3d32b9c64eca5fc38804eb. Lots of nice changes like code simplifications, documentation & naming improvements since the previous review but most significant change is new commit c28221545bf23492ee3d32b9c64eca5fc38804eb fixing a potential bug found by @hodlinator. Left some suggestions, but none are critical.

re: #31551 (comment)

Sorry for the extra pushes, it seems there is some inconsistency on Windows regarding file flushing and deletion behavior - so I’ve forced flushing in tests via braces and added read-underflow checks for each to make sure that scenario is covered. PR is ready for review again!

Could you explain in a little more detail what these failures were and how they were fixed? Could you also explain what exactly the (1«20) failures are, where they happen, and what’s the best theory about why they happen when previous approach using bigger buffer sizes did not seem to have a problem?

I don’t think any of these failures are very concerning, but I also don’t really understand them and previous PR discussion is hard to decipher so it it would be nice to have a clear description of the problems that have happened in one place.

Could you explain in a little more detail what these failures were and how they were fixed?

It was a test-only failure, I had an unnamed AutoFile{}.write() call to directly write the test content to a file in the buffered reader test. Since AutoFile was anonymous, it was closed immediately on POSIX systems but in Windows it lingered around without the last flush, but the readers have opened it up for testing already. See https://github.com/bitcoin/bitcoin/compare/38b7e8e5ced36cddd7e87fa852ac38daf906501a..fe6b0e425c88c42fc501acef86a642bc19feca9f#diff-73552341d85aec344497b47f1c2aa53a7e01a415030e40f56b1c454aef5f209dR567-R571

Could you also explain what exactly the (1«20) failures are, where they happen, and what’s the best theory about why they happen when previous approach using bigger buffer sizes did not seem to have a problem?

I have pushed it to a separate PR in my own repo and noticed that 1<<20 passes on a single thread (i.e. --jobs 1), it only fails when ci-only jobs are enabled and when we’re running on multiple threads, in which case it OOMs - but passes for lower values.

why they happen when previous approach using bigger buffer sizes did not seem to have a problem?

It likely didn’t fail for the previous approach since block413567.raw is less than 1<<20 and the build is passing for smaller values as well, we likely hit the limit with 1<<20 on multiple threads (in the same way as 1<<30 fails on a single thread).

The latest push contains:

• Migrated failure("AutoFile::write: position unknown") from AutoFile::write to AutoFile::write_buffer, guarded similarly behind a !m_xor.empty() (changed message slightly);
• Added explicit fileout.flush() to BlockManager::WriteBlockUndo next to the scope reduction to be more explicit and moved the scope introduction commit (which only does fileout.fclose()) next to the refactoring commits.

ACK 1f28d22940805492fbec5a9eb37a5ab767f8add1

Concept

Good to add read-buffering and switch to a bigger write buffer to help fine-tune IBD.

(Also fixes potential issue in BlockManager::WriteBlockUndo with flushing a file which is still open for write using another handle).

Microbenchmark results for Clang 19.1.7

0₿ build/bin/bench_bitcoin -min-time=30000 -filter=<bench_name>
1
2|    op/s |  err% |         ins/op |        bra/op | benchmark
3|--------:|------:|---------------:|--------------:|:----------
4|  348.16 |  0.7% |  17,892,592.79 |  2,786,616.55 | `WriteBlockBench` (with only rename-commit)
5|  527.23 |  0.8% |   9,938,390.94 |  1,059,982.61 | `WriteBlockBench` (with PR)
6|  153.67 |  0.0% |  70,042,685.48 |  4,703,506.75 | `ReadBlockBench` (with only rename-commit)
7|  179.84 |  0.0% |  63,495,559.95 |  3,329,391.01 | `ReadBlockBench` (with PR)

51% op/s improvement for WriteBlockBench. 17% for ReadBlockBench.

Code review ACK 1f28d22940805492fbec5a9eb37a5ab767f8add1. Only change since last review is moving the undo close-before-flush fix to an earlier commit and making it more explicit.

re: #31551 (comment)

Thanks for describing the previous failures. I still don’t really understand them and I guess I am not too concerned but would appreciate more of an explanation.

For the windows failure it seems like https://github.com/bitcoin/bitcoin/compare/38b7e8e5ced36cddd7e87fa852ac38daf906501a..fe6b0e425c88c42fc501acef86a642bc19feca9f#diff-73552341d85aec344497b47f1c2aa53a7e01a415030e40f56b1c454aef5f209dR567-R571 is not changing any test code but just dropping for (int rep{0}; rep < 10; ++rep) loop around the test. I don’t know how that changes things since there are no variables outside the loop and it looks like all files are closed before they are used again, both before and after that diff.

For the (1«20) OOM failure I just don’t even know the basics of when it happens and why it is expected to happen. From your reference to block413567.raw it sounds like it is happening in a benchmark? Normally I would not expect a benchmark to leak memory so is this happening in some special build with instrumentation that does leak memory, or just uses a lot of memory?

is not changing any test code

Github merged a few consecutive pushes, this is the smallest diff which showed the test fix: https://github.com/bitcoin/bitcoin/compare/aaceecad2ba85ac7513d1e2973960bdae65ab0fc..38b7e8e5ced36cddd7e87fa852ac38daf906501a?w=1#diff-73552341d85aec344497b47f1c2aa53a7e01a415030e40f56b1c454aef5f209dR568-R571 i.e. narrowing the scope of the Autofile{...}.write(...) test call.

0// Write out the file with random content
1{
2    AutoFile{test_file.Open(pos, false), obfuscation}.write(m_rng.randbytes<std::byte>(file_size));
3}
4BOOST_CHECK_EQUAL(fs::file_size(test_file.FileName(pos)), file_size);

Which assures that the same test file can be opened for reading now.

For the (1«20) OOM failure I just don’t even know the basics of when it happens and why it is expected to happen

That’s what bothers me as well, the build just fails silently. And if I make it single threaded to deduce which test causes it, it passes.

Normally I would not expect a benchmark to leak memory

We’re running the benchmarks as tests as well in https://github.com/bitcoin/bitcoin/blob/master/src/bench/CMakeLists.txt#L83. I don’t expect it to leak, just use extra instrumented memory on multiple threads which happens to add up, apparently. I’m trying to narrow down the culprit, but whenever I remove a few tests to see if the problem is with the other half, the build passes. And sometimes it takes 4 hours to run a build…

extra instrumented memory

Thanks but this is the part I don’t understand. Instrumented how? Which CI build is failing? Is there a link to the failure?

Instrumented how?

It’s the ASan + LSan + UBSan + integer, no depends, USDT build, so the sanitizers need to instrument the code to detect violations, which bloats the resource requirements - I read that AddressSanitizer typically doubles the memory overhead. The build doesn’t fail for uninstumented code or for smaller values.

Which CI build is failing?

My mistake, I though this was easy to find in https://github.com/l0rinc/bitcoin/pull/2 - see https://github.com/l0rinc/bitcoin/actions/runs/14329738280/job/40162805114?pr=2 terminating with a container sigterm:

…………………………………………………………………………. 13/318 - p2p_node_network_limited.py –v1transport passed, duration: 247 s

14/318 - p2p_node_network_limited.py –v2transport passed, duration: 250 s …..

………………………………………………………………. Error: Process completed with exit code 143.

For the (1«20) OOM failure I just don’t even know the basics of when it happens and why it is expected to happen. From your reference to block413567.raw it sounds like it is happening in a benchmark? Normally I would not expect a benchmark to leak memory so is this happening in some special build with instrumentation that does leak memory, or just uses a lot of memory?

Yeah, I don’t think the issue is in the benchmarks. The output from above (14/318 - p2p_node_network_limited.py --v2transport passed, duration: 250 s) is a functional test, which are run after the benchmarks (and unit tests) passed, so it seems more likely that the issue is while running the functional tests under asan.