Skip to content

partial merge #18011: Replace current benchmarking framework with nanobench #4188

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
PastaPastaPasta merged 2 commits into from
Jul 6, 2021
Merged

partial merge #18011: Replace current benchmarking framework with nanobench #4188

PastaPastaPasta merged 2 commits into from
Jul 6, 2021

Conversation

@kwvg
Copy link
Collaborator

@kwvg kwvg commented Jun 4, 2021 •
edited
Loading

Contents

bitcoin#18011

Example output from bench_dash

ns/op op/s err% total benchmark
81,730.00 12,235.41 7.7% 0.00 〰️ BLSDKG_BuildQuorumVerificationVectors_parallel_10 (Unstable with ~1.0 iters. Increase minEpochIterations to e.g. 10)
4,624,233.00 216.25 8.1% 0.05 〰️ BLSDKG_BuildQuorumVerificationVectors_parallel_100 (Unstable with ~1.0 iters. Increase minEpochIterations to e.g. 10)
95,615,174.00 10.46 2.6% 1.05 BLSDKG_BuildQuorumVerificationVectors_parallel_400
105,330.00 9,493.97 0.3% 0.00 BLSDKG_BuildQuorumVerificationVectors_simple_10
10,147,428.00 98.55 1.0% 0.11 BLSDKG_BuildQuorumVerificationVectors_simple_100
176,520,428.00 5.67 0.8% 1.97 BLSDKG_BuildQuorumVerificationVectors_simple_400

Things To Do

  • Reconsider if we want to benchmark parallel operations as it creates a long period of no output before the first benchmarks show
  • Find optimal minEpochIterations for Dash-specific tests

Disclosures

  • This is a work in progress and needs external review. Dash-specific changes have not been tested, only compilation has been ensured.
  • Contains patches by UdjinM6, most recently f16ff50

@PastaPastaPasta
Copy link
Member

PastaPastaPasta commented Jun 4, 2021

I spent some time trying to do this, but got annoyed by the number of benchmarks that were "wavy" and had a high error percentage. I couldn't find a good way to fix this, but I think this PR is still very valuable

@kwvg
Copy link
Collaborator Author

kwvg commented Jun 4, 2021

I noticed that just after I published this PR and clicked on the page for bitcoin#18011. Oops.

This is just one PR in a series of PRs that should get our benchmark system to par and unless there's any input about parallel tests, I might enclose them behind an opt-in flag that will warn the user that it is relatively time consuming even on a relatively modest computer

@PastaPastaPasta PastaPastaPasta added this to the 17.1 milestone Jun 5, 2021
@PastaPastaPasta PastaPastaPasta marked this pull request as draft June 5, 2021 17:59
@kwvg kwvg force-pushed the bench branch 2 times, most recently from 815c120 to d20cdd1 Compare June 18, 2021 12:13
@kwvg
Copy link
Collaborator Author

kwvg commented Jun 18, 2021

Results from test optimization and refactoring (courtesy of d20cdd1ad638eb14bfc433af2f7e1592ee80db05)

ns/op op/s err% total benchmark
10,303.11 97,058.07 1.8% 0.00 BLSDKG_BuildQuorumVerificationVectors_simple_10
94,420.59 10,590.91 1.4% 0.10 BLSDKG_BuildQuorumVerificationVectors_simple_100
459,254.56 2,177.44 2.4% 2.01 BLSDKG_BuildQuorumVerificationVectors_simple_400
546,491.68 1,829.85 1.6% 0.06 BLSDKG_VerifyContributionShares_aggregated_10
1,464,445.49 682.85 3.4% 1.60 BLSDKG_VerifyContributionShares_aggregated_100
2,758,060.48 362.57 0.7% 12.16 BLSDKG_VerifyContributionShares_aggregated_400
1,580,498.33 632.71 0.8% 0.17 BLSDKG_VerifyContributionShares_simple_10
5,377,070.56 185.97 2.7% 5.92 BLSDKG_VerifyContributionShares_simple_100
10,242,907.76 97.63 2.5% 43.62 BLSDKG_VerifyContributionShares_simple_400
1,485.92 672,982.69 0.1% 0.00 BLS_PubKeyAggregate_Normal
1,118.50 894,054.54 0.1% 0.00 BLS_SecKeyAggregate_Normal
1,679,007.24 595.59 2.3% 0.02 BLS_Sign_Normal
3,035.00 329,489.29 0.7% 0.00 BLS_SignatureAggregate_Normal
2,056,411.88 486.28 1.4% 0.02 BLS_Verify_Batched
478,662.71 2,089.15 0.5% 0.01 BLS_Verify_BatchedParallel
141,857,099.50 7.05 0.4% 1.56 BLS_Verify_LargeAggregatedBlock100
1,405,682,822.60 0.71 0.6% 15.49 BLS_Verify_LargeAggregatedBlock1000
145,493,207.00 6.87 0.2% 1.60 BLS_Verify_LargeAggregatedBlock1000PreVerified
325,996,394.00 3.07 0.3% 3.59 BLS_Verify_LargeBlock100
3,250,336,193.00 0.31 0.2% 35.76 BLS_Verify_LargeBlock1000
143,097,232.00 6.99 0.7% 1.58 BLS_Verify_LargeBlockSelfAggregated100

