bench: Benchmark blockToJSON #16267

pull fanatid wants to merge 1 commits into bitcoin:master from fanatid:bench-blocktojson changing 3 files +35 −0

fanatid commented at 10:44 AM on June 22, 2019: contributor
Related:
- "getblock performance issue on verbosity" #15925
- "refactor: Avoid UniValue copy constructor" #15974
fanquake added the label Tests on Jun 22, 2019
fanatid force-pushed on Jun 22, 2019
in src/bench/checkblock.cpp:19 in 1f034e1772 outdated
```
  14 |  
  15 |  namespace block_bench {
  16 |  #include <bench/data/block413567.raw.h>
  17 |  } // namespace block_bench
  18 |  
  19 | +static void SerializeJSONBlockTest(benchmark::State& state) {
```
promag commented at 2:10 PM on June 24, 2019:

This could be in a new benchmark, for instance, src/bench/rpc_blockchain.cpp.

promag commented at 2:13 PM on June 24, 2019:

nit, could rename to BlockToJsonVerbose.

fanatid commented at 1:35 PM on June 25, 2019:

@promag moved and renamed
in src/bench/checkblock.cpp:39 in 1f034e1772 outdated
```
  34 | +    while (state.KeepRunning()) {
  35 | +        (void)blockToJSON(block, &blockindex, &blockindex, /*verbose*/ true);
  36 | +    }
  37 | +}
  38 | +
  39 | +BENCHMARK(SerializeJSONBlockTest, 10);
```
promag commented at 2:12 PM on June 24, 2019:

These are usually together in the end.
promag commented at 2:19 PM on June 24, 2019:

10 seems to be a lot, on my system (2,9 GHz Intel Core i9):

time ./src/bench/bench_bitcoin -filter=SerializeJSONBlockTest # Benchmark, evals, iterations, total, min, max, median SerializeJSONBlockTest, 5, 10, 10.0509, 0.197745, 0.20436, 0.200661 ./src/bench/bench_bitcoin -filter=SerializeJSONBlockTest 9.54s user 1.08s system 90% cpu 11.755 total

I think @MarcoFalke said that each should take around 4 seconds?
fanatid commented at 1:35 PM on June 25, 2019:

On i5-8250U 10 looks fine?

$ time ./src/bench/bench_bitcoin -filter=BlockToJsonVerbose # Benchmark, evals, iterations, total, min, max, median BlockToJsonVerbose, 5, 10, 5.64343, 0.104094, 0.118915, 0.112168 real 0m6.397s user 0m6.183s sys 0m0.202s
MarcoFalke commented at 2:00 PM on June 25, 2019:

I think @MarcoFalke said that each should take around 4 seconds?

Yeah, they should take as much as all other tests take on average, which is 4 seconds on my machine. Though, this will vary based on what hardware you use.

fanatid commented at 2:08 PM on June 25, 2019:

How much I should set? I set 10 because it's give nearly 1s which based on this comment: https://github.com/bitcoin/bitcoin/blob/332c6134bb15384e5b91c631e821fe52a591d3bc/src/bench/bench.h#L137
promag commented at 2:19 PM on June 24, 2019: member

Concept ACK, some comments for your consideration.

in build_msvc/bench_bitcoin/bench_bitcoin.vcxproj:77 in cf75a25518 outdated

  73 | @@ -73,4 +74,4 @@
  74 |    <Import Label="hexdumpTarget" Project="..\msbuild\tasks\hexdump.targets" />
  75 |    <Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" />
  76 |    <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
  77 | -</Project>
  78 | \ No newline at end of file
  79 | +</Project>

promag commented at 1:48 PM on June 25, 2019:

nit, unrelated change.

in src/bench/rpc_blockchain.cpp:1 in cf75a25518 outdated
```
   0 | @@ -0,0 +1,38 @@
   1 | +// Copyright (c) 2016-2018 The Bitcoin Core developers
```
promag commented at 1:49 PM on June 25, 2019:

nit, 2019.
promag commented at 2:09 PM on June 25, 2019: member

ACK cf75a25, needs squash though.
fanatid force-pushed on Jun 25, 2019
fanatid commented at 2:19 PM on June 25, 2019: contributor

fixed unrelated changes and squashed
promag commented at 10:46 PM on June 25, 2019: member

ACK 8c1650d8b5014092cef511b3d56a49eda4c44424.
laanwj commented at 12:10 PM on June 27, 2019: member

