Requested in #13151 (comment). See #26415 and #21319.
Benchmarking shows a >50x increase in speed on both nvme and spinning disk.
Benchmark results:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |
|---|---|---|---|---|---|---|---|---|---|
| 5,377,375.00 | 185.96 | 0.2% | 60,125,513.00 | 11,633,676.00 | 5.168 | 3,588,800.00 | 0.4% | 0.09 | ReadBlockFromDiskTest |
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |
|---|---|---|---|---|---|---|---|---|---|
| 89,945.58 | 11,117.83 | 0.7% | 12,743.90 | 64,530.33 | 0.197 | 2,595.20 | 0.2% | 0.01 | ReadRawBlockFromDiskTest |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, TheCharlatan, achow101 |
| Concept ACK | kevkevinpal, josibake |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
19+
20+static void SetupReadBlockTest()
21+{
22+ // Create params and set blocksdir to /tmp folder
23+ SelectParams(CBaseChainParams::MAIN);
24+ gArgs.ForceSetArg("-blocksdir", "/tmp");
fs::temp_directory_path().
19+
20+static void SetupReadBlockTest()
21+{
22+ // Create params and set blocksdir to temporary folder (/tmp usually on linux)
23+ SelectParams(CBaseChainParams::MAIN);
24+ gArgs.ForceSetArg("-blocksdir", fs::temp_directory_path().string());
22+ // Create params and set blocksdir to temporary folder (/tmp usually on linux)
23+ SelectParams(CBaseChainParams::MAIN);
24+ gArgs.ForceSetArg("-blocksdir", fs::temp_directory_path().string());
25+
26+ // Serialize block
27+ CDataStream stream(benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION);
0 CDataStream stream{benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION};
nit (for new code)
27+ CDataStream stream(benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION);
28+ CBlock block;
29+ stream >> block;
30+
31+ // Write block at first position in <temp_directory>/blocks/blk00000.dat
32+ CAutoFile fileout(OpenBlockFile(FlatFilePos(0, 0)), SER_DISK, CLIENT_VERSION);
0 CAutoFile fileout{OpenBlockFile(FlatFilePos{0, 0}), SER_DISK, CLIENT_VERSION};
nit
30+
31+ // Write block at first position in <temp_directory>/blocks/blk00000.dat
32+ CAutoFile fileout(OpenBlockFile(FlatFilePos(0, 0)), SER_DISK, CLIENT_VERSION);
33+ assert(!fileout.IsNull());
34+ const size_t block_size{GetSerializeSize(block, fileout.GetVersion())};
35+ fileout << Params().MessageStart() << (uint32_t) block_size << block;
0 fileout << Params().MessageStart() << uint32_t{block_size} << block;
16+using node::BLOCK_SERIALIZATION_HEADER_SIZE;
17+using node::OpenBlockFile;
18+using node::ReadBlockFromDisk;
19+using node::ReadRawBlockFromDisk;
20+
21+static void WriteBlockToDisk()
It seems we’re doing the same thing here already: https://github.com/bitcoin/bitcoin/blob/678889e6c6231cf461de59eefe6fb8eb07468848/src/bench/load_external.cpp#L27-L49.
Maybe I’m missing some nuance, but can we avoid reimplementing this?
OpenBlockFile and FlatFilePos abstractions so that it will work with ReadBlockFromDisk and ReadRawBlockFromDisk. If these are internally changed then this benchmark will still work (in theory). In load_external it’s writing to an arbitrary file that’s passed into LoadExternalBlockFile which doesn’t use these abstractions internally. It’s also filling up the file with repeated blocks, whereas here we only write one.
It’s actually reimplimenting https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L670-L691
However, it’s private in blockstorage.cpp, but might be worth refactoring it to expose it for this benchmark?
BlockManager::SaveBlockToDisk.
Tested ACK and got these results (compiled with –enable-debug)
Specs:
mac book pro 2019 2.3 GHz 8-Core Intel Core i9 16 GB 2667 MHz DDR4 AMD Radeon Pro 5500M 4 GB
➜ bitcoin git:(PR26684) ✗ ./src/bench/bench_bitcoin -filter=ReadBlockFromDiskTest
0Warning, results might be unstable:
1* DEBUG defined
2
3Recommendations
4* Make sure you compile for Release
5
6| ns/op | op/s | err% | total | benchmark
7|--------------------:|--------------------:|--------:|----------:|:----------
8| 21,699,865.00 | 46.08 | 1.8% | 0.24 | `ReadBlockFromDiskTest`
➜ bitcoin git:(PR26684) ✗ ./src/bench/bench_bitcoin -filter=ReadRawBlockFromDiskTest
0Warning, results might be unstable:
1* DEBUG defined
2
3Recommendations
4* Make sure you compile for Release
5
6| ns/op | op/s | err% | total | benchmark
7|--------------------:|--------------------:|--------:|----------:|:----------
8| 87,262.00 | 11,459.74 | 5.1% | 0.01 | :wavy_dash: `ReadRawBlockFromDiskTest` (Unstable with ~1.0 iters. Increase `minEpochIterations` to e.g. 10)
Concept ACK
Based on the linked PRs, seems like having this benchmark is useful. There’s also been some discussion about XOR’ing block data and having a benchmark for reading blocks would definitely be good to have before jumping into that.
I would, however, suggest holding off on merging this right now given that it conflicts with BIP324 PRs and is more of a nice to have vs something urgently needed.
27+
28+ CBlock block;
29+ const auto pos{WriteBlockToDisk(chainman)};
30+
31+ bench.run([&] {
32+ assert(chainman.m_blockman.ReadBlockFromDisk(block, pos));
as per developer notes, assertions shouldn’t have side-effects, so the following would be preferred:
0 bool success{chainman.m_blockman.ReadBlockFromDisk(block, pos)};
1 assert(success);
(same for the second test below)
if it’d also catch our own assertion-variants)
They are required to have side-effects, as they must return the value that was passed in. So checking them is not possible and it seems better to consistently check nothing than to special case one function.
Okay, that makes sense. My blind guess was that clang-tidy (or any other similar tool) would only look at the passed expression and not care about the effect of the assert as a whole. Seems like even the passed expression is not side-effect free at several places in the code, e.g. https://github.com/bitcoin/bitcoin/blob/b565485c24c0feacae559a7f6f7b83d7516ca58d/src/init.cpp#L1428
so it indeed might be better to remove recommendations that we don’t even follow ourselves.
(Also, I sadly can’t even get clang-tidy to yield anything useful for regular asserts).
lgtm ACK. I think one issue could be that the bench (like for all tests) datadir is on a tmpfs and not the normal storage that blocks are stored on, so the bench is skewed toward highlighting code performance more than it matters, because block storage is generally slower (than tmpfs in memory) and the limiting factor.
Maybe a hint or comment on how to pick the datadir for this test can be added?
Edit, not sure if the benchmarks parse args like bench_bitcoin ... -- -datadir=$HOME/temp, but TMPDIR=$HOME/tmp/ may also work, see https://en.cppreference.com/w/cpp/filesystem/temp_directory_path. See also https://github.com/bitcoin/bitcoin/pull/26564
10+#include <streams.h>
11+#include <test/util/setup_common.h>
12+#include <util/chaintype.h>
13+#include <validation.h>
14+
15+static FlatFilePos WriteBlockToDisk(ChainstateManager& chainman) {
11+#include <test/util/setup_common.h>
12+#include <util/chaintype.h>
13+#include <validation.h>
14+
15+static FlatFilePos WriteBlockToDisk(ChainstateManager& chainman) {
16+ CDataStream stream{benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION};
0 DataStream stream{benchmark::data::block413567};
(see compile failure)
8+#include <consensus/validation.h>
9+#include <node/blockstorage.h>
10+#include <streams.h>
11+#include <test/util/setup_common.h>
12+#include <util/chaintype.h>
13+#include <validation.h>
22+}
23+
24+static void ReadBlockFromDiskTest(benchmark::Bench& bench)
25+{
26+ const auto testing_setup{MakeNoLogFileContext<const TestingSetup>(ChainType::MAIN)};
27+ ChainstateManager& chainman{*testing_setup->m_node.chainman};
BlockManager& already.
