bench: add "priority level" to the benchmark framework

furszy commented at 10:54 pm on September 22, 2022: member

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 added the label Tests on Sep 22, 2022

Mshafaaa approved

DrahtBot commented at 3:04 am on September 23, 2022: contributor

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.

in src/bench/bench.h:44 in 8d795da8d7 outdated

40@@ -41,6 +41,16 @@ using ankerl::nanobench::Bench;
41 
42 typedef std::function<void(Bench&)> BenchFunction;
43 
44+enum BenchPriorityLevel

martinus commented at 6:15 am on September 24, 2022:

I’d use enum class

martinus commented at 6:24 am on September 24, 2022:

think it would be nice if the enum would be some kind of bitmask instead. Then the entries act more like tags, and it would be possible to e.g. run all high and medium benchmarks by specifying both.

furszy commented at 3:31 pm on September 25, 2022:

yeah, much better. pushed it.

martinus commented at 6:26 am on September 24, 2022: contributor

quick code review ACK

luke-jr commented at 11:51 pm on September 24, 2022: member

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 force-pushed on Sep 25, 2022

furszy commented at 5:33 pm on September 25, 2022: member

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 commented at 5:52 am on September 26, 2022: contributor

I like it, code review ACK f95ce9b

in src/bench/bench.cpp:89 in 74ac7142d7 outdated

