Skip to content

bench: Assert that division by zero is unreachable #9547

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
merged 1 commit into from
Mar 6, 2017
Merged

bench: Assert that division by zero is unreachable #9547

merged 1 commit into from
Mar 6, 2017

Conversation

practicalswift
Copy link
Contributor

No description provided.

gmaxwell
gmaxwell reviewed Jan 13, 2017
View reviewed changes
src/bench/bench.cpp Outdated
@@ -88,7 +88,7 @@ bool benchmark::State::KeepRunning()
lastCycles = nowCycles;
++count;

if (now - beginTime < maxElapsed) return true; // Keep going
if (now - beginTime < maxElapsed || count == 1) return true; // Keep going
Copy link
Contributor

Choose a reason for hiding this comment

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

because of the ++count above I don't believe this can be 1 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmaxwell But there is a --count on the line below :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, my read it can't be 1 or 0, it must be >1 and then the decrements reduces it to 1. If it is possible to be 0 at the division then I believe there is a bug elsewhere (in addition).

Copy link
Contributor Author

@practicalswift practicalswift Jan 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If count would be guaranteed to be different from zero, then the check on line 50 (if (count == 0) {) wouldn't be needed, right? :-)

If the branch if (count == 0) { is taken then the relevant parts of the remaining code can be reduced to something like:

count = 0; // implicit
++count;
--count;
double average = (now-beginTime)/count;

Assuming I'm reading it correctly :-)

Let me know if I'm missing something obvious – this is one of my first contributions so please bear with me :-)

Copy link
Member

Choose a reason for hiding this comment

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

Looks like if maxElapsed is > 0 then the logic here is redundant for that code path. count=0 means now and beginTime are both 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to fix the issue in the output at the bottom of the function like so:

     // Output results
-    double average = (now-beginTime)/count;
-    int64_t averageCycles = (nowCycles-beginCycles)/count;
+    double average = (count > 0) ? (now-beginTime)/count : 0.0;
+    int64_t averageCycles = (count > 0) ? (nowCycles-beginCycles)/count : 0;
     std::cout << std::fixed << std::setprecision(15) << name << "," << count << "," << minTime << "," << maxTime << "," << average << ","
               << minCycles << "," << maxCycles << "," << averageCycles << "\n";

I would say that this reads ok in isolation as just dumb output code and avoids having to understand the branching in function body above.

@fanquake fanquake added the Tests label Jan 14, 2017
@practicalswift
Copy link
Contributor Author

practicalswift commented Jan 18, 2017 •
edited
Loading

If we're guaranteed that count cannot be 0 in this context, should I remove the then redundant if (count == 0) {-check on line 50 instead?

As said, please let me know if I'm missing something obvious here and I'll close the PR :-)

@maflcko
Copy link
Member

maflcko commented Jan 22, 2017

Agree with @instagibbs that the code is redundant. Maybe just add a comment or assert that says the count is never zero?

@practicalswift practicalswift force-pushed the avoid-potential-division-by-zero-in-benchmark-state-keeprunning branch 3 times, most recently from bbb78ed to e91074b Compare January 22, 2017 09:23
@practicalswift
Copy link
Contributor Author

@MarcoFalke Good point! Updated, squashed and pushed - looks good? :-)

@isle2983 isle2983 mentioned this pull request Jan 25, 2017
Closed
@jtimon
Copy link
Contributor

jtimon commented Feb 1, 2017

utACK e91074b

@laanwj
Copy link
Member

laanwj commented Feb 2, 2017

We shouldn't be using an assertion for error handling.

When can this happen? What should be the result (instead of dividing by zero)?

@laanwj
Copy link
Member

laanwj commented Feb 2, 2017

Okay, just re-read the post above, so this can never happen. That wasn't clear to me from the code change: please add a comment specifying that.

@practicalswift practicalswift force-pushed the avoid-potential-division-by-zero-in-benchmark-state-keeprunning branch from e91074b to db07f91 Compare February 2, 2017 09:46
@practicalswift
Copy link
Contributor Author

