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

x/crypto/ed25519: Use sync.Pool to reuse hash.Hash #30302

Open
gbrlsnchs opened this issue Feb 18, 2019 · 4 comments
Open

x/crypto/ed25519: Use sync.Pool to reuse hash.Hash #30302

gbrlsnchs opened this issue Feb 18, 2019 · 4 comments

Comments

@gbrlsnchs
Copy link

@gbrlsnchs gbrlsnchs commented Feb 18, 2019

In Ed25519 package, every signing and verifying calls create a new SHA-512 hash.Hash by calling sha512.New.

Thinking of scenarios where multiple calls are made to those functions (e.g., in an HTTP handler), using a sync.Pool to store those hash.Hash and reuse them would optimize both performance and memory usage.

@gopherbot gopherbot added this to the Unreleased milestone Feb 18, 2019
@dgryski
Copy link
Contributor

@dgryski dgryski commented Feb 18, 2019

In concrete terms, it's an allocation of 216 bytes that can't be avoided.

https://play.golang.org/p/X_ADMMwHUul

@agnivade
Copy link
Contributor

@agnivade agnivade commented Feb 19, 2019

@gbrlsnchs
Copy link
Author

@gbrlsnchs gbrlsnchs commented May 10, 2019

@dgryski Memory-wise it's not a concern (still it's an improvement), however it would reduce GC work, wouldn't it? If that's the case, on a large scale that would be beneficial.

@lukechampine
Copy link
Contributor

@lukechampine lukechampine commented Mar 9, 2020

Relatedly, the arguments to Sign and Verify also end up escaping to the heap, because they are passed as arguments to a hash.Hash interface. While we wait for progress on #33160, could we fix this by exporting sha512.digest and then doing a type assert?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants