Skip to content

encoding/base32: increase performance and code reuse #30376

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
Closed
wants to merge 5 commits into from
Closed

encoding/base32: increase performance and code reuse #30376

wants to merge 5 commits into from

Conversation

svent
Copy link
Contributor

@svent svent commented Feb 24, 2019 •
edited
Loading

Add benchmarks for the Encode/Decode functions operating on []byte and increase decoding performance by removing the calls to strings.Map/bytes.Map and reusing the newline filtering code that is used by NewDecoder.
Cut allocations in half for DecodeString.

Comparison using the new benchmarks:
name old time/op new time/op delta
Encode 16.7µs ± 1% 17.0µs ± 2% +2.25% (p=0.000 n=9+9)
EncodeToString 21.1µs ± 1% 20.9µs ± 1% -0.96% (p=0.000 n=10+10)
Decode 141µs ± 1% 54µs ± 1% -61.51% (p=0.000 n=10+10)
DecodeString 81.4µs ± 0% 54.7µs ± 1% -32.79% (p=0.000 n=9+10)

name old speed new speed delta
Encode 492MB/s ± 1% 481MB/s ± 2% -2.19% (p=0.000 n=9+9)
EncodeToString 389MB/s ± 1% 392MB/s ± 1% +0.97% (p=0.000 n=10+10)
Decode 93.0MB/s ± 1% 241.6MB/s ± 1% +159.82% (p=0.000 n=10+10)
DecodeString 161MB/s ± 0% 240MB/s ± 1% +48.78% (p=0.000 n=9+10)

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Feb 24, 2019
@gopherbot
Copy link
Contributor

This PR (HEAD: 3a9ab89) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/163598 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Agniva De Sarker:

Patch Set 1:

Can you compare your benchmarks using benchstat (https://godoc.org/golang.org/x/perf/cmd/benchstat) after running the benchmarks with a count of 10 ?

Also, can you check if stripNewlines inlines ? Ideally, we wouldn't want to lose performance in (*NewDecoder).Read.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Andrew Bonventre:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Sven Taute:

Patch Set 3:

(1 comment)

I updated the benchmark comparison using benchstat with 10 runs.

Thanks for the hint regarding inlining - unfortunately, stripNewlines does not get inlined. I just did some benchmarks, this degrades performance by about 4%. I am not sure whether I should duplicate the code because of this, it seems that most packages using base32 do not use NewDecoder.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 3:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 0d6c6a0) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/163598 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Sven Taute:

Patch Set 4:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Agniva De Sarker:

Patch Set 4:

(4 comments)

Please update the benchmarks with the new code. Thanks.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 8e8bdbb) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/163598 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Sven Taute:

Patch Set 6:

(4 comments)

I addressed PS4 and updated the benchmark comparison.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Agniva De Sarker:

Patch Set 6: Run-TryBot+1

(1 comment)

Over to Brad for the final decision.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 6:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=ce95c155

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 6: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 6:

(3 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Agniva De Sarker:

Patch Set 6:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 6:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Agniva De Sarker:

Patch Set 6:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 6:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Agniva De Sarker:

Patch Set 6:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from shankar shivanna:

Patch Set 6:

ok

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 6:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Sven Taute:

Patch Set 6:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@svent svent force-pushed the base32-performance branch from 8e8bdbb to 7b7737d Compare March 10, 2019 16:31
@gopherbot
Copy link
Contributor

This PR (HEAD: 7b7737d) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/163598 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Sven Taute:

Patch Set 8:

(4 comments)

I changed stripNewlines so it accepts a src and dst, which might be the same backing array. It will now always perform a copy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Agniva De Sarker:

Patch Set 8:

Sven, would you like to do a rebase with the latest master and update the benchmarks once more so that we have the latest numbers ?

I have also added Ian who can help with review.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@svent svent force-pushed the base32-performance branch from 7b7737d to f4be3cf Compare September 9, 2019 21:52
@gopherbot
Copy link
Contributor

This PR (HEAD: f4be3cf) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/163598 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Sven Taute:

Patch Set 10: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Sven Taute:

Patch Set 10:

Patch Set 8:

Sven, would you like to do a rebase with the latest master and update the benchmarks once more so that we have the latest numbers ?

I have also added Ian who can help with review.

Thanks for taking this up again.
I did a rebase and updated the benchmarks.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 12: Patch Set 11 was rebased

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ian Lance Taylor:

Patch Set 12: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 12:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=b908ed16

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 12: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/163598.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Sep 11, 2019
@svent @ianlancetaylor
encoding/base32: increase performance and code reuse
8ef6d6a 
Add benchmarks for the Encode/Decode functions operating on []byte and increase decoding performance by removing the calls to strings.Map/bytes.Map and reusing the newline filtering code that is used by NewDecoder.
Cut allocations in half for DecodeString.

Comparison using the new benchmarks:
name            old time/op    new time/op     delta
Encode            16.7µs ± 1%     17.0µs ± 2%    +2.25%  (p=0.000 n=9+9)
EncodeToString    21.1µs ± 1%     20.9µs ± 1%    -0.96%  (p=0.000 n=10+10)
Decode             141µs ± 1%       54µs ± 1%   -61.51%  (p=0.000 n=10+10)
DecodeString      81.4µs ± 0%     54.7µs ± 1%   -32.79%  (p=0.000 n=9+10)

name            old speed      new speed       delta
Encode           492MB/s ± 1%    481MB/s ± 2%    -2.19%  (p=0.000 n=9+9)
EncodeToString   389MB/s ± 1%    392MB/s ± 1%    +0.97%  (p=0.000 n=10+10)
Decode          93.0MB/s ± 1%  241.6MB/s ± 1%  +159.82%  (p=0.000 n=10+10)
DecodeString     161MB/s ± 0%    240MB/s ± 1%   +48.78%  (p=0.000 n=9+10)

Change-Id: Id53633514a9e14ecd0389d52114b2b8ca64370cb
GitHub-Last-Rev: f4be3cf
GitHub-Pull-Request: #30376
Reviewed-on: https://go-review.googlesource.com/c/go/+/163598
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/163598 has been merged.

@gopherbot gopherbot closed this Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@svent @gopherbot @googlebot