Concept and ~~code review ACK 8c1650d8b5014092cef511b3d56a49eda4c44424~~
in src/bench/rpc_blockchain.cpp:15 in 8c1650d8b5 outdated
```
  10 | +#include <rpc/blockchain.h>
  11 | +
  12 | +#include <univalue.h>
  13 | +
  14 | +namespace block_bench {
  15 | +#include <bench/data/block413567.raw.h>
```
laanwj commented at 12:14 PM on June 27, 2019:

This is a huge amount of data which is already part of the executable through checkblock.cpp. I think you need to make sure that this is only compiled and linked once.

fanatid commented at 12:29 PM on June 27, 2019:

Can you suggest how I can resolve this? Define namespace block_bench in header file? (not sure that this will help) Move namespace to own cpp file?

promag commented at 1:54 PM on June 27, 2019:

@laanwj nice catch, I wasn't aware of the generation rule in the makefile. @fanatid I'm going to push a commit with a suggestion.

promag commented at 2:17 PM on June 27, 2019:

@fanatid see #16299, you might want to rebase with it.

fanatid commented at 6:30 PM on June 27, 2019:

Thanks! Rebased on top of #16299
fanatid force-pushed on Jun 27, 2019
promag commented at 7:21 PM on June 27, 2019: member

@fanatid had to push a new commit to fix windows build. Maybe wait until it's merged to rebase this one.
DrahtBot commented at 8:18 PM on June 27, 2019: member



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



Conflicts

No conflicts as of last run.
promag commented at 2:32 PM on July 2, 2019: member

@fanatid FYI base PR is now building.
fanatid force-pushed on Jul 3, 2019
fanatid commented at 3:34 PM on July 3, 2019: contributor

Rebased. @promag thank you!
bench: Benchmark blockToJSON 91509ffe24
fanatid force-pushed on Jul 5, 2019
in src/bench/rpc_blockchain.cpp:21 in 91509ffe24
```
  16 | +    CDataStream stream(benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION);
  17 | +    char a = '\0';
  18 | +    stream.write(&a, 1); // Prevent compaction
  19 | +
  20 | +    CBlock block;
  21 | +    stream >> block;
```
fqlx commented at 11:10 PM on July 5, 2019:

Doesn't this need to be assigned?

promag commented at 12:11 AM on July 6, 2019:

Assign what? This uses the >> operator.

If you don't mind, please take some time reading the code/docs and discussions in other pull requests before jumping to quick reviews. In this case you would find more occurrences and then you could try to understand it.

in src/bench/rpc_blockchain.cpp:29 in 91509ffe24

  24 | +    const uint256 blockHash = block.GetHash();
  25 | +    blockindex.phashBlock = &blockHash;
  26 | +    blockindex.nBits = 403014710;
  27 | +
  28 | +    while (state.KeepRunning()) {
  29 | +        (void)blockToJSON(block, &blockindex, &blockindex, /*verbose*/ true);

fqlx commented at 11:13 PM on July 5, 2019:

No need to cast void here

promag commented at 12:06 AM on July 6, 2019:

It's needed to explicitly inform that the return value is not needed, otherwise a compiler warning is raised.

in src/bench/rpc_blockchain.cpp:18 in 91509ffe24

  13 | +#include <univalue.h>
  14 | +
  15 | +static void BlockToJsonVerbose(benchmark::State& state) {
  16 | +    CDataStream stream(benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION);
  17 | +    char a = '\0';
  18 | +    stream.write(&a, 1); // Prevent compaction

fqlx commented at 11:14 PM on July 5, 2019:

Can you explain this comment more? Or move hacks into a one line function?

laanwj commented at 6:14 PM on July 8, 2019: member

ACK 91509ffe247b0eacbf84214c7c9c3f8a0012f2eb
laanwj merged this on Jul 8, 2019
laanwj closed this on Jul 8, 2019
laanwj referenced this in commit 0a6ee9797e on Jul 8, 2019
fanatid deleted the branch on Jul 8, 2019
sidhujag referenced this in commit 6678aba111 on Jul 9, 2019
deadalnix referenced this in commit 2e7115ee28 on May 6, 2020
Munkybooty referenced this in commit 2cd358cfcb on Nov 4, 2021
Munkybooty referenced this in commit da34449bb7 on Nov 4, 2021
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#15925 getblock performance issue on verbosity #15974 refactor: Avoid UniValue copy constructor

bench: Benchmark blockToJSON #16267

Conflicts