@laanwj Good point! Updated and pushed. Looks good? :-)

@practicalswift
Copy link
Contributor Author

Any changes needed before merge? :-)

@laanwj
Copy link
Member

laanwj commented Feb 28, 2017

I think you should change the pull title first - it is incorrect to say that this is a potential division by zero, because it's avoided by previous code.

@practicalswift practicalswift changed the title Avoid potential division by zero in benchmark::State::KeepRunning() Assert that what might look like a possible division by zero is actually unreachable Feb 28, 2017
@practicalswift
Copy link
Contributor Author

@laanwj Changed. Looks good? :-)

@maflcko maflcko changed the title Assert that what might look like a possible division by zero is actually unreachable bench: Assert that division by zero is unreachable Feb 28, 2017
@laanwj laanwj merged commit db07f91 into bitcoin:master Mar 6, 2017
laanwj added a commit that referenced this pull request Mar 6, 2017
@laanwj
Merge #9547: bench: Assert that division by zero is unreachable
d32581c 
db07f91 Assert that what might look like a possible division by zero is actually unreachable (practicalswift)

Tree-SHA512: f1652eb37196a5b72f356503a1fbb44fb98aa8a94954ad1765f86d81ebf41a2337d4eb58c4f19937fda3752f5d2d642756e44afdbd438015b87ac20801246bff
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 21, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9547: bench: Assert that division by zero is unreachable
d5e90b7 
db07f91 Assert that what might look like a possible division by zero is actually unreachable (practicalswift)

Tree-SHA512: f1652eb37196a5b72f356503a1fbb44fb98aa8a94954ad1765f86d81ebf41a2337d4eb58c4f19937fda3752f5d2d642756e44afdbd438015b87ac20801246bff
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 25, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9547: bench: Assert that division by zero is unreachable
4bcd83f 
db07f91 Assert that what might look like a possible division by zero is actually unreachable (practicalswift)

Tree-SHA512: f1652eb37196a5b72f356503a1fbb44fb98aa8a94954ad1765f86d81ebf41a2337d4eb58c4f19937fda3752f5d2d642756e44afdbd438015b87ac20801246bff
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9547: bench: Assert that division by zero is unreachable
f23ea23 
db07f91 Assert that what might look like a possible division by zero is actually unreachable (practicalswift)

Tree-SHA512: f1652eb37196a5b72f356503a1fbb44fb98aa8a94954ad1765f86d81ebf41a2337d4eb58c4f19937fda3752f5d2d642756e44afdbd438015b87ac20801246bff
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 1, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9547: bench: Assert that division by zero is unreachable
9ed9b73 
db07f91 Assert that what might look like a possible division by zero is actually unreachable (practicalswift)

Tree-SHA512: f1652eb37196a5b72f356503a1fbb44fb98aa8a94954ad1765f86d81ebf41a2337d4eb58c4f19937fda3752f5d2d642756e44afdbd438015b87ac20801246bff
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 1, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9547: bench: Assert that division by zero is unreachable
56d0165 
db07f91 Assert that what might look like a possible division by zero is actually unreachable (practicalswift)

Tree-SHA512: f1652eb37196a5b72f356503a1fbb44fb98aa8a94954ad1765f86d81ebf41a2337d4eb58c4f19937fda3752f5d2d642756e44afdbd438015b87ac20801246bff
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 1, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9547: bench: Assert that division by zero is unreachable
99739e7 
db07f91 Assert that what might look like a possible division by zero is actually unreachable (practicalswift)

Tree-SHA512: f1652eb37196a5b72f356503a1fbb44fb98aa8a94954ad1765f86d81ebf41a2337d4eb58c4f19937fda3752f5d2d642756e44afdbd438015b87ac20801246bff
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 1, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#9547: bench: Assert that division by zero is unreachable
a61b747 
db07f91 Assert that what might look like a possible division by zero is actually unreachable (practicalswift)

