bench: update nanobench add `-min_time` #23025

martinus · 2021-09-18T10:44:59Z

This PR updates the nanobench with the latest release from upstream, v4.3.6. It fixes the missing performance counters.

Due to discussions on #22999 I have done some work that should make the benchmark results more reliable. It introduces a new flag -min_time that allows to run a benchmark for much longer then the default. When results are unreliable, choosing a large timeframe here should usually get repeatable results even when frequency scaling cannot be disabled. The default is now 10ms. For this to work I have changed the AddrManGood and EvictionProtection benchmarks so they work with any number of iterations.

Also, this adds more usage documentation to bench_bitcoin -h and I've cherry-picked two changes from #22999 authored by Jon Atack

jonatack

Nice--thank you for working on this! Using the min duration arg does improve stability for me (lower err%) and is convenient as a runtime option.

Tested Concept/Approach ACK with various suggestions below.

src/bench/peer_eviction.cpp

src/bench/addrman.cpp

src/bench/bench_bitcoin.cpp

src/bench/bench.h

src/bench/rollingbloom.cpp

src/bench/bench_bitcoin.cpp

src/bench/bench.cpp

DrahtBot · 2021-09-19T02:50:48Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22950 ([p2p] Pimpl AddrMan to abstract implementation details by amitiuttarwar)

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.

martinus · 2021-09-19T11:00:51Z

Thanks for the reviews! I've incorporated all the suggestions into the right commits and rebased everything.

jonatack

Almost-ACK b901174

src/bench/bench_bitcoin.cpp

src/bench/bench.cpp

martinus · 2021-09-20T05:32:26Z

Rebased with header cleanups, and signed all commits.

jonatack · 2021-09-20T08:53:10Z

ACK e7e8900

src/bench/addrman.cpp

Most importantly, this update fixes a bug in nanobench that always disabled performance counters on linux. It also adds another sanitizer suppression that is caught in clang++ 12.

Introduced in bitcoin#19055, MuHashDiv benchmark used to multiply with a loop based on epochIterations. That does not do what it is supposed to do, because epochIterations() is determined automatically from nanobench. Also, multiplication is not needed for the algorithm (as pointed out by a comment in bitcoin#19055), so it's better to remove this loop.

Moves some of the setup into the benchmark loop so it is possible to run the loop for an arbitrary number of times. Due to recent optimizations in bitcoin#22974 the benchmark now runs much faster, so the inner loop now calls Good() 32 times as often to get better numbers. Renamed the benchmark to AddrManAddThenGood because that's now what is actually tested. To get the the number of just Good(), one needs to subtract the benchmark result of AddrManAdd.

Moves copying of the setup into the benchmark loop so it is possible to run the loop for an arbitrary number of times. The overhead due to copying the candidates inside the loop is about 3%.

When it is not easily possible to stabilize benchmark machine and code the argument -min_time can be used to specify a minimum duration that a benchmark should take. E.g. choose -min_time=1000 if you are willing to wait about 1 second for each benchmark result. The default is now set to 10ms instead of 0, which should make runs on fast machines more stable with negligible slowdown.

This adds some usage description with tips to `bench_bitcoin -h`.

Drops unneeded and adds missing includes

- use ALLOW_BOOL for -list arg instead of ALLOW_ANY - touch up `-asymptote=<n1,n2,n3...>` help - pack Args struct a bit more efficiently - handle args in alphabetical order

The benchmarks can now run much longer due to the minimum of 10ms or directly with -min_time. With -min_time=20000 I could trigger two ubsan errors in the benchmarks, which are fixed in this commit by using unsigned type and adding "& 0xFF".

jonatack · 2021-09-21T13:27:12Z

re-ACK e148a52

The Win64 CI failure looks unrelated.

$ NANOBENCH_SUPPRESS_WARNINGS=1 ./bench_bitcoin -filter=AddrMan*.* -min_time=20000

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|      762,448,657.67 |                1.31 |    6.8% |     23.20 | `AddrManAdd`
|    1,794,860,430.00 |                0.56 |    2.5% |     19.71 | `AddrManAddThenGood`
|        9,101,993.99 |              109.87 |    7.8% |     20.56 | `AddrManGetAddr`
|           11,379.90 |           87,874.25 |    2.1% |     21.93 | `AddrManSelect`

If you retouch, maybe order the addrman benchmark functions alphabetically, which would be the same order as the bench result output and place the two similar functions, AddrManAdd and AddrManAddThenGood, next to each other.

