bench: Add `--sanity-check` flag, use it in `make check` #25107

Could also add separate flags for --num-epochs and --epoch-iterations (which default to 11 and 0 respectively, as they do currently in Config).

I'd use bench.epochs(1).epochIterations(1) instead of modifying the Config members directly

That's a great suggestion, will do (edit: done).

Also added a comment to preempt "why are we running the benchmarks in make check at all" questions.

nit, sort here and below

The other args are snakecased and this is kebabcase; maybe good to use one style or the other.

Yes, I agree. I'll convert all the arguments to kebab-case (which is customary for UNIX-style arguments) in another commit.

nit, place/pack this just below the first bool member

ok, sure, why not, sorting the fields by type makes as much sense as anything

yes, they've been sorted roughly by increasing size

maybe provide some kind of user feedback that the option is running (I did this to sanity-check that it was)

--- a/src/bench/bench.cpp
+++ b/src/bench/bench.cpp
@@ -58,6 +58,9 @@ void benchmark::BenchRunner::RunAll(const Args& args)
     std::smatch baseMatch;
 
     std::vector<ankerl::nanobench::Result> benchmarkResults;
+    if (args.one_iteration) {
+        std::cout << "Running with --one-iteration option, i.e. epochs(1).epochIterations(1)" << std::endl;
+    }
     for (const auto& p : benchmarks()) {
         if (!std::regex_match(p.first, baseMatch, reFilter)) {

output

$ time ./src/bench/bench_bitcoin -filter=WalletBalance*.* --one-iteration
Running with --one-iteration option, i.e. epochs(1).epochIterations(1)
Warning, results might be unstable:
* DEBUG defined
* CPU frequency scaling enabled: CPU 0 between 2,485.0 and 3,100.0 MHz
* Turbo is enabled, CPU frequency will fluctuate

Recommendations
* Make sure you compile for Release
* Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------

(unrelated, and not sure there is a use case, but --one-iteration could later be changed to an integer --iterations arg)

Revealing the fact that it's using epochs(1).epochIterations(1) seems like an unnecessary implementation detail for the bench_bitcoin user.

I'd also prefer a config name like --sanity-check. Maybe in future this could do more to be quicker, like run all the benchmarks in parallel (I had a quick look if that works but got a segfault, probably due to some global states)

Revealing the fact that it's using epochs(1).epochIterations(1) seems like an unnecessary implementation detail for the bench_bitcoin user.

Yeah. That's too much detail. But adding a warning message that it's active to the output makes sense, I guess.

I'd also prefer a config name like --sanity-check. Maybe in future this could do more to be quicker, like run all the benchmarks in parallel (I had a quick look if that works but got a segfault, probably due to some global states)

Agree on the rename. Running the benchmarks in parallel would be another stress test but it seems we're not ready for that :smile:

Though ideally I would like to aim for <10ms per iteration for all benchmarks, so running 1 iteration in parallel shouldn't have that much effect. Running 1 of everything in serial is still supposed to be fast!

Agree on the rename. @laanwj did you want to do that rename here? If not, I'd like to merge this soon.

Yes, I'm going to make the requested changes here and re-push in a bit.

        std::cout << "Running with -sanity-check option, benchmark results will be useless." << std::endl;

Also s/--sanity-check/-sanity-check/ in the commit message?

I'm not sure as there isn't a single-letter option like -s here, but per https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html: "Long options consist of -- followed by a name made of alphanumeric characters and dashes. Option names are typically one to three words long, with hyphens to separate words. Users can abbreviate the option names as long as the abbreviations are unique."

Edit: never mind, our convention is to use a single-hyphen prefix. That said, either way works for documentation purposes.

	$(BENCH_BINARY) -sanity-check > /dev/null