Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Benchmarks for block verification #11035

Merged
merged 7 commits into from
Sep 11, 2019
Merged

Benchmarks for block verification #11035

merged 7 commits into from
Sep 11, 2019

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Sep 10, 2019 •
edited
Loading

Uses real blocks from mainnet to benchmark the verify_* family of methods
in the verification module.

NOTE the functions benchmarked here are pub but the module is not exposed publicly. To work around this it is necessary to run the benchmarks with cargo bench --features=bench. Prior art.

Results:

verify_block_basic      time:   [309.11 us 310.68 us 312.20 us]
                        change: [+0.0444% +3.8884% +7.8211%] (p = 0.02 < 0.05)
                        Change within noise threshold.
Found 9 outliers among 100 measurements (9.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild
  6 (6.00%) high severe

Benchmarking verify_block_unordered: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 288.1s or reduce sample count to 10
verify_block_unordered  time:   [25.622 ms 25.870 ms 26.151 ms]
                        change: [-5.8548% -3.4823% -0.9301%] (p = 0.01 < 0.05)
                        Change within noise threshold.
Found 7 outliers among 100 measurements (7.00%)
  3 (3.00%) high mild
  4 (4.00%) high severe

verify_block_family (partial)
                        time:   [348.14 ns 350.11 ns 352.17 ns]
                        change: [-3.3510% -0.3836% +2.6583%] (p = 0.82 > 0.05)
                        No change in performance detected.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) high mild
  2 (2.00%) high severe

verify_block_family (full)
                        time:   [615.22 us 620.32 us 625.74 us]
                        change: [-6.1034% -3.6971% -0.3676%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

Also exposes the TestBlockChain in a test helper.

@dvdplm
WIP
09a5612
@dvdplm
wip
926ce64
@dvdplm
Benchmarks for block verification
33ff04b 
Uses real blocks from mainnet to benchmark the `verify_*` family of methods in the `verification` module.

Also exposes the `TestBlockChain` in a test helper.
@dvdplm dvdplm self-assigned this Sep 10, 2019
@dvdplm dvdplm added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Sep 10, 2019
@dvdplm dvdplm marked this pull request as ready for review September 10, 2019 20:19
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 10, 2019
niklasad1
niklasad1 reviewed Sep 11, 2019
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
niklasad1
niklasad1 reviewed Sep 11, 2019
View reviewed changes
ethcore/src/block.rs Outdated Show resolved Hide resolved
niklasad1
niklasad1 reviewed Sep 11, 2019
View reviewed changes
ethcore/types/src/verification.rs
@@ -57,7 +57,7 @@ impl VerificationQueueInfo {
}

/// An unverified block.
#[derive(PartialEq, Debug, MallocSizeOf)]
#[derive(Clone, PartialEq, Debug, MallocSizeOf)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clone is used for benchmarks/tests?

Why not #[cfg_attr(any(foo, bar), derive(Clone)]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that is the reason. It's more convenience than necessity though. In general though, why is it bad to derive Clone? Reading https://rust-lang-nursery.github.io/api-guidelines/interoperability.html#c-common-traits it seems idiomatic to do so whenever it's convenient?

Copy link
Collaborator

@niklasad1 niklasad1 Sep 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

nothing bad I guess but it might have the reasoning that such big structures should not be cloned which are enforced by rustc when no Clone impl exists but I don't know really just guessing!

EDIT: It would produce a slightly bigger binary I guess but so small that it is probably negligible

ordian
ordian reviewed Sep 11, 2019
View reviewed changes
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

You probably want to revert all unintended changes as @niklasad1 pointed out.

.gitlab-ci.yml Outdated Show resolved Hide resolved
@ordian ordian added this to the 2.7 milestone Sep 11, 2019
@dvdplm
Fix review grumbles
5ffa37f 
	Revert unwanted changes
	Tweak CI benchmark checks
ordian
ordian approved these changes Sep 11, 2019
View reviewed changes
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Good stuff

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 11, 2019
niklasad1
niklasad1 approved these changes Sep 11, 2019
View reviewed changes
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

👍

@ordian ordian merged commit f4d14e2 into master Sep 11, 2019
@ordian ordian deleted the dp/chore/add-benchmarks-to-verification branch September 11, 2019 12:15
dvdplm added a commit that referenced this pull request Sep 13, 2019
@dvdplm
Merge branch 'master' into dp/chore/new-ethereum-consensus-tests
bd0f53a 
* master: (70 commits)
  ethcore: remove `test-helper feat` from build (#11047)
  Include test-helpers from ethjson (#11045)
  [ethcore]: cleanup dependencies (#11043)
  add more tx tests (#11038)
  Fix parallel transactions race-condition (#10995)
  [ethcore]: make it compile without `test-helpers` feature (#11036)
  Benchmarks for block verification (#11035)
  Move snapshot related traits to their proper place (#11012)
  cleanup json crate (#11027)
  [spec] add istanbul test spec (#11033)
  [json-spec] make blake2 pricing spec more readable (#11034)
  Add blake2_f precompile (#11017)
  Add new line after writing block to hex file. (#10984)
  fix: remove unused error-chain (#11028)
  fix: remove needless use of itertools (#11029)
  Convert `std::test` benchmarks to use Criterion (#10999)
  Fix block detail updating (#11015)
  [trace] introduce trace failed to Ext (#11019)
  cli: update usage and version headers (#10924)
  [private-tx] remove unused rand (#11024)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@niklasad1 niklasad1 niklasad1 approved these changes

@ordian ordian ordian approved these changes

Assignees

@dvdplm dvdplm

Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
3 participants
@dvdplm @ordian @niklasad1