bench: add readblock benchmark #26684

andrewtoth · 2022-12-11T02:22:23Z

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`

DrahtBot · 2022-12-11T02:22:25Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

No conflicts as of last run.

src/bench/readblock.cpp

maflcko

(feel free to ignore the nits)

src/bench/readblock.cpp

kevkevinpal · 2022-12-23T01:07:22Z

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

Warning, results might be unstable:
* DEBUG defined

Recommendations
* Make sure you compile for Release

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       21,699,865.00 |               46.08 |    1.8% |      0.24 | `ReadBlockFromDiskTest`

➜ bitcoin git:(PR26684) ✗ ./src/bench/bench_bitcoin -filter=ReadRawBlockFromDiskTest

Warning, results might be unstable:
* DEBUG defined

Recommendations
* Make sure you compile for Release

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           87,262.00 |           11,459.74 |    5.1% |      0.01 | :wavy_dash: `ReadRawBlockFromDiskTest` (Unstable with ~1.0 iters. Increase `minEpochIterations` to e.g. 10)

josibake · 2023-05-03T10:05:01Z

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.

src/bench/readblock.cpp

maflcko

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 #26564

src/bench/readblock.cpp

fa6e6a3 doc: Remove confusing assert linter (MarcoFalke) Pull request description: The `assert()` documentation and linter are redundant and confusing: * The source code already refuses to compile with `assert()` disabled. * They violate the assumptions about `Assert()`, which *requires* side effects. * The existing linter doesn't enforce the guideline, only checking for `++` and `--` side effects. Fix all issues by removing the docs and the linter. See also bitcoin/bitcoin#26684 (comment) Going forward everyone is free to use whatever code in this regard they think is the easiest to read. Also, everyone is still free to share style-nits, if they think it is a good use of their time and of the pull request author. Finally, the author is still free to dismiss or ignore this style-nit, or any other style-nit. ACKs for top commit: hebasto: ACK fa6e6a3, I have reviewed the code and it looks OK. theStack: ACK fa6e6a3 Tree-SHA512: 686738d71e1316cc95e5d3f71869b55a02bfb137c795cc0875057f4410e564bc8eff03c985a2087b007fb08fc84551c7da1e8b30c7a9c3f2b14e5e44a5970236

fa6e6a3 doc: Remove confusing assert linter (MarcoFalke) Pull request description: The `assert()` documentation and linter are redundant and confusing: * The source code already refuses to compile with `assert()` disabled. * They violate the assumptions about `Assert()`, which *requires* side effects. * The existing linter doesn't enforce the guideline, only checking for `++` and `--` side effects. Fix all issues by removing the docs and the linter. See also bitcoin#26684 (comment) Going forward everyone is free to use whatever code in this regard they think is the easiest to read. Also, everyone is still free to share style-nits, if they think it is a good use of their time and of the pull request author. Finally, the author is still free to dismiss or ignore this style-nit, or any other style-nit. ACKs for top commit: hebasto: ACK fa6e6a3, I have reviewed the code and it looks OK. theStack: ACK fa6e6a3 Tree-SHA512: 686738d71e1316cc95e5d3f71869b55a02bfb137c795cc0875057f4410e564bc8eff03c985a2087b007fb08fc84551c7da1e8b30c7a9c3f2b14e5e44a5970236

src/bench/readblock.cpp

maflcko · 2023-11-27T11:56:41Z

lgtm ACK 1c4b9cb

TheCharlatan

ACK 1c4b9cb

TheCharlatan · 2023-12-20T21:47:27Z

src/bench/readblock.cpp

+#include <streams.h>
+#include <test/util/setup_common.h>
+#include <util/chaintype.h>
+#include <validation.h>


NIt: Could make this IWYU correct (see the logs from the tidy CI job).

TheCharlatan · 2023-12-20T21:49:14Z

src/bench/readblock.cpp

+static void ReadBlockFromDiskTest(benchmark::Bench& bench)
+{
+    const auto testing_setup{MakeNoLogFileContext<const TestingSetup>(ChainType::MAIN)};
+    ChainstateManager& chainman{*testing_setup->m_node.chainman};


Nit: This could be BlockManager& already.

achow101 · 2024-01-02T16:06:12Z

ACK 1c4b9cb

…ck by reading raw block e710cef rest: read raw block in rest_block and deserialize for json (Andrew Toth) 95ce078 rpc: read raw block in getblock and deserialize for verbosity > 0 (Andrew Toth) 0865ab8 test: check more details on zmq raw block response (Andrew Toth) 38265cc zmq: read raw block with ReadRawBlockFromDisk (Andrew Toth) da338aa blockstorage: check nPos in ReadRawBlockFromDisk before seeking back (Andrew Toth) Pull request description: For the `getblock` endpoint with `verbosity=0`, the `rest_block` REST endpoint for `bin` and `hex`, and zmq `NotifyBlock` we don't have to deserialize the block since we're just sending the raw data. This PR uses `ReadRawBlockFromDisk` instead of `ReadBlockFromDisk` to serve these requests, and only deserializes for `verbosity > 0` and `json` REST requests. See benchmarks in #26684. Benchmarked using ApacheBench. Requesting block 750,000 in binary 10k times on a single core (set `-rest=1` in config): `ab -n 10000 -c 1 "http://127.0.0.1:8332/rest/block/0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e.bin"` On master, mean time 15ms. On this branch, mean time 1ms. For RPC ``` echo '{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", 0]}' > /tmp/data.json ab -p /tmp/data.json -n 1000 -c 1 -A user:password "http://127.0.0.1:8332/" ``` On master, mean time 32ms On this branch, mean time 13ms ACKs for top commit: achow101: re-ACK e710cef Tree-SHA512: 4cea13c7b563b2139d041b1fdcfdb793c8cc688654ae08db07e7ee6b875c5e582b8185db3ae603abbfb06d2164724f29205774620b48c493726b991999af289e

fa6e6a3 doc: Remove confusing assert linter (MarcoFalke) Pull request description: The `assert()` documentation and linter are redundant and confusing: * The source code already refuses to compile with `assert()` disabled. * They violate the assumptions about `Assert()`, which *requires* side effects. * The existing linter doesn't enforce the guideline, only checking for `++` and `--` side effects. Fix all issues by removing the docs and the linter. See also bitcoin#26684 (comment) Going forward everyone is free to use whatever code in this regard they think is the easiest to read. Also, everyone is still free to share style-nits, if they think it is a good use of their time and of the pull request author. Finally, the author is still free to dismiss or ignore this style-nit, or any other style-nit. ACKs for top commit: hebasto: ACK fa6e6a3, I have reviewed the code and it looks OK. theStack: ACK fa6e6a3 Tree-SHA512: 686738d71e1316cc95e5d3f71869b55a02bfb137c795cc0875057f4410e564bc8eff03c985a2087b007fb08fc84551c7da1e8b30c7a9c3f2b14e5e44a5970236

DrahtBot added the Tests label Dec 11, 2022

andrewtoth force-pushed the bench-readblock branch 3 times, most recently from 1b7e839 to 592f801 Compare December 11, 2022 17:08

kristapsk reviewed Dec 11, 2022

View reviewed changes

src/bench/readblock.cpp Outdated Show resolved Hide resolved

andrewtoth force-pushed the bench-readblock branch 2 times, most recently from b083b56 to 2142fc9 Compare December 11, 2022 18:20

maflcko reviewed Dec 12, 2022

View reviewed changes

src/bench/readblock.cpp Outdated Show resolved Hide resolved

maflcko reviewed Dec 12, 2022

View reviewed changes

src/bench/readblock.cpp Outdated Show resolved Hide resolved

src/bench/readblock.cpp Outdated Show resolved Hide resolved

src/bench/readblock.cpp Outdated Show resolved Hide resolved

andrewtoth force-pushed the bench-readblock branch from 2142fc9 to 493df1a Compare December 13, 2022 02:00

stickies-v reviewed Dec 14, 2022

View reviewed changes

src/bench/readblock.cpp Outdated Show resolved Hide resolved

andrewtoth force-pushed the bench-readblock branch 3 times, most recently from 521125f to ac8f68e Compare December 14, 2022 19:05

andrewtoth mentioned this pull request Feb 6, 2023

rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block #26415

Merged

achow101 requested review from theStack and josibake April 25, 2023 15:44

DrahtBot added the CI failed label May 11, 2023

andrewtoth force-pushed the bench-readblock branch from ac8f68e to 6de2f65 Compare August 2, 2023 23:26

DrahtBot removed the CI failed label Aug 3, 2023

andrewtoth force-pushed the bench-readblock branch from 6de2f65 to 7fba1fb Compare August 7, 2023 14:15

theStack reviewed Aug 7, 2023

View reviewed changes

src/bench/readblock.cpp Outdated Show resolved Hide resolved

maflcko approved these changes Aug 8, 2023

View reviewed changes

maflcko reviewed Aug 8, 2023

View reviewed changes

src/bench/readblock.cpp Outdated Show resolved Hide resolved

maflcko mentioned this pull request Aug 21, 2023

doc: Remove confusing assert linter #28304

Merged

DrahtBot added the CI failed label Nov 18, 2023

maflcko reviewed Nov 19, 2023

View reviewed changes

src/bench/readblock.cpp Outdated Show resolved Hide resolved

andrewtoth force-pushed the bench-readblock branch from 7fba1fb to a4c2f3c Compare November 26, 2023 18:22

bench: add readblock benchmark

1c4b9cb

andrewtoth force-pushed the bench-readblock branch from a4c2f3c to 1c4b9cb Compare November 26, 2023 18:24

DrahtBot removed the CI failed label Nov 26, 2023

furszy mentioned this pull request Nov 28, 2023

index: block filters sync, reduce disk read operations by caching last header #28955

Merged

furszy mentioned this pull request Dec 15, 2023

test: test_bitcoin: allow -testdatadir=<datadir> #26564

Merged

TheCharlatan approved these changes Dec 20, 2023

View reviewed changes

achow101 merged commit 00bf4a1 into bitcoin:master Jan 2, 2024

andrewtoth deleted the bench-readblock branch January 2, 2024 16:18

hebasto mentioned this pull request Jan 9, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#75

Closed

bitcoin locked and limited conversation to collaborators Jan 1, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

bench: add readblock benchmark #26684

bench: add readblock benchmark #26684

Uh oh!

andrewtoth commented Dec 11, 2022

Uh oh!

DrahtBot commented Dec 11, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevkevinpal commented Dec 23, 2022 •

edited

Loading

Uh oh!

josibake commented May 3, 2023

Uh oh!

Uh oh!

maflcko left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

maflcko commented Nov 27, 2023

Uh oh!

TheCharlatan left a comment

Uh oh!

TheCharlatan Dec 20, 2023

Uh oh!

TheCharlatan Dec 20, 2023

Uh oh!

achow101 commented Jan 2, 2024

Uh oh!

Uh oh!

bench: add readblock benchmark #26684

bench: add readblock benchmark #26684

Uh oh!

Conversation

andrewtoth commented Dec 11, 2022

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevkevinpal commented Dec 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josibake commented May 3, 2023

Uh oh!

Uh oh!

maflcko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Nov 27, 2023

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

TheCharlatan Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jan 2, 2024

Uh oh!

Uh oh!

DrahtBot commented Dec 11, 2022 •

edited

Loading

kevkevinpal commented Dec 23, 2022 •

edited

Loading

maflcko left a comment •

edited

Loading