bench: add "priority level" to the benchmark framework #26158

furszy · 2022-09-22T22:54:16Z

This is from today's meeting, a simple "priority level" for the benchmark framework.

Will allow us to run certain benchmarks while skip non-prioritized ones in make check.

By default, bench_bitcoin will run all the benchmarks. make checkwill only run the high priority ones,
and have marked all the existent benchmarks as "high priority" to retain the current behavior.

Could test it by modifying any benchmark priority to something different from "high", and
run bench_bitcoin -priority-level=high and/or bench_bitcoin -priority-level=medium,low
(the first command will skip the modified bench while the second one will include it).

Note: the second commit could be avoided by having a default arg value for the priority
level but.. an explicit set in every BENCHMARK macro call makes it less error-prone.

DrahtBot · 2022-09-23T03:04:45Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25695 (tidy: add modernize-use-using by fanquake)
#21590 (Safegcd-based modular inverses in MuHash3072 by sipa)
#18014 (lib: Optimizing siphash implementation by elichai)
#15294 (refactor: Extract RipeMd160 by Empact)

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

quick code review ACK

src/bench/bench.h

luke-jr · 2022-09-24T23:51:02Z

I think the default should be to run them all?

make check already overrides the number of iterations, and could easily limit it to just high priority.

furszy · 2022-09-25T17:33:52Z

Feedback tackled, thanks both

Changes:

Moved priority level to be a bitfield. So combinations between different levels are allowed.
Now can pass -priority-level=medium,low or -priority-level=all.
Moved default priority level to "all", and adapted make check to only run high priority ones.
Surrounded bench main() with try/catch, mainly to have a cleaner exit in args
parsing errors (e.g. "unknown priority level").

martinus · 2022-09-26T05:52:17Z

I like it, code review ACK f95ce9b

stickies-v

Concept ACK - looks like a good improvement to allow make check not become too slow and still allow flexibility as to which benchmarks we want to add to the test suite.

src/bench/bench.cpp

furszy · 2022-09-26T19:23:39Z

Updated per feedback.

Pairs unpacked, small diff

stickies-v

Went through most of the code now, a few more mostly minor comments.

nit: From a readability pov, I'm a bit uneasy with priority_level sometimes being a uint8_t and sometimes a PriorityLevel. Not very intuitive, but I don't have a clear solution atm. (I do like the bitmask approach).

src/bench/bench.h

src/bench/bench_bitcoin.cpp

src/bench/bench.h

src/bench/bench.cpp

src/bench/bench_bitcoin.cpp

furszy · 2022-09-27T21:34:26Z

Updated per feedback, thanks @stickies-v 👍🏼.

Changes diff:

Renamed ToPriorityLevel to StringToPriority.
Moved PriorityToString runtime_error to assertion.
Adjusted the new flag help message description.
Made parsePriorityLevel use SplitString function.

stickies-v

Code Review ACK 6e95eae

Will do some testing shortly.

src/bench/bench.h

src/bench/bench_bitcoin.cpp

src/bench/bench.cpp

src/bench/bench_bitcoin.cpp

furszy · 2022-09-28T16:31:17Z

Feedback tackled, thanks both.

Changes (diff):

Placed the benchmark implementation inside benchmark namespace.
Added missing cassert include.
Removed static_assert by defining bitwise operators on the enum class.

stickies-v

Did some first testing already, everything seems to work as expected (except for -priority-level=all not working anymore as commented).

I'm not sure we want to do something about it, but currently we can also specify a benchmark as benchmark::PriorityLevel::ALL, in which case it would get run for any -priority-level.

E.g. with

BENCHMARK(AddrManAdd, benchmark::PriorityLevel::LOW);
BENCHMARK(AddrManSelect, benchmark::PriorityLevel::MEDIUM);
BENCHMARK(AddrManGetAddr, benchmark::PriorityLevel::HIGH);
BENCHMARK(AddrManAddThenGood, benchmark::PriorityLevel::ALL);

For ./bench_bitcoin --priority-level=low we get:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       19,047,208.00 |               52.50 |    1.8% |      0.21 | `AddrManAdd`
|       48,141,333.00 |               20.77 |    1.8% |      0.53 | `AddrManAddThenGood`

src/bench/bench.cpp

src/bench/bench_bitcoin.cpp

src/bench/bench.cpp

stickies-v

re-ACK 010f53e

achow101 · 2022-10-19T00:55:01Z

ACK 010f53e

maflcko · 2022-10-19T08:12:52Z

Is there a guideline on how to decide the priority level? I'd presume this is subjective, or is it simply a performance question? That is, slow benchmarks have low priority?

furszy · 2022-10-19T13:59:54Z

Is there a guideline on how to decide the priority level? I'd presume this is subjective, or is it simply a performance question? That is, slow benchmarks have low priority?

