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

avoid double alloc in NewCidV1 #132

Merged
merged 1 commit into from
Sep 12, 2021
Merged

avoid double alloc in NewCidV1 #132

merged 1 commit into from
Sep 12, 2021

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Sep 7, 2021

(see commit message)

We allocate once via "make([]byte, len)",
and again when that buffer is converted to a string.

Thankfully, since Go 1.10 we have strings.Builder,
designed specifically for this use case.

In a downstream benchmark in go-car,
which needs to reconstruct many CID values,
we see small but nice gains:

    name           old time/op    new time/op    delta
    ReadBlocks-16    1.09ms ± 4%    1.06ms ± 5%   -3.33%  (p=0.007 n=11+11)

    name           old speed      new speed      delta
    ReadBlocks-16   478MB/s ± 4%   494MB/s ± 5%   +3.46%  (p=0.007 n=11+11)

    name           old alloc/op   new alloc/op   delta
    ReadBlocks-16    1.30MB ± 0%    1.25MB ± 0%   -3.86%  (p=0.000 n=12+12)

    name           old allocs/op  new allocs/op  delta
    ReadBlocks-16     9.50k ± 0%     8.45k ± 0%  -11.05%  (p=0.000 n=12+12)
Copy link
Contributor

@ribasushi ribasushi left a comment

Choose a reason for hiding this comment

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

This looks sane.

Although I wonder: Multihash is a []byte, and PutUvarint woks on []byte as well. Would it not be even faster to:

  • stack-allocate something reasonable ( ~80-something bytes, you need to account for obscenely large codecs+512bit hashes )
  • if stuff fits - work on the above, otherwise make() the right size
  • write the 1 + uvarint + copy the mh
  • string() the entire resulting thing

@mvdan
Copy link
Contributor Author

mvdan commented Sep 7, 2021

Possibly? I guess it's a tradeoff between "copy uvarint bytes around" and "use more stack space and possibly allocate if our stack space isn't big enough". It's hard to make the call without stats. I think that copying the PutUvarint bytes (most likely just a few bytes, max 10) is cheap enough that we shouldn't pay any attention to it :) The new NewCidV1 is faster than the old one, anyway.

@mvdan mvdan merged commit 5640b01 into master Sep 12, 2021
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.

3 participants