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.

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

crypto/merkle: pre-allocate data slice in innherHash #6443

Merged
merged 2 commits into from
May 12, 2021
Merged

crypto/merkle: pre-allocate data slice in innherHash #6443

merged 2 commits into from
May 12, 2021

Conversation

cuonglm
Copy link
Contributor

@cuonglm cuonglm commented May 10, 2021

So we can reduce pressure on runtime for checking that slice has enough
capacity before appending.

Benchmark result shows less memory usage and faster runtime:

name                          old time/op    new time/op    delta
HashAlternatives/recursive-8    25.2µs ± 1%    19.6µs ± 1%  -22.07%  (p=0.000 n=10+9)
HashAlternatives/iterative-8    24.2µs ± 1%    19.0µs ± 0%  -21.28%  (p=0.000 n=10+10)

name                          old alloc/op   new alloc/op   delta
HashAlternatives/recursive-8    25.4kB ± 0%    19.1kB ± 0%  -24.92%  (p=0.000 n=10+10)
HashAlternatives/iterative-8    28.1kB ± 0%    21.8kB ± 0%  -22.54%  (p=0.000 n=10+10)

name                          old allocs/op  new allocs/op  delta
HashAlternatives/recursive-8       497 ± 0%       398 ± 0%  -19.92%  (p=0.000 n=10+10)
HashAlternatives/iterative-8       498 ± 0%       399 ± 0%  -19.88%  (p=0.000 n=10+10)

Fixes #6442

@cuonglm
Copy link
Contributor Author

cuonglm commented May 10, 2021

cc @odeke-em

@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #6443 (98e2d28) into master (811dbe4) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6443      +/-   ##
==========================================
+ Coverage   60.92%   60.97%   +0.05%     
==========================================
  Files         287      287              
  Lines       27087    27091       +4     
==========================================
+ Hits        16502    16518      +16     
+ Misses       8883     8870      -13     
- Partials     1702     1703       +1     
Impacted Files Coverage Δ
crypto/merkle/hash.go 100.00% <100.00%> (ø)
privval/signer_server.go 89.47% <0.00%> (-5.27%) ⬇️
libs/clist/clist.go 63.09% <0.00%> (-3.58%) ⬇️
p2p/pex/reactor.go 82.17% <0.00%> (-2.98%) ⬇️
consensus/wal.go 59.00% <0.00%> (-1.25%) ⬇️
p2p/switch.go 60.18% <0.00%> (-0.47%) ⬇️
mempool/reactor.go 74.46% <0.00%> (ø)
consensus/state.go 66.87% <0.00%> (+0.27%) ⬆️
p2p/router.go 78.07% <0.00%> (+0.48%) ⬆️
p2p/conn/connection.go 79.06% <0.00%> (+0.55%) ⬆️
... and 6 more

Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

Could you add a changelog entry as well please? We like to give credit to contributors.

@cuonglm
Copy link
Contributor Author

cuonglm commented May 10, 2021

Could you add a changelog entry as well please? We like to give credit to contributors.

Done

Copy link
Contributor

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @cuonglm!

@odeke-em
Copy link
Contributor

One other thing we could do much later on after this PR is to add a sync.Pool to reuse slices since the byte slices are passed into tmhash.Sum and read-only.

crypto/merkle/hash.go Outdated Show resolved Hide resolved
return tmhash.Sum(append(innerPrefix, append(left, right...)...))
data := make([]byte, 0, len(innerPrefix)+len(left)+len(right))
n := copy(data, innerPrefix)
n = copy(data[n:], left)
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh oh, this is invalid, it has to be a: n += copy(data[n:], left) otherwise you'll just have the length of left

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, just realized it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, somehow, the copy always return 0...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay, we need to pre-allocate all slice elem 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then the running time is nearly the same, compare using append vs copy:

name                          old time/op    new time/op    delta
HashAlternatives/recursive-8    22.8µs ± 0%    22.6µs ± 0%  -0.69%  (p=0.000 n=10+10)
HashAlternatives/iterative-8    21.9µs ± 0%    21.7µs ± 1%  -0.75%  (p=0.000 n=9+9)

So we can reduce pressure on runtime for checking that slice has enough
capacity before appending.

Benchmark result shows less memory usage and faster runtime:

name                          old time/op    new time/op    delta
HashAlternatives/recursive-8    25.2µs ± 1%    22.6µs ± 0%   -9.98%  (p=0.000 n=10+10)
HashAlternatives/iterative-8    24.2µs ± 1%    21.7µs ± 1%  -10.29%  (p=0.000 n=10+9)

name                          old alloc/op   new alloc/op   delta
HashAlternatives/recursive-8    25.4kB ± 0%    19.1kB ± 0%  -24.92%  (p=0.000 n=10+10)
HashAlternatives/iterative-8    28.1kB ± 0%    21.8kB ± 0%  -22.54%  (p=0.000 n=10+10)

name                          old allocs/op  new allocs/op  delta
HashAlternatives/recursive-8       497 ± 0%       398 ± 0%  -19.92%  (p=0.000 n=10+10)
HashAlternatives/iterative-8       498 ± 0%       399 ± 0%  -19.88%  (p=0.000 n=10+10)

Fixes #6442
@odeke-em
Copy link
Contributor

@marbar3778 @tychoish please help us merge lest the PR gets stale. Thank you.

@cmwaters cmwaters merged commit a9fc0c3 into tendermint:master May 12, 2021
JayT106 pushed a commit to JayT106/tendermint that referenced this pull request Sep 16, 2022
So we can reduce pressure on runtime for checking that slice has enough
capacity before appending.
mergify bot pushed a commit that referenced this pull request Sep 21, 2022
Cherry-picking PR #6443

#### PR checklist

- [x] Tests written/updated, or no tests needed
- [x] `CHANGELOG_PENDING.md` updated, or no changelog entry needed
- [x] Updated relevant documentation (`docs/`) and code comments, or no
      documentation updates needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto/merkle: innherHash use more memory than necessary
5 participants