Yeah ok. So far, the turning point here seems to be introduction of benchmarks who measure/test how certain flows behave on a large scale (context #25685 (comment)). Which, usually, require a somewhat slow context setup (e.g. fill-up the wallet with 20k utxo to benchmark the tx creation process, coin selection, etc) which goes outside of the scope of the "run all benchs" sanity check that we are currently doing in make check.

maflcko · 2022-10-19T14:11:01Z

Ok, so this is similar to what the functional tests call "extended"? I am asking because right now it looks like this doesn't change the "priority" of any benchmark, and if one is changed we should have at least one CI task that runs all benchmarks, similar to how there is one that runs all functional tests.

furszy · 2022-10-19T14:46:15Z

Ok, so this is similar to what the functional tests call "extended"? I am asking because right now it looks like this doesn't change the "priority" of any benchmark, and if one is changed we should have at least one CI task that runs all benchmarks, similar to how there is one that runs all functional tests.

Yeah, it's similar to the "extended" concept there. The goal is to not only be restricted to have fast and small benchmarks due the make check duration concerns. Some big issues aren't problems at all in small scale.

And yeah, this PR does not change the current behavior in any way. Only introduces the filtering by priority functionality and, by default, makes make check only run the high priority ones. Then #25685 introduces two "low priority" benchmarks.

absolutely, a CI task to run them all would be nice, yeah.

furszy · 2022-10-19T14:53:04Z

Side note, to make the priority level decision process simpler, could also remove the medium level. So we only have two options, high or low priority.

kouloumos · 2022-10-19T15:01:05Z

Side note, to make the priority level decision process simpler, could also remove the medium level. So we only have two options, high or low priority.

I was also thinking about that decision process where someone is deciding the priority level of his new benchmark. How to differentiate between "medium" and 'low"? Do you have any scenarios/thoughts for that?

Instead of 2 priorities, maybe we could go with the "extended" notion that functional tests use?

furszy · 2022-10-19T15:23:40Z

I was also thinking about that decision process where someone is deciding the priority level of his new benchmark. How to differentiate between "medium" and 'low"? Do you have any scenarios/thoughts for that?

I think that it depends on the benchmarked area mostly. E.g. If we are benchmarking a process from the validation code (like the duplicate-inputs benchmark), it should have higher priority than a benchmark that tests the ListTransactions RPC-only function.
Another example could be a bug in the wallet that slows down the block scanning process on big wallets by 10x. Which should have lower priority than the duplicate-inputs bench (as it cannot be run in make check due the slow context setup requirements, because it might need to generate a 100k blocks chain), but higher than the ListTransactions one.

Instead of 2 priorities, maybe we could go with the "extended" notion that functional tests use?

Could go with that terminology as well, yeah. But, I think that, if we define which areas, problems and time constrains are more suited for each priority level, then the different levels approach make more sense.

theStack

Code-review ACK 010f53e ⛰️

Left two small nits below, but feel free to ignore. Happy to also re-review if you decide to change the priorities numbers/naming ("low"/"high" or "normal"/"extended" or whatever).

src/bench/bench.cpp

src/bench/bench_bitcoin.cpp

Will allow us to run certain benchmarks while skip non-prioritized ones in 'make check'.

no-functional changes. Only have set the priority level explicitly on every BENCHMARK macro call.

so we have a cleaner exit on internal runtime errors. e.g. an unknown priority level.

furszy · 2022-10-20T13:31:49Z

Updated per feedback. Thanks everyone :).

Have removed the medium priority level for now, mainly to minimize future doubts/discussions around it. It could easily be re-added in the future (I left a blank space for it) when more benchmarks like the ones described in #26158 (comment) are introduced.

Tiny diff.

Ready to go.

theStack

re-ACK 3e9d0be

stickies-v

re-ACK 3e9d0be

kouloumos · 2022-10-20T14:21:36Z

re-ACK 3e9d0be

achow101 · 2022-10-20T15:00:07Z

ACK 3e9d0be

…mework

hebasto · 2022-11-10T16:46:41Z

src/Makefile.test.include

-	@echo "Running bench/bench_bitcoin (one iteration sanity check)..."
-	$(BENCH_BINARY) --sanity-check > /dev/null
+	@echo "Running bench/bench_bitcoin (one iteration sanity check, only high priority)..."
+	$(BENCH_BINARY) -sanity-check -priority-level=high > /dev/null


For the "Win64 native [vs2022]" CI task the same -priority-level=high has been added in #26481.

DrahtBot added the Tests label Sep 22, 2022

Mshafaaa approved these changes Sep 23, 2022

View reviewed changes

This was referenced Sep 23, 2022

tidy: add modernize-use-using #25695

