Skip to content

bench: Benchmark blockToJSON #16267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 1 commit into from
Jul 8, 2019
Merged

bench: Benchmark blockToJSON #16267

laanwj merged 1 commit into from
Jul 8, 2019

Conversation

@fanatid
Copy link
Contributor

@fanatid fanatid commented Jun 22, 2019

Related:

@fanquake fanquake added the Tests label Jun 22, 2019
@fanatid fanatid force-pushed the bench-blocktojson branch from 890fc96 to 1f034e1 Compare June 22, 2019 11:26
promag
promag reviewed Jun 24, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, some comments for your consideration.

src/bench/checkblock.cpp Outdated
Copy link
Contributor

@promag promag Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are usually together in the end.

Copy link
Contributor

@promag promag Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@fanatid fanatid Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@maflcko maflcko Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@fanatid fanatid Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much I should set? I set 10 because it's give nearly 1s which based on this comment:

bitcoin/src/bench/bench.h

Line 137 in 332c613

// Choose a num_iters_for_one_second that takes roughly 1 second. The goal is that all benchmarks should take approximately

promag
promag reviewed Jun 25, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK cf75a25, needs squash though.

@fanatid fanatid force-pushed the bench-blocktojson branch from cf75a25 to 8c1650d Compare June 25, 2019 14:19
@fanatid
Copy link
Contributor Author

fanatid commented Jun 25, 2019

fixed unrelated changes and squashed

@promag
Copy link
Contributor

promag commented Jun 25, 2019

ACK 8c1650d8b5014092cef511b3d56a49eda4c44424.

@laanwj
Copy link
Member

laanwj commented Jun 27, 2019 •
edited
Loading

Concept and code review ACK 8c1650d8b5014092cef511b3d56a49eda4c44424

laanwj
laanwj reviewed Jun 27, 2019
View reviewed changes
@fanatid fanatid force-pushed the bench-blocktojson branch from 8c1650d to 7766475 Compare June 27, 2019 18:30
@promag
Copy link
Contributor

promag commented Jun 27, 2019

@fanatid had to push a new commit to fix windows build. Maybe wait until it's merged to rebase this one.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 27, 2019 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@promag
Copy link
Contributor

promag commented Jul 2, 2019

@fanatid FYI base PR is now building.

@fanatid fanatid force-pushed the bench-blocktojson branch from 7766475 to 9859060 Compare July 3, 2019 15:33
@fanatid
Copy link
Contributor Author

fanatid commented Jul 3, 2019

Rebased.
@promag thank you!

@fanatid fanatid force-pushed the bench-blocktojson branch from 9859060 to 91509ff Compare July 5, 2019 14:54
fqlx
fqlx reviewed Jul 5, 2019
View reviewed changes
fqlx
fqlx reviewed Jul 5, 2019
View reviewed changes
src/bench/rpc_blockchain.cpp
static void BlockToJsonVerbose(benchmark::State& state) {
CDataStream stream(benchmark::data::block413567, SER_NETWORK, PROTOCOL_VERSION);
char a = '\0';
stream.write(&a, 1); // Prevent compaction
Copy link

@fqlx fqlx Jul 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@laanwj
Copy link
Member

laanwj commented Jul 8, 2019

ACK 91509ff

@laanwj laanwj merged commit 91509ff into bitcoin:master Jul 8, 2019
laanwj added a commit that referenced this pull request Jul 8, 2019
@laanwj
Merge #16267: bench: Benchmark blockToJSON
0a6ee97 
91509ff bench: Benchmark blockToJSON (Kirill Fomichev)

Pull request description:

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

ACKs for top commit:
  laanwj:
    ACK 91509ff

Tree-SHA512: e70b12cb31921c7527bde334f52f39776da698b6bbdb196079a8b68478c67585a5bd7bed7403f65166bd604f7ed60778c53dc064d743bb8368318a1283d1073e
@fanatid fanatid deleted the bench-blocktojson branch July 8, 2019 19:01
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 9, 2019
@laanwj @sidhujag
Merge bitcoin#16267: bench: Benchmark blockToJSON
6678aba 
91509ff bench: Benchmark blockToJSON (Kirill Fomichev)

Pull request description:

  Related:
  - "getblock performance issue on verbosity" bitcoin#15925
  - "refactor: Avoid UniValue copy constructor" bitcoin#15974

ACKs for top commit:
  laanwj:
    ACK 91509ff

Tree-SHA512: e70b12cb31921c7527bde334f52f39776da698b6bbdb196079a8b68478c67585a5bd7bed7403f65166bd604f7ed60778c53dc064d743bb8368318a1283d1073e
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 6, 2020
@laanwj @jasonbcox
Merge #16267: bench: Benchmark blockToJSON
2e7115e 
Summary:
91509ffe247b0eacbf84214c7c9c3f8a0012f2eb bench: Benchmark blockToJSON (Kirill Fomichev)

Pull request description:

  Related:
  - "getblock performance issue on verbosity" bitcoin/bitcoin#15925
  - "refactor: Avoid UniValue copy constructor" #15974

ACKs for top commit:
  laanwj:
    ACK 91509ffe247b0eacbf84214c7c9c3f8a0012f2eb

Tree-SHA512: e70b12cb31921c7527bde334f52f39776da698b6bbdb196079a8b68478c67585a5bd7bed7403f65166bd604f7ed60778c53dc064d743bb8368318a1283d1073e

Backport of Core [[bitcoin/bitcoin#16267 | PR16267]]

Also cleans up the includes per Core [[bitcoin/bitcoin#16659 | PR16659]]

Depends on D5980

Test Plan:
```
ninja check
ninja src/bench/bitcoin-bench
./src/bench/bitcoin-bench -filter=BlockToJsonVerbose
```

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D5981
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+3 more reviewers

@fqlx fqlx fqlx left review comments

@promag promag promag left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fanatid @promag @laanwj @DrahtBot @fqlx @maflcko @fanquake