84@@ -63,6 +85,11 @@ void benchmark::BenchRunner::RunAll(const Args& args)
85 
86     std::vector<ankerl::nanobench::Result> benchmarkResults;
87     for (const auto& p : benchmarks()) {
88+
89+        if (!(p.second.second & args.priority_level)) {

stickies-v commented at 5:21 pm on September 26, 2022:

Will expand the diff a bit, but wdyt about using structured bindings for iterating over benchmarks() and unpacking the second pair (as you can’t do nested structured bindings)? p.second.second could benefit from some improved readability.

furszy commented at 7:11 pm on September 26, 2022:

sure, give me a second.

stickies-v commented at 5:57 pm on September 26, 2022: contributor

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.

furszy force-pushed on Sep 26, 2022

furszy commented at 7:23 pm on September 26, 2022: member

Updated per feedback.

Pairs unpacked, small diff

in src/bench/bench.h:53 in 23b6a7ab5b outdated

48+    HIGH = 1 << 2,
49+    ALL = HIGH | MEDIUM | LOW,
50+};
51+
52+/** Priority level default value */
53+static const PriorityLevel DEFAULT_PRIORITY_LEVEL = PriorityLevel::ALL;

stickies-v commented at 3:27 pm on September 27, 2022:

0static const PriorityLevel DEFAULT_PRIORITY_LEVEL{PriorityLevel::ALL};

in src/bench/bench_bitcoin.cpp:52 in 23b6a7ab5b outdated

47@@ -45,6 +48,17 @@ static std::vector<double> parseAsymptote(const std::string& str) {
48     return numbers;
49 }
50 
51+static uint8_t parsePriorityLevel(const std::string& str) {
52+    std::stringstream ss(str);
53+    uint8_t levels = 0;

stickies-v commented at 3:39 pm on September 27, 2022:

0    std::stringstream ss{str};
1    uint8_t levels{0};

in src/bench/bench_bitcoin.cpp:33 in 23b6a7ab5b outdated

29@@ -30,6 +30,9 @@ static void SetupBenchArgs(ArgsManager& argsman)
30     argsman.AddArg("-output-csv=<output.csv>", "Generate CSV file with the most important benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
31     argsman.AddArg("-output-json=<output.json>", "Generate JSON file with all benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
32     argsman.AddArg("-sanity-check", "Run benchmarks for only one iteration", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
33+    argsman.AddArg("-priority-level=<l1,l2,l3>", strprintf("Run benchmarks of a certain priority level (%s | %s | %s), default: %s", PriorityToString(benchmark::PriorityLevel::LOW),

stickies-v commented at 4:20 pm on September 27, 2022:

0    argsman.AddArg("-priority-level=<l1,l2,l3>", strprintf("Run benchmarks of one or multiple priority level(s) (%s | %s | %s), default: %s", PriorityToString(benchmark::PriorityLevel::LOW),

stickies-v commented at 4:21 pm on September 27, 2022:

Given the low amount of options and option names, perhaps better to replace <l1,l2,l3> with the actual priority level names?

furszy commented at 9:07 pm on September 27, 2022:

I would try to not pollute the flag name with specific parameters. So that it does not depend on the format function output (same behavior as every other flag here and init.cpp).

furszy commented at 9:25 pm on September 27, 2022:

Done

stickies-v commented at 10:46 am on September 28, 2022:

okay, fair enough - consider this resolved, thanks!

in src/bench/bench_bitcoin.cpp:57 in 23b6a7ab5b outdated

54+    while (ss.good()) {
55+        std::string level;
56+        std::getline(ss, level, ',');
57+        levels |= benchmark::ToPriorityLevel(level);
58+    }
59+    return levels;

stickies-v commented at 4:23 pm on September 27, 2022:

Would require #include <util/string.h>, but I feel like SplitString is better suited here?

0    uint8_t levels = 0;
1    for (const auto& level: SplitString(str, ',')) {
2        levels |= benchmark::ToPriorityLevel(level);
3    }
4    return levels;

furszy commented at 9:17 pm on September 27, 2022:

Sure. Same function can be used inside parseAsymptote.

in src/bench/bench.h:65 in 23b6a7ab5b outdated

51+
52+/** Priority level default value */
53+static const PriorityLevel DEFAULT_PRIORITY_LEVEL = PriorityLevel::ALL;
54+
55+uint8_t ToPriorityLevel(const std::string& str);
56+std::string PriorityToString(PriorityLevel level);

stickies-v commented at 4:42 pm on September 27, 2022:

I find this interface a bit inconsistent:

ToPriorityLevel I would expect to return a PriorityLevel, not a uint8_t. I’d do the static cast in parsePriorityLevel instead of here
StringToPriority instead of ToPriorityLevel would be a more logical reverse operation name to PriorityToString imo

furszy commented at 9:07 pm on September 27, 2022:

sure, sounds good

in src/bench/bench.cpp:61 in 23b6a7ab5b outdated

56+}
57+
58+std::string benchmark::PriorityToString(PriorityLevel level)
59+{
60+    for (const auto& it : map_priority_level) {
61+        if (it.second == level) return it.first;

stickies-v commented at 4:44 pm on September 27, 2022:

Also here for readability it’s best to use structured bindings I think?

in src/bench/bench.cpp:63 in 23b6a7ab5b outdated

58+std::string benchmark::PriorityToString(PriorityLevel level)
59+{
60+    for (const auto& it : map_priority_level) {
61+        if (it.second == level) return it.first;
62+    }
63+    throw std::runtime_error(strprintf("Unknown priority level %d", level));

stickies-v commented at 5:07 pm on September 27, 2022:

I’m not sure why this compiles because I think you can’t pass a PriorityLevel like that:

0    throw std::runtime_error(strprintf("Unknown priority level %u", static_cast<uint8_t>(level)));

Also, I don’t think this is a runtime error because it can only be hit when map_priority_level is out of sync with PriorityLevel?

furszy commented at 9:15 pm on September 27, 2022:

Also, I don’t think this is a runtime error because it can only be hit when map_priority_level is out of sync with PriorityLevel?

Yeah, but I don’t think that we have a good way to assert with a message.

in src/bench/bench_bitcoin.cpp:35 in 23b6a7ab5b outdated

29@@ -30,6 +30,9 @@ static void SetupBenchArgs(ArgsManager& argsman)
30     argsman.AddArg("-output-csv=<output.csv>", "Generate CSV file with the most important benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
31     argsman.AddArg("-output-json=<output.json>", "Generate JSON file with all benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
32     argsman.AddArg("-sanity-check", "Run benchmarks for only one iteration", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
33+    argsman.AddArg("-priority-level=<l1,l2,l3>", strprintf("Run benchmarks of a certain priority level (%s | %s | %s), default: %s", PriorityToString(benchmark::PriorityLevel::LOW),
34+                                                           PriorityToString(benchmark::PriorityLevel::MEDIUM), PriorityToString(benchmark::PriorityLevel::HIGH),
35+                                                           PriorityToString(benchmark::PriorityLevel::ALL)), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

stickies-v commented at 5:09 pm on September 27, 2022:

0                                                           PriorityToString(benchmark::DEFAULT_PRIORITY_LEVEL)), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

stickies-v commented at 5:25 pm on September 27, 2022: contributor

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).

furszy force-pushed on Sep 27, 2022

furszy commented at 9:34 pm on September 27, 2022: member

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.

in src/bench/bench.h:55 in 6e95eaede1 outdated

50+};
51+
52+/** Priority level default value */
53+static const PriorityLevel DEFAULT_PRIORITY_LEVEL{PriorityLevel::ALL};
54+
55+benchmark::PriorityLevel StringToPriority(const std::string& str);

stickies-v commented at 10:46 am on September 28, 2022:

nit: we’re in the benchmark namespace already

0PriorityLevel StringToPriority(const std::string& str);

in src/bench/bench.cpp:63 in 6e95eaede1 outdated

58+std::string benchmark::PriorityToString(PriorityLevel in_level)
59+{
60+    for (const auto& [name, level] : map_priority_level) {
61+        if (level == in_level) return name;
62+    }
63+    assert(false); // out-of-sync `map_priority_level`

stickies-v commented at 11:05 am on September 28, 2022:

requires #include <cassert>

You could also use assert(!"<message>") as in: https://github.com/bitcoin/bitcoin/blob/9fcdb9f3a044330d3d7515fa35709102c98534d2/src/net_processing.cpp#L2145

in src/bench/bench.cpp:58 in 6e95eaede1 outdated

53+    auto it = map_priority_level.find(str);
54+    if (it == map_priority_level.end()) throw std::runtime_error(strprintf("Unknown priority level %s", str));
55+    return it->second;
56+}
57+
58+std::string benchmark::PriorityToString(PriorityLevel in_level)

stickies-v commented at 11:39 am on September 28, 2022:

Currently, there’s a bit of inconsistency in the namespace specification (benchmark::PriorityLevel in other places). What do you think about putting all the benchmark implementations here in namespace benchmark and remove all the benchmark:: specifiers? This makes the code more concise and explicit, and mirrors the header file. (thank you @hebasto for the idea).

furszy commented at 4:00 pm on September 28, 2022:

sure, I thought about it as well but didn’t want to expand this further. Now that it’s not just me, will add a small commit for it at the beginning :).

stickies-v commented at 11:46 am on September 28, 2022: contributor

Code Review ACK 6e95eaede

Will do some testing shortly.

in src/bench/bench_bitcoin.cpp:54 in 6e95eaede1 outdated

47@@ -45,6 +48,14 @@ static std::vector<double> parseAsymptote(const std::string& str) {
48     return numbers;
49 }
50 
51+static uint8_t parsePriorityLevel(const std::string& str) {
52+    uint8_t levels = 0;
53+    for (const auto& level: SplitString(str, ',')) {
54+        levels |= static_cast<uint8_t>(benchmark::StringToPriority(level));

maflcko commented at 1:03 pm on September 28, 2022:

in theory it would be possible to define the operator on the enum class. See for example

0static inline constexpr NetPermissionFlags operator|(NetPermissionFlags a, NetPermissionFlags b)

furszy commented at 3:58 pm on September 28, 2022:

That is interesting. Wasn’t expecting to be able to store not declared enum combinations inside the enum variable. So.. same as happens with the NetPermissionFlags, could remove every static_cast and do something like:

0PriorityLevel levels{0};
1for (const auto& level: SplitString(str, ',')) {
2   levels = levels | benchmark::StringToPriority(level);
3}
4return levels;

Which.. can make the software crash if I pass a combination of tags that isn’t declared (e.g. “medium, low”) and call PriorityToString(levels) (at the assertion point).

furszy force-pushed on Sep 28, 2022

bench: place benchmark implementation inside benchmark namespace f1593780b8

furszy force-pushed on Sep 28, 2022

furszy commented at 4:31 pm on September 28, 2022: member

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.

in src/bench/bench.cpp:68 in 9c3ab931fb outdated

63+    std::string tags;
64+    for (const auto& [name, level] : map_priority_level) {
65+        if (level & in_level) tags += name + ",";
66+    }
67+    tags.pop_back(); // remove the last comma
68+    assert(!tags.empty()); // out-of-sync `map_priority_level`

stickies-v commented at 4:48 pm on September 28, 2022:

I’d switch these up - doesn’t seem to alter behaviour and prevents UB when no matches are found and tags is empty

0    assert(!tags.empty()); // out-of-sync `map_priority_level`
1    tags.pop_back(); // remove the last comma

furszy commented at 4:43 pm on September 29, 2022:

yeah good

in src/bench/bench_bitcoin.cpp:35 in 9c3ab931fb outdated

29@@ -30,6 +30,9 @@ static void SetupBenchArgs(ArgsManager& argsman)
30     argsman.AddArg("-output-csv=<output.csv>", "Generate CSV file with the most important benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
31     argsman.AddArg("-output-json=<output.json>", "Generate JSON file with all benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
32     argsman.AddArg("-sanity-check", "Run benchmarks for only one iteration", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
33+    argsman.AddArg("-priority-level=<l1,l2,l3>", strprintf("Run benchmarks of one or multiple priority level(s) (%s | %s | %s), default: '%s' (all)", PriorityToString(benchmark::PriorityLevel::LOW),
34+                                                           PriorityToString(benchmark::PriorityLevel::MEDIUM), PriorityToString(benchmark::PriorityLevel::HIGH),
35+                                                           PriorityToString(benchmark::DEFAULT_PRIORITY_LEVEL)), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

stickies-v commented at 4:56 pm on September 28, 2022:

I think the default level is now specified twice, once from PriorityToString(benchmark::DEFAULT_PRIORITY_LEVEL)) and once hardcoded here as “all”

./src/bench/bench_bitcoin –help

0  -priority-level=<l1,l2,l3>
1       Run benchmarks of one or multiple priority level(s) (low | medium |
2       high), default: 'high,low,medium' (all)

in src/bench/bench.cpp:47 in 9c3ab931fb outdated

41@@ -41,18 +42,44 @@ void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& bench
42 
43 } // namespace
44 
45-benchmark::BenchRunner::BenchmarkMap& benchmark::BenchRunner::benchmarks()
46+namespace benchmark {
47+
48+std::map<std::string, PriorityLevel> map_priority_level = {

stickies-v commented at 3:16 pm on September 29, 2022:

Since the previous force push, we now can’t pass -priority-level=all anymore, I don’t think that’s intentional? And if it was, I don’t think I agree with that -priority-level=low,medium,high is cumbersome and will silently be incomplete when adding new levels.

furszy commented at 5:05 pm on September 29, 2022:

Since the previous force push, we now can’t pass -priority-level=all anymore, I don’t think that’s intentional?

Yes, to avoid tags clashing problems. e.g. when you call PriorityToString with any value, the returned string was containing the actual priority and, as it matches with the rest of the tags, the “all” tag.

But.. will re-add the “all” shortcut differently now.

And you know, I actually forgot to go further here, should had removed the PriorityLevel::ALL and replace it by a plain 0xff. We shouldn’t allow benchmarks to be set under the PriorityLevel::ALL tag as you did above (the BENCHMARK(AddrManAddThenGood, benchmark::PriorityLevel::ALL)).

And if it was, I don’t think I agree with that -priority-level=low,medium,high is cumbersome and will silently be incomplete when adding new levels.

With the latest change (0xff inclusion), all levels will be automatically added there.

stickies-v commented at 3:22 pm on September 29, 2022: contributor

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

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

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

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

furszy force-pushed on Sep 29, 2022

in src/bench/bench.cpp:57 in cbc077e77a outdated

53+
54+PriorityLevel StringToPriority(const std::string& str)
55+{
56+    if (str == "all") return (PriorityLevel)0xff; // "all" shortcut
57+    auto it = map_priority_level.find(str);
58+    if (it == map_priority_level.end()) throw std::runtime_error(strprintf("Unknown priority level %s", str));

amovfx commented at 4:41 pm on October 6, 2022:

Is there a valid concern that the string could be entered in as multicase? Like Low, Medium, High, All? Maybe this would be better suited in parsePriorityLevel?

0    std::transform(str.begin(), str.end(), str.begin(), ::tolower);
1    if (str == "all") return (PriorityLevel)0xff; // "all" shortcut
2    auto it = map_priority_level.find(str);
3    if (it == map_priority_level.end()) throw std::runtime_error(strprintf("Unknown priority level %s", str));

furszy commented at 9:23 pm on October 6, 2022:

Something like that would be better suited for the general ArgsManager::ParseParameters function. Not only for this new flag.

But I’m not so sure about it. Init flags are keys, and keys shouldn’t have different values. Should be a 1-1 map.

amovfx commented at 6:49 pm on October 7, 2022:

Makes sense, I was thinking the same thing last night. ArgsManager should perhapse enforce case if that is a good idea. Thanks.

amovfx commented at 4:51 pm on October 6, 2022: none

Reviewed for pr review club. I was surprised to see all the benchmarks that were updated got benchmark::PriorityLevel::HIGH.

furszy commented at 9:28 pm on October 6, 2022: member

Reviewed for pr review club. I was surprised to see all the benchmarks that were updated got benchmark::PriorityLevel::HIGH.

Why? It’s stated in the PR description:

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

The goal is to not introduce a behavior change here, only add the new functionality. Then discuss specific cases on parallel independent PRs.

amovfx commented at 5:48 pm on October 7, 2022: none

@furszy Sorry, I need to read better. Thanks for taking the time to clarify.

in src/bench/bench.h:75 in afc9c1bb54 outdated

71@@ -49,22 +72,24 @@ struct Args {
72     fs::path output_csv;
73     fs::path output_json;
74     std::string regex_filter;
75+    PriorityLevel priority_level{DEFAULT_PRIORITY_LEVEL};

kouloumos commented at 2:30 pm on October 12, 2022:

As the default value is also assigned when parsing the args, isn’t this an extra initialization? Also, if this is removed, DEFAULT_PRIORITY_LEVEL can be moved to bench_bitcoin.cpp together with the rest of the default values. (afc9c1bb544dfc5418102dd2e5f9f2819b24b885)

furszy commented at 6:55 pm on October 14, 2022:

done

hernanmarino commented at 3:24 pm on October 12, 2022: contributor

code review ACK . Thoroughly tested changing priorities, everything works fine. I like the approach of the bit mapping for priorities

pablomartin4btc commented at 10:26 pm on October 12, 2022: member

Tested ACK,

Compiled this pr, changed a benchmark priority and recompiled only the bitcoin_bench (make -C src bitcoin_bench) playing with the different priority levels (low, medium, high, all, none) and some combinations, it works as expected.

All existing benchmarks are set with “high priority” to match the current behaviour where all benchmarks run by default on “make check”, so there’s no impact merging the change.

I understand @stickies-v’s point of view about the inconsistecy between the priority level enum and the bitvector, in terms of their original purpose, as the enum specifies items from a limited set, the uint8_t mask/ mapping would represent the combination/ agreggation of all of them. I don’t have a strong position on how to unlink the uint8_t use case as on the other side it looks a very good approach.

in src/bench/bench_bitcoin.cpp:35 in cbc077e77a outdated

29@@ -30,6 +30,9 @@ static void SetupBenchArgs(ArgsManager& argsman)
30     argsman.AddArg("-output-csv=<output.csv>", "Generate CSV file with the most important benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
31     argsman.AddArg("-output-json=<output.json>", "Generate JSON file with all benchmark results", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
32     argsman.AddArg("-sanity-check", "Run benchmarks for only one iteration", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
33+    argsman.AddArg("-priority-level=<l1,l2,l3>", strprintf("Run benchmarks of one or multiple priority level(s) (%s | %s | %s), default: '%s'", PriorityToString(benchmark::PriorityLevel::LOW),
34+                                                           PriorityToString(benchmark::PriorityLevel::MEDIUM), PriorityToString(benchmark::PriorityLevel::HIGH),
35+                                                           PriorityToString(benchmark::DEFAULT_PRIORITY_LEVEL)), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

kouloumos commented at 12:50 pm on October 13, 2022:

I am not sure that I see the benefit of the PriorityToString function, I believe that it adds extra circular transformation complexity (priority -> string -> priority) without a solid benefit.

In case of a change to priorities (adding/removing options), we will still need to make a change here. Maybe a function that uses map_priority_level to concatenate all available options could be a better fit. With this changed what’s left for PriorityToString is its usage with DEFAULT_PRIORITY_LEVEL which can be erased by using string instead of priority:DEFAULT_PRIORITY_LEVEL{"all"}. (afc9c1bb544dfc5418102dd2e5f9f2819b24b885)

furszy commented at 6:55 pm on October 14, 2022:

sounds good, pushed inside 085e88ef.

kouloumos commented at 12:55 pm on October 13, 2022: contributor

ACK cbc077e77a5d0ba8ae11b1f5d89480c997fdef55 minus my comments.

Do we have a heuristic on the expected behavior when passing an arg without options? e.g bench_bitcoin -priority-level? Although I don’t consider it an issue, I observed that there are inconsistencies (which can be traced back to how each arg is parsing the empty string ""):

-asymptote, -min-time, -output-csv, -output-json: behave like the arg was never given.
filter: filters with the empty string “”, therefore no benchmarks are executed.
-priority-level: Errors with unknown priority level

furszy commented at 3:27 pm on October 14, 2022: member

Do we have a heuristic on the expected behavior when passing an arg without options? e.g bench_bitcoin -priority-level? Although I don’t consider it an issue, I observed that there are inconsistencies (which can be traced back to how each arg is parsing the empty string ""):

-asymptote, -min-time, -output-csv, -output-json: behave like the arg was never given.

filter: filters with the empty string “”, therefore no benchmarks are executed.

-priority-level: Errors with unknown priority level

Unlike the other args, the -priority-level inputted string need to match against one of the existent levels.

The filter arg behavior make sense because it’s a pattern matching string. e.g. if you provide an empty string, then nothing will match against it. So no benchmarks will be executed.

For the others, probably it’s not a bad idea to return an error (or print to console) when an empty flag is passed. So the user knows that the arg is wrong.

furszy force-pushed on Oct 14, 2022

furszy commented at 6:56 pm on October 14, 2022: member

Updated per @kouloumos feedback.

Have removed PriorityToString(), and replaced it by ListPriorities().

in src/bench/bench_bitcoin.cpp:22 in c2f53f9874 outdated

17@@ -18,6 +18,8 @@
18 
19 static const char* DEFAULT_BENCH_FILTER = ".*";
20 static constexpr int64_t DEFAULT_MIN_TIME_MS{10};
21+/** Priority level default value, run "all" priority levels */
22+static const std::string DEFAULT_PRIORITY_LEVEL{"all"};

stickies-v commented at 5:30 pm on October 17, 2022:

Not a huge concern, but I’m not terribly excited about this string reference. Would be nicer imo to have the default be a uint8_t so it can’t go out of sync, but that would require the PriorityToString fn again which may be overkill.

furszy commented at 0:02 am on October 18, 2022:

yeah. Same feeling here. But.. would say to leave it as is for now, otherwise it would mean to re-introduce the PriorityToString function only to stringify this priority level.

stickies-v commented at 1:12 pm on October 18, 2022:

nit: I think priority better highlights the difference between the aggregated priority and the individual priority level

0static const std::string DEFAULT_PRIORITY{"all"};

in src/bench/bench.cpp:60 in c2f53f9874 outdated

57+    for (const auto& [name, level] : map_priority_level) {
58+        tags += name + ",";
59+    }
60+    tags.pop_back(); // remove the last comma
61+    return tags;
62+}

stickies-v commented at 5:31 pm on October 17, 2022:

With #include <util/string.h>:

0{
1    return Join(map_priority, ',', [](const auto& entry){  return entry.first; });
2}

stickies-v commented at 5:32 pm on October 17, 2022:

Not a huge fan of how this sorts alphabetically by label instead of by increasing priority, but… not a very straightforward way around that it seems. Just raising in case you have ideas.

furszy commented at 0:19 am on October 18, 2022:

Not a huge fan of how this sorts alphabetically by label instead of by increasing priority, but… not a very straightforward way around that it seems. Just raising in case you have ideas.

Could add a custom comparator. One sec.

stickies-v commented at 5:48 pm on October 17, 2022: contributor

As discussed before, I’m not a huge fan of using PriorityLevel both as an enum (corresponding with a single, enumerated flag to indicate the level of each benchmark) and the bitvector representing a (potential) aggregation of multiple levels. I think this makes the code less clear, and I’d prefer an enum to be used as an actual enum instead of some hybrid form.

What do you think about this diff?

use a uint8_t for the bitvector
use an enum instead of enum class so we can implicitly convert to its underlying type. I know this contradicts an earlier comment but I think it makes sense here, and the code is also significantly different from the one where the comment was made
no need for a manual if "all" check, generalizes a bit better

In my opinion it makes the intent of the code more clear, and also reduces the LoC required for this PR.

Note: I think after this diff, some variables should be renamed to drop “level” in their name, but I did not do that here to keep the diff clear.

 0diff --git a/src/bench/bench.cpp b/src/bench/bench.cpp
 1index d2749be1c..37785759e 100644
 2--- a/src/bench/bench.cpp
 3+++ b/src/bench/bench.cpp
 4@@ -45,10 +45,11 @@ void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& bench
 5 
 6 namespace benchmark {
 7 
 8-std::map<std::string, PriorityLevel> map_priority_level = {
 9+std::map<std::string, uint8_t> map_priority_level = {
10     std::make_pair("high", PriorityLevel::HIGH),
11     std::make_pair("medium", PriorityLevel::MEDIUM),
12-    std::make_pair("low", PriorityLevel::LOW)
13+    std::make_pair("low", PriorityLevel::LOW),
14+    std::make_pair("all", 0xff)
15 };
16 
17 std::string ListPriorities()
18@@ -56,9 +57,8 @@ std::string ListPriorities()
19     return Join(map_priority_level, ',', [](const auto& entry){  return entry.first; });
20 }
21 
22-PriorityLevel StringToPriority(const std::string& str)
23+uint8_t StringToPriority(const std::string& str)
24 {
25-    if (str == "all") return (PriorityLevel)0xff; // "all" shortcut
26     auto it = map_priority_level.find(str);
27     if (it == map_priority_level.end()) throw std::runtime_error(strprintf("Unknown priority level %s", str));
28     return it->second;
29diff --git a/src/bench/bench.h b/src/bench/bench.h
30index 295d20bf0..08c682d97 100644
31--- a/src/bench/bench.h
32+++ b/src/bench/bench.h
33@@ -41,25 +41,15 @@ using ankerl::nanobench::Bench;
34 
35 typedef std::function<void(Bench&)> BenchFunction;
36 
37-enum class PriorityLevel : uint8_t
38+enum PriorityLevel : uint8_t
39 {
40     LOW = 1 << 0,
41     MEDIUM = 1 << 1,
42     HIGH = 1 << 2,
43 };
44-static inline constexpr PriorityLevel operator|(PriorityLevel a, PriorityLevel b)
45-{
46-    using t = typename std::underlying_type<PriorityLevel>::type;
47-    return static_cast<PriorityLevel>(static_cast<t>(a) | static_cast<t>(b));
48-}
49-static inline constexpr bool operator&(PriorityLevel tags, PriorityLevel tag)
50-{
51-    using t = typename std::underlying_type<PriorityLevel>::type;
52-    return static_cast<t>(tags) & static_cast<t>(tag);
53-}
54 
55 std::string ListPriorities();
56-PriorityLevel StringToPriority(const std::string& str);
57+uint8_t StringToPriority(const std::string& str);
58 
59 struct Args {
60     bool is_list_only;
61@@ -69,7 +59,7 @@ struct Args {
62     fs::path output_csv;
63     fs::path output_json;
64     std::string regex_filter;
65-    PriorityLevel priority_level;
66+    uint8_t priority_level;
67 };
68 
69 class BenchRunner
70diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp
71index 54d5d67fa..2a1376aa9 100644
72--- a/src/bench/bench_bitcoin.cpp
73+++ b/src/bench/bench_bitcoin.cpp
74@@ -49,8 +49,8 @@ static std::vector<double> parseAsymptote(const std::string& str) {
75     return numbers;
76 }
77 
78-static benchmark::PriorityLevel parsePriorityLevel(const std::string& str) {
79-    benchmark::PriorityLevel levels{0};
80+static uint8_t parsePriorityLevel(const std::string& str) {
81+    uint8_t levels{0};
82     for (const auto& level: SplitString(str, ',')) {
83         levels = levels | benchmark::StringToPriority(level);
84     }

furszy force-pushed on Oct 17, 2022

furszy force-pushed on Oct 18, 2022

furszy commented at 0:29 am on October 18, 2022: member

As discussed before, I’m not a huge fan of using PriorityLevel both as an enum (corresponding with a single, enumerated flag to indicate the level of each benchmark) and the bitvector representing a (potential) aggregation of multiple levels. I think this makes the code less clear, and I’d prefer an enum to be used as an actual enum instead of some hybrid form.

Yeah, I wasn’t a huge fan of it neither. Have applied all the suggestions, thanks @stickies-v :).

Plus, as said in #26158 (review), added a custom comparator to order the ListPriorities() outcome (squashed inside 3a09b15).

in src/bench/bench_bitcoin.cpp:55 in bacb1aef9c outdated

48@@ -45,6 +49,14 @@ static std::vector<double> parseAsymptote(const std::string& str) {
49     return numbers;
50 }
51 
52+static uint8_t parsePriorityLevel(const std::string& str) {
53+    uint8_t levels{0};
54+    for (const auto& level: SplitString(str, ',')) {
55+        levels = levels | benchmark::StringToPriority(level);

stickies-v commented at 11:20 am on October 18, 2022:

nit: shorthand

0        levels |= benchmark::StringToPriority(level);

in src/bench/bench.cpp:47 in bacb1aef9c outdated

41@@ -41,18 +42,47 @@ void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& bench
42 
43 } // namespace
44 
45-benchmark::BenchRunner::BenchmarkMap& benchmark::BenchRunner::benchmarks()
46+namespace benchmark {
47+
48+std::map<std::string, uint8_t> map_priority_level = {

stickies-v commented at 11:22 am on October 18, 2022:

nit: Could add a brief docstring to highlight that this can represent multiple levels

0// map a label to one or multiple priority levels
1std::map<std::string, uint8_t> map_priority_level = {

stickies-v commented at 1:12 pm on October 18, 2022:

nit: I think priority better highlights the difference between the aggregated priority and the individual priority level

0std::map<std::string, uint8_t> map_label_priority = {

in src/bench/bench.cpp:63 in bacb1aef9c outdated

60+};
61+
62+std::string ListPriorities()
63 {
64-    static std::map<std::string, BenchFunction> benchmarks_map;
65+    std::set<std::pair<std::string, uint8_t>, CompPriorities> vec(map_priority_level.begin(), map_priority_level.end());

stickies-v commented at 1:02 pm on October 18, 2022:

Since we’re only using it in this function, I think I’d prefer a local lambda over a struct. Easier to read imo:

 0diff --git a/src/bench/bench.cpp b/src/bench/bench.cpp
 1index 13d3c203d..b9a8e85c4 100644
 2--- a/src/bench/bench.cpp
 3+++ b/src/bench/bench.cpp
 4@@ -51,17 +51,12 @@ std::map<std::string, uint8_t> map_priority_level = {
 5     std::make_pair("all", 0xff)
 6 };
 7 
 8-struct CompPriorities {
 9-    template <typename T>
10-    bool operator()(const T& l, const T& r) const {
11-        return l.second < r.second;
12-    }
13-};
14-
15 std::string ListPriorities()
16 {
17-    std::set<std::pair<std::string, uint8_t>, CompPriorities> vec(map_priority_level.begin(), map_priority_level.end());
18-    return Join(vec, ',', [](const auto& entry){ return entry.first; });
19+    using item_t = std::pair<std::string, uint8_t>;
20+    auto sort_by_priority = [](item_t a, item_t b){ return a.second < b.second; };
21+    std::set<item_t, decltype(sort_by_priority)> sorted_priorities(map_priority_level.begin(), map_priority_level.end(), sort_by_priority);
22+    return Join(sorted_priorities, ',', [](const auto& entry){ return entry.first; });
23 }
24 
25 uint8_t StringToPriority(const std::string& str)

If not, I think naming could be improved, e.g. sorted_priorities instead of vec (it’s not a vector) and SortByPriority instead of CompPriorities?

furszy commented at 2:14 pm on October 18, 2022:

sounds good. It was called vec because the variable was initially a vector, then changed it to a set.

in src/bench/bench.cpp:54 in bacb1aef9c outdated

57+    bool operator()(const T& l, const T& r) const {
58+        return l.second < r.second;
59+    }
60+};
61+
62+std::string ListPriorities()

stickies-v commented at 1:05 pm on October 18, 2022:

0// List priority labels, comma-separated and sorted by increasing priority
1std::string ListPriorities()

stickies-v commented at 1:17 pm on October 18, 2022:

What do you think about returning a sorted std::vector<std::string> here to make it more reusable? The Join() can easily be applied directly in bench_bitcoin.cpp.

furszy commented at 2:26 pm on October 18, 2022:

better to add it when we get an actual use case for it. For now, I think that is fine as is.

stickies-v commented at 2:39 pm on October 18, 2022:

Okay, I don’t agree but not that important so you can mark as resolved.

in src/bench/bench.cpp:90 in bacb1aef9c outdated

91@@ -62,33 +92,39 @@ void benchmark::BenchRunner::RunAll(const Args& args)
92     }
93 
94     std::vector<ankerl::nanobench::Result> benchmarkResults;
95-    for (const auto& p : benchmarks()) {
96-        if (!std::regex_match(p.first, baseMatch, reFilter)) {
97+    for (const auto& [name, bench_func] : benchmarks()) {

stickies-v commented at 1:09 pm on October 18, 2022:

nit: since this unpacking touches a few lines, I think this would benefit from being split out into a separate refactor commit to make that more clear?

furszy commented at 2:28 pm on October 18, 2022:

I think that splitting it would be too much for such a small change.

stickies-v commented at 2:39 pm on October 18, 2022:

Okay, I don’t agree but not that important so you can mark as resolved.

in src/bench/bench.h:62 in bacb1aef9c outdated

58@@ -49,22 +59,24 @@ struct Args {
59     fs::path output_csv;
60     fs::path output_json;
61     std::string regex_filter;
62+    uint8_t priority_level;

stickies-v commented at 1:11 pm on October 18, 2022:

nit: I think priority better highlights the difference between the aggregated priority and the individual priority level

0    uint8_t priority;

stickies-v approved

stickies-v commented at 1:24 pm on October 18, 2022: contributor

ACK bacb1aef9

A couple more suggestions and nits but none of them blocking and could be done in follow-up too if preferred.

furszy force-pushed on Oct 18, 2022

furszy commented at 2:31 pm on October 18, 2022: member

Thanks @stickies-v for the feedback!, applied most of them.

Pure styling diff, no behavior change.

Should be ready to go now.

in src/bench/bench.cpp:92 in 06664b8ef0 outdated

87@@ -62,33 +88,39 @@ void benchmark::BenchRunner::RunAll(const Args& args)
88     }
89 
90     std::vector<ankerl::nanobench::Result> benchmarkResults;
91-    for (const auto& p : benchmarks()) {
92-        if (!std::regex_match(p.first, baseMatch, reFilter)) {
93+    for (const auto& [name, bench_func] : benchmarks()) {
94+        const auto& [func, priority] = bench_func;

stickies-v commented at 2:37 pm on October 18, 2022:

nit: I think you renamed a bit too much - this one actually is a priority_level. My suggestion was to help distinguish between PriorityLevel and a uint8_t, not to just drop the “level” part everywhere. Same thing here and here

furszy commented at 2:53 pm on October 18, 2022:

pushed it.

furszy force-pushed on Oct 18, 2022

stickies-v approved

stickies-v commented at 2:58 pm on October 18, 2022: contributor

re-ACK 010f53e

achow101 commented at 0:55 am on October 19, 2022: member

ACK 010f53e7da9602dc8c92623d4839b5e4c9a813fc

maflcko commented at 8:12 am on October 19, 2022: member

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 commented at 1:59 pm on October 19, 2022: member

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 (review)). 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 commented at 2:11 pm on October 19, 2022: member

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 commented at 2:46 pm on October 19, 2022: member

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 commented at 2:53 pm on October 19, 2022: member

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 commented at 3:01 pm on October 19, 2022: contributor

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 commented at 3:23 pm on October 19, 2022: member

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.

in src/bench/bench.cpp:52 in 6fa5522f28 outdated

43@@ -43,15 +44,38 @@ void GenerateTemplateResults(const std::vector<ankerl::nanobench::Result>& bench
44 
45 namespace benchmark {
46 
47+// map a label to one or multiple priority levels
48+std::map<std::string, uint8_t> map_label_priority = {
49+    std::make_pair("high", PriorityLevel::HIGH),
50+    std::make_pair("medium", PriorityLevel::MEDIUM),
51+    std::make_pair("low", PriorityLevel::LOW),
52+    std::make_pair("all", 0xff)

theStack commented at 10:14 am on October 20, 2022:

stylistic nit (feel free to ignore):

0    {"high", PriorityLevel::HIGH},
1    {"medium", PriorityLevel::MEDIUM},
2    {"low", PriorityLevel::LOW},
3    {"all", 0xff}

furszy commented at 1:24 pm on October 20, 2022:

thanks, pushed

in src/bench/bench_bitcoin.cpp:52 in 6fa5522f28 outdated

48@@ -45,6 +49,14 @@ static std::vector<double> parseAsymptote(const std::string& str) {
49     return numbers;
50 }
51 
52+static uint8_t parsePriorityLevel(const std::string& str) {

theStack commented at 10:19 am on October 20, 2022:

nit: I think we use lowerCamelCase only for interface method names (as per developer guidelines).

0static uint8_t ParsePriorityLevel(const std::string& str) {

furszy commented at 1:11 pm on October 20, 2022:

ok, I followed the same naming as the function that is above (parseAsymptote). Will rename it if I have to retouch.

theStack approved

theStack commented at 10:22 am on October 20, 2022: contributor

Code-review ACK 010f53e7da9602dc8c92623d4839b5e4c9a813fc ⛰️

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).

bench: add "priority level" to the benchmark framework

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

05b8c76232

bench: explicitly make all current benchmarks "high" priority

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

3da7cd2a76

bench: surround main() execution with try/catch

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

466b54bd4a

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

furszy force-pushed on Oct 20, 2022

furszy commented at 1:31 pm on October 20, 2022: member

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 approved

theStack commented at 1:50 pm on October 20, 2022: contributor

re-ACK 3e9d0bea8deb61596c91ead997e9db83f5b0ff68

stickies-v approved

stickies-v commented at 2:20 pm on October 20, 2022: contributor

re-ACK https://github.com/bitcoin/bitcoin/commit/3e9d0bea8deb61596c91ead997e9db83f5b0ff68

kouloumos commented at 2:21 pm on October 20, 2022: contributor

re-ACK 3e9d0bea8deb61596c91ead997e9db83f5b0ff68

achow101 commented at 3:00 pm on October 20, 2022: member

ACK 3e9d0bea8deb61596c91ead997e9db83f5b0ff68

achow101 merged this on Oct 20, 2022

achow101 closed this on Oct 20, 2022

sidhujag referenced this in commit 11e9552d8d on Oct 23, 2022

in src/Makefile.test.include:377 in 3e9d0bea8d

372@@ -373,8 +373,8 @@ endif
373 if TARGET_WINDOWS
374 else
375 if ENABLE_BENCH
376-	@echo "Running bench/bench_bitcoin (one iteration sanity check)..."
377-	$(BENCH_BINARY) --sanity-check > /dev/null
378+	@echo "Running bench/bench_bitcoin (one iteration sanity check, only high priority)..."
379+	$(BENCH_BINARY) -sanity-check -priority-level=high > /dev/null

hebasto commented at 4:46 pm on November 10, 2022:

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

furszy deleted the branch on May 27, 2023

bitcoin locked this on May 26, 2024

bench: add “priority level” to the benchmark framework #26158

Conflicts