Closed

index: Remove unused BlockFilter::Unserialize() #25637

Closed

BIP324: Cipher suite #25361

Closed

BIP324: Handshake prerequisites #23561

Closed

BIP324: Add encrypted p2p transport {de}serializer #23233

Closed

martinus reviewed Sep 24, 2022

View reviewed changes

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

furszy force-pushed the 2022_bench_priority_level branch from 0106da0 to f95ce9b Compare September 25, 2022 15:30

stickies-v reviewed Sep 26, 2022

View reviewed changes

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

furszy force-pushed the 2022_bench_priority_level branch 2 times, most recently from f01077a to 23b6a7a Compare September 26, 2022 19:20

stickies-v reviewed Sep 27, 2022

View reviewed changes

furszy force-pushed the 2022_bench_priority_level branch 2 times, most recently from 4cfc82b to 6e95eae Compare September 27, 2022 21:27

stickies-v reviewed Sep 28, 2022

View reviewed changes

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

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

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

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

maflcko reviewed Sep 28, 2022

View reviewed changes

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

furszy force-pushed the 2022_bench_priority_level branch from 6e95eae to 19ac657 Compare September 28, 2022 16:20

bench: place benchmark implementation inside benchmark namespace

f159378

furszy force-pushed the 2022_bench_priority_level branch from 19ac657 to 9c3ab93 Compare September 28, 2022 16:28

stickies-v reviewed Sep 29, 2022

View reviewed changes

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

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

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

furszy force-pushed the 2022_bench_priority_level branch 2 times, most recently from a0c486a to cbc077e Compare September 29, 2022 17:03

stickies-v reviewed Oct 18, 2022

View reviewed changes

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

furszy force-pushed the 2022_bench_priority_level branch from 06664b8 to 010f53e Compare October 18, 2022 14:53

stickies-v approved these changes Oct 18, 2022

View reviewed changes

theStack approved these changes Oct 20, 2022

View reviewed changes

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

src/bench/bench_bitcoin.cpp Show resolved Hide resolved

furszy added 4 commits October 20, 2022 10:21

bench: add "priority level" to the benchmark framework

05b8c76

Will allow us to run certain benchmarks while skip non-prioritized ones in 'make check'.

bench: explicitly make all current benchmarks "high" priority

3da7cd2

no-functional changes. Only have set the priority level explicitly on every BENCHMARK macro call.

bench: surround main() execution with try/catch

466b54b

so we have a cleaner exit on internal runtime errors. e.g. an unknown priority level.

build: only run high priority benchmarks in 'make check'

3e9d0be

furszy force-pushed the 2022_bench_priority_level branch from 010f53e to 3e9d0be Compare October 20, 2022 13:23

theStack approved these changes Oct 20, 2022

View reviewed changes

stickies-v approved these changes Oct 20, 2022

View reviewed changes

achow101 merged commit fabc031 into bitcoin:master Oct 20, 2022

furszy mentioned this pull request Oct 20, 2022

wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection #25685

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 23, 2022

Merge bitcoin#26158: bench: add "priority level" to the benchmark fra…

11e9552

…mework

pablomartin4btc mentioned this pull request Nov 8, 2022

bench: add benchmark for wallet 'AvailableCoins' function. #25234

Merged

hebasto reviewed Nov 10, 2022

View reviewed changes

hebasto mentioned this pull request Dec 23, 2022

bench: Suppress output when running with -sanity-check option #26481

Merged

furszy deleted the 2022_bench_priority_level branch May 27, 2023 01:47

bitcoin locked and limited conversation to collaborators May 26, 2024

bench: add "priority level" to the benchmark framework #26158

bench: add "priority level" to the benchmark framework #26158

Uh oh!

Conversation

furszy commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

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

luke-jr commented Sep 24, 2022

Uh oh!

furszy commented Sep 25, 2022

Uh oh!

martinus commented Sep 26, 2022

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Sep 26, 2022

Uh oh!

stickies-v left a comment

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!

furszy commented Sep 27, 2022

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Sep 28, 2022

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 19, 2022

Uh oh!

furszy commented Oct 19, 2022

Uh oh!

maflcko commented Oct 19, 2022

Uh oh!

furszy commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Oct 19, 2022

Uh oh!

kouloumos commented Oct 19, 2022

Uh oh!

furszy commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

furszy commented Sep 22, 2022 •

edited

Loading

DrahtBot commented Sep 23, 2022 •

edited

Loading

martinus left a comment •

edited

Loading

achow101 commented Oct 19, 2022 •

edited

Loading

furszy commented Oct 19, 2022 •

edited

Loading

furszy commented Oct 19, 2022 •

edited

Loading

furszy commented Oct 20, 2022 •

edited

Loading