Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: Dry run bench_bitcoin as part "make check" to allow for quick identification of assertion/sanitizer failures in benchmarking code #14092

Conversation

Projects
None yet
5 participants
@practicalswift
Copy link
Member

commented Aug 28, 2018

Dry run bench_bitcoin (-evals=1 -scaling=0: <1 second running time) as part make check to allow for quick identification of assertion/sanitizer failures or crashes in benchmarking code.

This is already tested in Travis but it is nice to have it locally too. The cost is near zero.

@practicalswift practicalswift changed the title tests: Dry run bench_bitcoin (-evals=1 -scaling=0: <1 second running time) as part "make check" to allow for quick identification of assertion/sanitizer failures in benchmarking code tests: Dry run bench_bitcoin as part "make check" to allow for quick identification of assertion/sanitizer failures in benchmarking code Aug 28, 2018

@fanquake fanquake added the Tests label Aug 28, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

in this case you'll probably want to remove the RUN_BENCH logic from travis as it does pretty much the same (introduced in #13811)
ping @MarcoFalke

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

@laanwj Yes, that is my plan if @MarcoFalke is OK with it :-)

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

I don't see any downside to doing this. So Concept ACK, but I can't review build system changes.

src/Makefile.test.include Outdated
@@ -167,6 +167,8 @@ check-local: $(BITCOIN_TESTS:.cpp=.cpp.test)
$(PYTHON) $(top_builddir)/test/util/bitcoin-util-test.py
@echo "Running test/util/rpcauth-test.py..."
$(PYTHON) $(top_builddir)/test/util/rpcauth-test.py
@echo "Running bench/bench_bitcoin -evals=1 -scaling=0..."
$(top_builddir)/src/bench/bench_bitcoin -evals=1 -scaling=0 2> >(grep -vE "^WARNING: ") > /dev/null

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 28, 2018

Member

Does scaling=0 actually run everything at least once? If not, just set to a small value larger than zero.

This comment has been minimized.

Copy link
@practicalswift

practicalswift Aug 28, 2018

Author Member

@MarcoFalke Yes it does, see:

bitcoin/src/bench/bench.cpp

Lines 115 to 118 in 78dae8c

uint64_t num_iters = static_cast<uint64_t>(p.second.num_iters_for_one_second * scaling);
if (0 == num_iters) {
num_iters = 1;
}

@practicalswift practicalswift force-pushed the practicalswift:dry-run-of-bench_bitcoin-as-part-of-check-local branch Aug 29, 2018

@practicalswift practicalswift force-pushed the practicalswift:dry-run-of-bench_bitcoin-as-part-of-check-local branch 2 times, most recently Aug 29, 2018

practicalswift added some commits Aug 30, 2018

tests: Dry run bench_bitcoin (-evals=1 -scaling=0: <1 second running …
…time) as part "make check" to allow for quick identification of assertion/sanitizer failures in benchmarking code
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14252 (build: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan) by practicalswift)

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

Concept ACK dfef0df, can't review the build system changes, though.

@fanquake

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

Tested by merging dfef0df and then applying the changes from #14011 (so that make check actually runs on macOS).

Got the bench output:

OK
Running bench/bench_bitcoin -evals=1 -scaling=0...
bench/bench_bitcoin -evals=1 -scaling=0 > /dev/null
.....

Added an assert(0) to AssembleBlock to check that a failure is caught.

OK
Running bench/bench_bitcoin -evals=1 -scaling=0...
bench/bench_bitcoin -evals=1 -scaling=0 > /dev/null
Assertion failed: (0), function AssembleBlock, file bench/block_assemble.cpp, line 57.
/bin/sh: line 1: 60728 Abort trap: 6           bench/bench_bitcoin -evals=1 -scaling=0 > /dev/null
make[3]: *** [check-local] Error 134
make[2]: *** [check-am] Error 2
make[1]: *** [check-recursive] Error 1
make: *** [check-recursive] Error 1

Probably worth getting @theuni to glance over the test.include changes pre-merge.

@MarcoFalke MarcoFalke merged commit dfef0df into bitcoin:master Nov 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Nov 5, 2018

Merge #14092: tests: Dry run bench_bitcoin as part "make check" to al…
…low for quick identification of assertion/sanitizer failures in benchmarking code

dfef0df tests: Dry run bench_bitcoin (-evals=1 -scaling=0: <1 second running time) as part "make check" to allow for quick identification of assertion/sanitizer failures in benchmarking code (practicalswift)
00c6306 Remove RUN_BENCH logic (practicalswift)

Pull request description:

  Dry run `bench_bitcoin` (`-evals=1 -scaling=0`: <1 second running time) as part `make check` to allow for quick identification of assertion/sanitizer failures or crashes in benchmarking code.

  This is already tested in Travis but it is nice to have it locally too. The cost is near zero.

Tree-SHA512: 1f51b86b34bf97f75785f2694891d80f1bfb3e050211e6f6c35d8d9bc80c75bdebaa5ebfa51855ac0cf76d8773c3026bc576f60d0227afb0e646d728b83abde7

@scravy scravy referenced this pull request Mar 3, 2019

Closed

bench is broken #722

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.