Tree-SHA512: f1652eb37196a5b72f356503a1fbb44fb98aa8a94954ad1765f86d81ebf41a2337d4eb58c4f19937fda3752f5d2d642756e44afdbd438015b87ac20801246bff
@str4d str4d mentioned this pull request Feb 22, 2019
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Jan 24, 2020
@zkbot
Auto merge of #3858 - str4d:microbench-1, r=str4d
74ff73a 
Micro-benchmarking framework part 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6733
- bitcoin/bitcoin#6770
- bitcoin/bitcoin#6892
  - Excluding changes to `src/policy/policy.h` which we don't have yet.
- bitcoin/bitcoin#7934
  - Just the benchmark, not the performance improvements.
- bitcoin/bitcoin#8039
- bitcoin/bitcoin#8107
- bitcoin/bitcoin#8115
- bitcoin/bitcoin#8914
  - Required resolving several merge conflicts in code that had been refactored upstream. The changes were simple enough that I decided it was okay to impose merge conflicts on pulling in those refactors later.
- bitcoin/bitcoin#9200
- bitcoin/bitcoin#9202
  - Adds support for measuring CPU cycles, which is later removed in an upstream PR after the refactor. I am including it to reduce future merge conflicts.
- bitcoin/bitcoin#9281
  - Only changes to `src/bench/bench.cpp`
- bitcoin/bitcoin#9498
- bitcoin/bitcoin#9712
- bitcoin/bitcoin#9547
- bitcoin/bitcoin#9505
  - Just the benchmark, not the performance improvements.
- bitcoin/bitcoin#9792
  - Just the benchmark, not the performance improvements.
- bitcoin/bitcoin#10272
- bitcoin/bitcoin#10395
  - Only changes to `src/bench/`
- bitcoin/bitcoin#10735
  - Only changes to `src/bench/base58.cpp`
- bitcoin/bitcoin#10963
- bitcoin/bitcoin#11303
  - Only the benchmark backend change.
- bitcoin/bitcoin#11562
- bitcoin/bitcoin#11646
- bitcoin/bitcoin#11654

This pulls in all changes to the micro-benchmark framework prior to December 2017, when it was rewritten. The rewrite depends on other upstream PRs we have not pulled in yet.

This does not pull in all benchmarks prior to December 2017. It leaves out benchmarks that either test code we do not have yet (except for the `FastRandomContext` refactor, which I decided to pull in), or would require rewrites to work with our changes to the codebase.
@sickpig sickpig mentioned this pull request Apr 28, 2020
Merged
@random-zebra random-zebra mentioned this pull request May 27, 2020
Merged
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Jun 8, 2020
@furszy
Merge #1650: [Tests] Benchmarking Framework
4ed15cc 
3f3edde [Bench] Use PIVX address in Base58Decode test (random-zebra)
5a1be90 [Travis] Disable benchmark framework for trusty test (random-zebra)
1bd89ac Initialize recently introduced non-static class member lastCycles to zero in constructor (random-zebra)
ec60671 Require a steady clock for bench with at least micro precision (random-zebra)
84069ce bench: prefer a steady clock if the resolution is no worse (random-zebra)
38367b1 bench: switch to std::chrono for time measurements (random-zebra)
a24633a Remove countMaskInv caching in bench framework (random-zebra)
9e9bc22 Restore default format state of cout after printing with std::fixed/setprecision (random-zebra)
3dd559d Avoid static analyzer warnings regarding uninitialized arguments (random-zebra)
e85f224 Replace boost::function with std::function (C++11) (random-zebra)
98c0857 Prevent warning: variable 'x' is uninitialized (random-zebra)
7f0d4b3 FastRandom benchmark (random-zebra)
d9fa0c6 Add prevector destructor benchmark (random-zebra)
e1527ba Assert that what might look like a possible division by zero is actually unreachable (random-zebra)
e94cf15 bench: Fix initialization order in registration (random-zebra)
151c25f Basic CCheckQueue Benchmarks (random-zebra)
51aedbc Use std:thread:hardware_concurrency, instead of Boost, to determine available cores (random-zebra)
d447613 Use real number of cores for default -par, ignore virtual cores (random-zebra)
9162a56 [Refactoring] Removed using namespace <xxx> from bench/ sources (random-zebra)
5c07f67 bench: Add support for measuring CPU cycles (random-zebra)
41ce1ed bench: Fix subtle counting issue when rescaling iteration count (random-zebra)
68ea794 Avoid integer division in the benchmark inner-most loop. (random-zebra)
3fa4f27 bench: Added base58 encoding/decoding benchmarks (random-zebra)
4442118 bench: Add crypto hash benchmarks (random-zebra)
a5179b6 [Trivial] ensure minimal header conventions (random-zebra)
8607d6b Support very-fast-running benchmarks (random-zebra)
4aebb60 Simple benchmarking framework (random-zebra)

