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

sha256: drop minio in favor of crypto/sha256 for go1.21 and above #173

Merged
merged 2 commits into from
May 24, 2023

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented May 24, 2023

In go1.21 the go std has a new SHANI accelerated rountine (just like minio's sha256). See golang/go#50543.
Reduce the code surface by dropping what is now a redondent librairy.

Benchmarks using go1.21 and go1.20 shows no significant difference between minio and the std for sha256:

name                         old time/op    new time/op    delta
SumAllLarge/sha2-256-12        8.63ms ± 3%    8.63ms ± 1%      ~     (p=0.220 n=20+17)
SumAllLarge/sha2-512-12        23.1ms ± 3%    23.0ms ± 3%      ~     (p=0.361 n=18+20)
SumAllLarge/dbl-sha2-256-12    36.0ms ± 4%     8.6ms ± 4%   -76.16%  (p=0.000 n=20+20)

name                         old speed      new speed      delta
SumAllLarge/sha2-256-12      1.94GB/s ± 4%  1.95GB/s ± 1%      ~     (p=0.220 n=20+17)
SumAllLarge/sha2-512-12       725MB/s ± 3%   729MB/s ± 3%      ~     (p=0.350 n=18+20)
SumAllLarge/dbl-sha2-256-12   467MB/s ± 4%  1958MB/s ± 4%  +319.53%  (p=0.000 n=20+20)

dlb-sha2-256 quadrupled in speed because it was never wired to use minio's sha256 implementation and thus
was brought back up to speed by crypto/sha256's improvement.

I think we should remove github.com/multiformats/go-multihash/register/miniosha256 one go1.22 is released (~1 year) since it would be purposeless, we could also stub it to github.com/multiformats/go-multihash/register/sha256 for all versions but no one imports it directly.

@Jorropo Jorropo self-assigned this May 24, 2023
@Jorropo Jorropo force-pushed the no-minio branch 4 times, most recently from 335109a to 75fa368 Compare May 24, 2023 10:17
In go1.21 the go std has a new SHANI accelerated rountine (just like minio's sha256). See golang/go#50543.
Reduce the code surface by dropping what is now a redondent librairy.

Benchmarks using go1.21 and go1.20 shows no significant difference between minio and the std for sha256:
```
name                         old time/op    new time/op    delta
SumAllLarge/sha2-256-12        8.63ms ± 3%    8.63ms ± 1%      ~     (p=0.220 n=20+17)
SumAllLarge/sha2-512-12        23.1ms ± 3%    23.0ms ± 3%      ~     (p=0.361 n=18+20)
SumAllLarge/dbl-sha2-256-12    36.0ms ± 4%     8.6ms ± 4%   -76.16%  (p=0.000 n=20+20)

name                         old speed      new speed      delta
SumAllLarge/sha2-256-12      1.94GB/s ± 4%  1.95GB/s ± 1%      ~     (p=0.220 n=20+17)
SumAllLarge/sha2-512-12       725MB/s ± 3%   729MB/s ± 3%      ~     (p=0.350 n=18+20)
SumAllLarge/dbl-sha2-256-12   467MB/s ± 4%  1958MB/s ± 4%  +319.53%  (p=0.000 n=20+20)
```
`dlb-sha2-256` quadrupled in speed because it was never wired to use minio's sha256 implementation and thus
was brought back up to speed by `crypto/sha256`'s improvement.

I think we should remove `github.com/multiformats/go-multihash/register/miniosha256` one go1.22 is released (~1 year) since it would be purposeless,
we could also stub it to github.com/multiformats/go-multihash/register/sha256 for all versions but [no one imports it directly](https://pkg.go.dev/github.com/multiformats/go-multihash/register/miniosha256?tab=importedby).
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Exciting to see this kind of speedup in the standard library!

register/sha256/sha256.go Show resolved Hide resolved
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Discussed, this is fine since go-multihash is the package importing miniosha256.

@Jorropo Jorropo merged commit d2f43bc into multiformats:master May 24, 2023
@Jorropo Jorropo deleted the no-minio branch May 24, 2023 10:48
Jorropo added a commit to Jorropo/go-libp2p that referenced this pull request May 24, 2023
Jorropo added a commit to Jorropo/go-libp2p that referenced this pull request Aug 16, 2023
Jorropo added a commit to Jorropo/go-libp2p that referenced this pull request Aug 16, 2023
marten-seemann pushed a commit to libp2p/go-libp2p that referenced this pull request Aug 17, 2023
lunny pushed a commit to go-gitea/gitea that referenced this pull request Feb 25, 2024
Go 1.21 improved the performance of `crypto/sha256`. It's now similar to
`minio/sha256-simd`, so we should just use the standard libs.

https://go.dev/doc/go1.21#crypto/sha256
https://go-review.googlesource.com/c/go/+/408795
multiformats/go-multihash#173
gts2030 pushed a commit to superblock-dev/go-libp2p that referenced this pull request May 23, 2024
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.

2 participants