@PastaPastaPasta PastaPastaPasta marked this pull request as ready for review June 22, 2021 17:12
PastaPastaPasta
PastaPastaPasta requested changes Jun 22, 2021
View reviewed changes
UdjinM6
UdjinM6 reviewed Jun 24, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

Few first sight notes

@kwvg kwvg changed the title Merge #18011: Replace current benchmarking framework with nanobench partial merge #18011: Replace current benchmarking framework with nanobench Jun 24, 2021
UdjinM6
UdjinM6 reviewed Jun 24, 2021
View reviewed changes
@kwvg kwvg force-pushed the bench branch 2 times, most recently from fa36f5c to 95c7f50 Compare June 25, 2021 14:14
@PastaPastaPasta
Copy link
Member

PastaPastaPasta commented Jun 25, 2021

Build fails

@PastaPastaPasta
Copy link
Member

PastaPastaPasta commented Jun 26, 2021

needs rebase

@UdjinM6
Copy link

UdjinM6 commented Jun 27, 2021

Pls see 01831b8771fa39fde39f90a8aed96f1e8717c23f and maybe 9f95e7632431ee788e6862e3194dcefb5a021a2f

@kwvg kwvg force-pushed the bench branch 2 times, most recently from 0cfe304 to 5435351 Compare July 2, 2021 10:02
@kwvg
Copy link
Collaborator Author

kwvg commented Jul 2, 2021

Rebased, bench works now!

@kwvg kwvg requested review from PastaPastaPasta and UdjinM6 July 2, 2021 10:04
UdjinM6
UdjinM6 previously approved these changes Jul 2, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

LGTM, ACK

@UdjinM6
Copy link

UdjinM6 commented Jul 3, 2021

Pls rebase and drop 14244 and 13822 (the were merged in other PRs recently)

@kwvg kwvg requested a review from UdjinM6 July 4, 2021 10:12
PastaPastaPasta
PastaPastaPasta previously approved these changes Jul 4, 2021
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

ACK

@UdjinM6
Copy link

UdjinM6 commented Jul 5, 2021

Noticed few issues - not sure if I missed them earlier 🙈 or if they were introduced via the most recent rebase 👀 anyway, pls see f16ff50

kwvg added 2 commits July 5, 2021 18:28
@kwvg
merge bitcoin#18011: Replace current benchmarking framework with nano…
b5db3c3 
…bench
@kwvg
bench: adapt bls_dkg tests to nanobench
baabcf4 
* stop using global pointers, every run gets a unique pointer for itself
* keep tests relatively self contained, remove the need for CleanupBLSDkgTests()
* remove parallel tests, they seem to be somewhat unpredictable and seem to have no correlated increased CPU usage
* minor cleanup and refactoring, convert struct to class, rearrange variables
* correlate batch with quorum, minEpochIterations set to 1, unless tests are known unstable, then set to 10
UdjinM6
UdjinM6 approved these changes Jul 5, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

PastaPastaPasta
PastaPastaPasta approved these changes Jul 6, 2021
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

re-ACK

@PastaPastaPasta PastaPastaPasta merged commit 6e4099e into dashpay:develop Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

18

Development

Successfully merging this pull request may close these issues.

3 participants

@kwvg @PastaPastaPasta @UdjinM6