Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 2 commits into from
Nov 5, 2018
Merged

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

maflcko merged 2 commits into from
Nov 5, 2018

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Aug 28, 2018 •
edited
Loading

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
Copy link
Member

laanwj commented Aug 28, 2018 •
edited
Loading

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
Copy link
Contributor Author

practicalswift commented Aug 28, 2018

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

@maflcko
Copy link
Member

maflcko commented Aug 28, 2018

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

maflcko
maflcko reviewed Aug 28, 2018
View reviewed changes
src/Makefile.test.include Outdated
Copy link
Member

@maflcko maflcko Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@practicalswift practicalswift Aug 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 dry-run-of-bench_bitcoin-as-part-of-check-local branch from 1d0e186 to fcb34f4 Compare August 29, 2018 12:33
@ken2812221 ken2812221 mentioned this pull request Aug 29, 2018
Merged
@practicalswift practicalswift force-pushed the dry-run-of-bench_bitcoin-as-part-of-check-local branch 2 times, most recently from ab4bea1 to e4f0372 Compare August 29, 2018 19:35
@practicalswift
Remove RUN_BENCH logic
00c6306
@practicalswift
tests: Dry run bench_bitcoin (-evals=1 -scaling=0: <1 second running …
dfef0df 
…time) as part "make check" to allow for quick identification of assertion/sanitizer failures in benchmarking code
@practicalswift practicalswift force-pushed the dry-run-of-bench_bitcoin-as-part-of-check-local branch from e4f0372 to dfef0df Compare August 30, 2018 20:54
@practicalswift practicalswift mentioned this pull request Aug 31, 2018
Closed
@DrahtBot DrahtBot mentioned this pull request Sep 18, 2018
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 18, 2018 •
edited
Loading

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.

@maflcko
Copy link
Member

maflcko commented Sep 18, 2018

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

@fanquake
Copy link
Member

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

@practicalswift practicalswift mentioned this pull request Oct 22, 2018
Closed
@maflcko maflcko merged commit dfef0df into bitcoin:master Nov 5, 2018
maflcko pushed a commit that referenced this pull request Nov 5, 2018
Merge #14092: tests: Dry run bench_bitcoin as part "make check" to al…
73a8408 
…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
@domob1812 domob1812 mentioned this pull request Nov 13, 2018
Open
@scravy scravy mentioned this pull request Mar 3, 2019
Closed
@practicalswift practicalswift deleted the dry-run-of-bench_bitcoin-as-part-of-check-local branch April 10, 2021 19:35
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request May 23, 2022
@Munkybooty
Merge bitcoin#14092: tests: Dry run bench_bitcoin as part "make check…
a557022 
…" to allow 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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Build system Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@practicalswift @laanwj @maflcko @DrahtBot @fanquake