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.

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

Add benchmark for ssz.Root.equals #2710

Merged
merged 1 commit into from
Jun 17, 2021
Merged

Add benchmark for ssz.Root.equals #2710

merged 1 commit into from
Jun 17, 2021

Conversation

twoeths
Copy link
Contributor

@twoeths twoeths commented Jun 16, 2021 •
edited

Motivation

  • byteArrayEquals is better than ssz.Root.equals in terms of performance

Description

Added benchmark result:

// Compare state root
// ================================================================
// ssz.Root.equals                                                        17262.51 ops/s      57.92900 us/op 256558 runs    15.01 s
// ssz.Root.equals with valueOf()                                         61656.08 ops/s      16.21900 us/op 903088 runs    15.03 s
// byteArrayEquals with valueOf()                                         848896.4 ops/s      1.178000 us/op 9904067 runs    15.38 s

Closes #2708

Update

  • Only add performance test, ssz.Root.equals is improved in ssz

@github-actions github-actions bot added Api scope-networking All issues related to networking, gossip, and libp2p. labels Jun 16, 2021
@codeclimate
Copy link

codeclimate bot commented Jun 16, 2021 •
edited

Code Climate has analyzed commit 2cb1f40 and detected 0 issues on this pull request.

View more on Code Climate.

@twoeths twoeths marked this pull request as ready for review June 16, 2021 09:49
mpetrunic
mpetrunic reviewed Jun 16, 2021
View reviewed changes
Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

Can we make ssz.Root use byteArrayEquals instead? 😄

@wemeetagain wemeetagain mentioned this pull request Jun 16, 2021
Merged
@wemeetagain
Copy link
Member

Can we make ssz.Root use byteArrayEquals instead? smile

We can! I ran the benchmark with ChainSafe/ssz#130 and #2713 applied, got these results:

Compare state root                                                                                                                      
================================================================                                                                        
ssz.Root.equals                                                         1101322 ops/s      908.0000 ns/op 12559206 runs    15.51 s      
ssz.Root.equals with valueOf()                                         731528.9 ops/s      1.367000 us/op 8654589 runs    15.37 s       
byteArrayEquals with valueOf()                                         785546.0 ops/s      1.273000 us/op 9585978 runs    15.39 s

dapplion
dapplion previously approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Looks good, but why is the ssz lib version so slow?? Can we implement an optimization there too?

@3xtr4t3rr3str14l
Copy link
Contributor

since #2713 is merged do we not need this PR anymore?

@wemeetagain
Copy link
Member

since #2713 is merged do we not need this PR anymore?

We should still commit the perf tests here

@twoeths twoeths changed the title Use byteArrayEquals Add benchmark for ssz.Root.equals Jun 17, 2021
@wemeetagain wemeetagain merged commit 645689e into master Jun 17, 2021
@wemeetagain wemeetagain deleted the tuyen/byteArrayEquals branch June 17, 2021 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpetrunic mpetrunic mpetrunic left review comments

@wemeetagain wemeetagain wemeetagain approved these changes

@dapplion dapplion dapplion left review comments

Assignees
No one assigned
Labels
scope-networking All issues related to networking, gossip, and libp2p.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ssz.Root.equals performance
5 participants
@twoeths @wemeetagain @3xtr4t3rr3str14l @mpetrunic @dapplion