Pull request description:

  Introduces the benchmarking framework, loosely based on google's micro-benchmarking library (https://github.com/google/benchmark), ported from Bitcoin, up to 0.16.
  The benchmark framework is hard-coded to run each benchmark for one wall-clock second,
  and then spits out .csv-format timing information to stdout.

  Backported PR:
  - bitcoin#6733
  - bitcoin#6770
  - bitcoin#6892
  - bitcoin#8039
  - bitcoin#8107
  - bitcoin#8115
  - bitcoin#9200
  - bitcoin#9202
  - bitcoin#9281
  - bitcoin#6361
  - bitcoin#10271
  - bitcoin#9498
  - bitcoin#9712
  - bitcoin#9547
  - bitcoin#9505 (benchmark only. Rest was in #1557)
  - bitcoin#9792 (benchmark only. Rest was in #643)
  - bitcoin#10272
  - bitcoin#10395 (base58 only)
  - bitcoin#10963
  - bitcoin#11303 (first commit)
  - bitcoin#11562
  - bitcoin#11646
  - bitcoin#11654

  Current output of `src/bench/bench_pivx`:
  ```
  #Benchmark,count,min(ns),max(ns),average(ns),min_cycles,max_cycles,average_cycles
  Base58CheckEncode,131072,7697,8065,7785,20015,20971,20242
  Base58Decode,294912,3305,3537,3454,8595,9198,8981
  Base58Encode,180224,5498,6020,5767,14297,15652,14994
  CCheckQueueSpeed,320,3159960,3535173,3352787,8216030,9191602,8717388
  CCheckQueueSpeedPrevectorJob,96,9184484,11410840,10823070,23880046,29668680,28140445
  FastRandom_1bit,320,3143690,4838162,3199156,8173726,12579373,8317941
  FastRandom_32bit,60,17097612,17923669,17367440,44454504,46602306,45156079
  PrevectorClear,3072,334741,366618,346731,870340,953224,901516
  PrevectorDestructor,2816,344233,368912,357281,895022,959187,928948
  RIPEMD160,288,3404503,3693917,3577774,8851850,9604334,9302363
  SHA1,384,2718128,2891558,2802513,7067238,7518184,7286652
  SHA256,176,6133760,6580005,6239866,15948035,17108376,16223916
  SHA512,240,4251468,4358706,4313463,11054006,11332826,11215186
  Sleep100ms,10,100221470,100302411,100239073,260580075,260790726,260625870
  ```

  NOTE: Not all the tests have been pulled yet (as we might not have the code being tested, or it  would require rewrites to work with our different code base), but the framework is updated to December 2017.

ACKs for top commit:
  Fuzzbawls:
    ACK 3f3edde

Tree-SHA512: c283311a9accf6d2feeb93b185afa08589ebef3f18b6e86980dbc3647b9845f75ac9ecce2f1b08738d25ceac36596a2c89d41e4dbf3b463502aa695611aa1f8e
@practicalswift practicalswift deleted the avoid-potential-division-by-zero-in-benchmark-state-keeprunning branch April 10, 2021 19:29
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@gmaxwell gmaxwell gmaxwell left review comments

@instagibbs instagibbs instagibbs left review comments

@isle2983 isle2983 isle2983 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@practicalswift @maflcko @jtimon @laanwj @gmaxwell @instagibbs @isle2983 @fanquake