Skip to content

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

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
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
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Unreleased

Comments

@gbrlsnchs
Copy link

gbrlsnchs commented Feb 18, 2019 •
edited
Loading

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 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

/cc @FiloSottile

@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 19, 2019
@gbrlsnchs
Copy link
Author

gbrlsnchs commented May 10, 2019 •
edited
Loading

@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

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
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@dgryski @agnivade @lukechampine @dsnet @gbrlsnchs @gopherbot