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

Use minio/sha256-simd for accelerated SHA256 #23052

Merged
merged 4 commits into from
Feb 22, 2023

Conversation

zeripath
Copy link
Contributor

minio/sha256-simd provides additional acceleration for SHA256 using AVX512, SHA Extensions for x86 and ARM64 for ARM.

It provides a drop-in replacement for crypto/sha256 and if the extensions are not available it falls back to standard crypto/sha256.

minio/sha256-simd provides additional acceleration for SHA256 using AVX512, SHA Extensions for x86 and ARM64 for ARM.

It provides a drop-in replacement for crypto/sha256 and if the extensions are not available
it falls back to standard crypto/sha256.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

Clearly this is not a complete drop-in replacement... It appears to be missing the implementation for BinaryEncoder...

OK...

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 22, 2023
@zeripath
Copy link
Contributor Author

I think once minio/sha256-simd#68 is merged there's potential for further improvement here.

@zeripath
Copy link
Contributor Author

Given the CI failure in c304241 that means that the CI runners should have support for these extensions.

That means that this is likely to cause a measurable improvement in speed of CI.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 22, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 22, 2023
@delvh delvh added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 22, 2023
@silverwind
Copy link
Member

silverwind commented Feb 22, 2023

I wonder why they did not just upstream the improvements into crypto.

I'm generally wary of such "speed improvements" as the code quality often does not match the standards of the stdlib as seen by various broken JSON implementations, for example.

@jolheiser jolheiser added this to the 1.20.0 milestone Feb 22, 2023
@zeripath
Copy link
Contributor Author

They're trying to upstream the changes but go is being a little slow to pull them.

Agreed in general about speed improvements not necessarily being worth it, but this is being used by minio and is written by minio and has a good test suite.

@techknowlogick techknowlogick merged commit 1319ba6 into go-gitea:main Feb 22, 2023
@jolheiser jolheiser removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 22, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 23, 2023
* giteaofficial/main:
  Improve reverse proxies documentation (go-gitea#23068)
  Improve accessibility for issue comments (go-gitea#22612)
  Wrap unless-check in docker manifests (go-gitea#23079)
  Add accessibility to the menu on the navbar (go-gitea#23059)
  Use minio/sha256-simd for accelerated SHA256 (go-gitea#23052)
  Fix some more hidden problems (go-gitea#23074)
  Add sillyguodong to maintainers (go-gitea#23067)
  Improving CONTRIBUTING.md for backport details (go-gitea#23057)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. performance/cpu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants