bench: Remove -priority-level= option #34210
Conversation
DrahtBot
changed the title
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34210. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK. |
maflcko
force-pushed
the
2601-bench-less-prio
branch
from
fa25dbb to
faa2eab
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
ACK faa2eab Tested:
|
|
cc @furszy |
l0rinc
left a comment
l0rinc
left a comment
There was a problem hiding this comment.
Concept ACK - please see a few remaining suggestions that I would prefer we still did if we're already touching these lines.
-BEGIN VERIFY SCRIPT- sed --in-place --regexp-extended 's/BENCHMARK\(([^,]+), benchmark::PriorityLevel::(HIGH|LOW)\)/BENCHMARK(\1)/g' $( git grep -l PriorityLevel ) sed --in-place 's/#define BENCHMARK(n, priority_level)/#define BENCHMARK(n)/g' ./src/bench/bench.h -END VERIFY SCRIPT-
Duplicate benchmarks with the same name are not supported. Expanding the name with __LINE__ is confusing and brittle, because it makes duplication bugs silent. Fix this twofold: * By enforcing unique benchmarks at compile-time and link-time. For example, a link failure may now look like: "mold: error: duplicate symbol: bench_runner_AddrManAdd" * By enforcing unique benchmarks at run-time. This should never happen, due to the build-failure, but a failure may look like: "Assertion `benchmarks().try_emplace(std::move(name), std::move(func)).second' failed."
This makes the code more consistent. Also, use "using BenchFunction = ..." while touching the header. Also, fixup the whitespace after and earlier scripted-diff.
maflcko
force-pushed
the
2601-bench-less-prio
branch
from
faa2eab to
fa3df52
Compare
|
Should be trivial to re-review the nits with git range-diff |
I don't think they are redundant. There is only one semicolon after macro expansion. In fact, including the semicolon in the macro itself may lead to redundant semicolons. Generally, macros should not include a trailing semicolon, because it is confusing, and inconsistent. Also, tools (such as clang-format) may get confused. Edit: To find other places where the semicolon could be removed from the macro: git ls-files | xargs perl -0777 -ne 'print "$ARGV: $&\n" while /#define\s*\S*\(([^\\\n]|\\\n)*?;\n/sg' |
|
Concept ACK - these were introduced to be able to skip running benchmarks that were very slow. This ultimately just led to nobody running the benchmarks, even when changing the code, so they were silently broken (#32277). |
6 participants
The option was added in #26158, when the project was using an autotools-based build system. However, in the meantime this option is unused:
First, commit 27f1121 removed the option from one CI task
Then test: Run all benchmarks in the sanity check #32310 removed the option from CMakeList.txt, because:
Finally, after commit 0ad4376, I don't see a single reason to keep this option, so remove it.
Also, there is a commit to turn a silent ignore of duplicate bench names into an error.