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

laanwj · 2022-05-11T14:33:02Z

The benchmarks are run as part of make check for a crash-sanity check. The actual results are being ignored. So only run them for one iteration.

This makes the bench_bitcoin part take 2m00 instead of 5m20 here. Which is still too long (imo), but this needs to be solved in the WalletLoading* benchmarks which take that long per iteration.

Also change all bench_bitcoin arguments to kebab-case to be consistent with the other tools (in a separate commit).

theStack · 2022-05-11T14:38:29Z

Concept ACK

src/bench/bench.cpp

w0xlt · 2022-05-11T14:46:37Z

Concept ACK

src/Makefile.test.include

fanquake · 2022-05-12T08:09:42Z

Concept ACK

jonatack

Concept and approach ACK, now building and testing.

src/bench/bench_bitcoin.cpp

src/bench/bench.h

laanwj · 2022-05-12T11:14:54Z

Re-pushed to address @jonatack's comments.

jonatack

Tested ACK c52a71e though the reduction in run time isn't large in my testing

OK
Running bench/bench_bitcoin (one iteration sanity check)...
bench/bench_bitcoin --one-iteration > /dev/null

Modulo typo in first commit message s/kebab-hase/kebab-case/

src/bench/bench.cpp

maflcko · 2022-05-13T05:44:21Z

Looks like #24924 might be related

laanwj · 2022-05-17T09:30:37Z

Re-pushed:

Argument is now -sanity-check.
Add warning that results are useless.
Fix typo in commit message.

Should be ready for final review round and merge.

The benchmarks are run as part of `make check` for a minimum sanity check. The actual results are being ignored. So only run them for one iteration. This makes the `bench_bitcoin` part take 2m00 instead of 5m20 here. Which is still too long (imo), but this needs to be solved in the `WalletLoading*` benchmarks which take that long per iteration.

This is customary for UNIX-style arguments, and more consistent with our other tools

hebasto · 2022-05-17T09:57:13Z

Concept ACK.

hebasto · 2022-05-17T10:28:39Z

src/bench/bench.cpp

@@ -57,6 +57,10 @@ void benchmark::BenchRunner::RunAll(const Args& args)
    std::regex reFilter(args.regex_filter);
    std::smatch baseMatch;

+    if (args.sanity_check) {
+        std::cout << "Running with --sanity check option, benchmark results will be useless." << std::endl;


Suggested change

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

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.

hebasto · 2022-05-17T10:29:03Z

src/Makefile.test.include

-	@echo "Running bench/bench_bitcoin ..."
-	$(BENCH_BINARY) > /dev/null
+	@echo "Running bench/bench_bitcoin (one iteration sanity check)..."
+	$(BENCH_BINARY) --sanity-check > /dev/null


Suggested change

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

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

jonatack · 2022-05-17T11:15:23Z

ACK 4f31c21 on the sanity-check version per git diff c52a71e 4f31c28 (modulo s/--sanity check/--sanity-check/ in src/bench/bench.cpp::L61)

laanwj · 2022-05-17T11:56:49Z

@hebasto Maybe I'm an old UNIX fart in some things but I prefer long options prefixed with two -, I prefer not to change this. And you can specify them with one - if you want, sure, that works too.

hebasto

ACK 4f31c21, tested on Ubuntu 22.04.

…heck

…k` in "Win64 native" task 91bee4d ci: Run `bench_bitcoin.exe --sanity-check` in "Win64 native" task (Hennadii Stepanov) Pull request description: This PR adds [`--sanity-check`](bitcoin/bitcoin#25107) flag to `src\bench_bitcoin.exe` invocation as its results are been discarded. Also a better name used for the script as it follows GNU's `make check`. ACKs for top commit: fanquake: ACK 91bee4d Tree-SHA512: fd5feeda72d1ef46c5fbfc2aa5c042ab2e3de7772546379da4596306b5658ab95f62939fba237c0bd7a1b09c85de20fc1cd9e5df1efe11bdae50d4a7b8081f74

laanwj added the Tests label May 11, 2022

laanwj commented May 11, 2022

View reviewed changes

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

laanwj force-pushed the 2022-05-bench-one-iteration branch 2 times, most recently from c9de493 to 63068c7 Compare May 12, 2022 06:32

laanwj commented May 12, 2022

View reviewed changes

src/Makefile.test.include Show resolved Hide resolved

jonatack reviewed May 12, 2022

View reviewed changes

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

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

laanwj force-pushed the 2022-05-bench-one-iteration branch from 63068c7 to c52a71e Compare May 12, 2022 11:14

jonatack reviewed May 12, 2022

View reviewed changes

src/bench/bench.cpp Show resolved Hide resolved

laanwj force-pushed the 2022-05-bench-one-iteration branch from c52a71e to 48b3828 Compare May 17, 2022 09:30

laanwj changed the title ~~bench: Add --one-iteration flag, use it in make check~~ bench: Add --sanity-check flag, use it in make check May 17, 2022

laanwj added 2 commits May 17, 2022 11:32

bench: Make all arguments -kebab-case

4f31c21

This is customary for UNIX-style arguments, and more consistent with our other tools

laanwj force-pushed the 2022-05-bench-one-iteration branch from 48b3828 to 4f31c21 Compare May 17, 2022 09:32

hebasto reviewed May 17, 2022

View reviewed changes

hebasto approved these changes May 17, 2022

View reviewed changes

fanquake merged commit dd8a2df into bitcoin:master May 17, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 28, 2022

Merge bitcoin#25107: bench: Add --sanity-check flag, use it in make c…

b08a8d3

…heck

hebasto mentioned this pull request Oct 2, 2022

ci: Run bench_bitcoin.exe --sanity-check in "Win64 native" task #26230

Merged

bitcoin locked and limited conversation to collaborators May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

laanwj commented May 11, 2022 •

edited

Loading

theStack commented May 11, 2022

w0xlt commented May 11, 2022

fanquake commented May 12, 2022

jonatack left a comment •

edited

Loading

laanwj commented May 12, 2022

jonatack left a comment •

edited

Loading

maflcko commented May 13, 2022

laanwj commented May 17, 2022 •

edited

Loading

hebasto commented May 17, 2022

hebasto May 17, 2022

jonatack May 17, 2022 •

edited

Loading

hebasto May 17, 2022

jonatack commented May 17, 2022 •

edited

Loading

laanwj commented May 17, 2022 •

edited

Loading

hebasto left a comment

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

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

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

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

Conversation

laanwj commented May 11, 2022 • edited Loading

theStack commented May 11, 2022

w0xlt commented May 11, 2022

fanquake commented May 12, 2022

jonatack left a comment • edited Loading

Choose a reason for hiding this comment

laanwj commented May 12, 2022

jonatack left a comment • edited Loading

Choose a reason for hiding this comment

maflcko commented May 13, 2022

laanwj commented May 17, 2022 • edited Loading

hebasto commented May 17, 2022

hebasto May 17, 2022

Choose a reason for hiding this comment

jonatack May 17, 2022 • edited Loading

Choose a reason for hiding this comment

hebasto May 17, 2022

Choose a reason for hiding this comment

jonatack commented May 17, 2022 • edited Loading

laanwj commented May 17, 2022 • edited Loading

hebasto left a comment

Choose a reason for hiding this comment

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

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

laanwj commented May 11, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack left a comment •

edited

Loading

laanwj commented May 17, 2022 •

edited

Loading

jonatack May 17, 2022 •

edited

Loading

jonatack commented May 17, 2022 •

edited

Loading

laanwj commented May 17, 2022 •

edited

Loading