martinus · 2021-09-21T13:51:28Z

@jonatack Interesting that your benchmark results are so different. Did you compile for release? I can't believe my computer is ~20 times faster than yours... I compile with ./autogen.sh && ./configure --with-incompatible-bdb CC=clang CXX=clang++

jonatack · 2021-09-21T13:58:02Z

@martinus no, it's a debug build and the computer is not optimized. Re-building a non-debug build would take a long time (I have a slow 2-core laptop) and I'd already tested it properly above. Sorry for the weird example.

laanwj · 2021-09-24T16:38:53Z

Code review ACK e148a52

fanquake added the Upstream label Sep 18, 2021

jonatack reviewed Sep 18, 2021

View reviewed changes

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

This was referenced Sep 19, 2021

[p2p] Pimpl AddrMan to abstract implementation details #22950

Merged

net: Encapsulate asmap in NetGroupManager #22910

Merged

martinus force-pushed the 2021-09-update-nanobench branch from 354fbc0 to b901174 Compare September 19, 2021 10:51

martinus changed the title ~~bench: update nanobench add -min_duration_ms~~ bench: update nanobench add -min_time Sep 19, 2021

jonatack reviewed Sep 19, 2021

View reviewed changes

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

src/bench/bench.cpp Show resolved Hide resolved

martinus force-pushed the 2021-09-update-nanobench branch from b901174 to e7e8900 Compare September 20, 2021 05:31

maflcko reviewed Sep 20, 2021

View reviewed changes

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

martinus added 2 commits September 21, 2021 11:46

bench: update nanobench from 4.3.4 to 4.3.6

eed99cf

Most importantly, this update fixes a bug in nanobench that always disabled performance counters on linux. It also adds another sanitizer suppression that is caught in clang++ 12.

laanwj added the Tests label Sep 21, 2021

martinus and others added 7 commits September 21, 2021 14:45

bench: make EvictionProtection.* work with any number of iterations

9fef832

Moves copying of the setup into the benchmark loop so it is possible to run the loop for an arbitrary number of times. The overhead due to copying the candidates inside the loop is about 3%.

bench: add usage description and documentation

1f10f16

This adds some usage description with tips to `bench_bitcoin -h`.

bench: clean up includes

d312fd9

Drops unneeded and adds missing includes

bench: various args improvements

da4e2f1

- use ALLOW_BOOL for -list arg instead of ALLOW_ANY - touch up `-asymptote=<n1,n2,n3...>` help - pack Args struct a bit more efficiently - handle args in alphabetical order

bench: fixed ubsan implicit conversion

e148a52

The benchmarks can now run much longer due to the minimum of 10ms or directly with -min_time. With -min_time=20000 I could trigger two ubsan errors in the benchmarks, which are fixed in this commit by using unsigned type and adding "& 0xFF".

martinus force-pushed the 2021-09-update-nanobench branch from e7e8900 to e148a52 Compare September 21, 2021 12:47

Bossday mentioned this pull request Sep 21, 2021

. #23055

Closed

laanwj merged commit 03cb2b4 into bitcoin:master Sep 24, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 24, 2021

Merge bitcoin#23025: bench: update nanobench add -min_time

1f347af

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 24, 2021

Merge bitcoin#23025: bench: update nanobench add -min_time

fbf3524

martinus deleted the 2021-09-update-nanobench branch September 26, 2021 09:02

ryanofsky mentioned this pull request Sep 27, 2021

refactor: Clarify and disable unused ArgsManager flags #22766

Merged

bitcoin locked and limited conversation to collaborators Oct 30, 2022

bench: update nanobench add -min_time #23025

bench: update nanobench add -min_time #23025

Uh oh!

Conversation

martinus commented Sep 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Sep 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

martinus commented Sep 19, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

martinus commented Sep 20, 2021

Uh oh!

jonatack commented Sep 20, 2021

Uh oh!

Uh oh!

jonatack commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinus commented Sep 21, 2021

Uh oh!

jonatack commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 24, 2021

Uh oh!

Uh oh!

bench: update nanobench add `-min_time` #23025

bench: update nanobench add `-min_time` #23025

martinus commented Sep 18, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading

DrahtBot commented Sep 19, 2021 •

edited

Loading

jonatack commented Sep 21, 2021 •

edited

Loading

jonatack commented Sep 21, 2021 •

edited

Loading