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.

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

Use crypto/sha256 #29386

Merged
merged 3 commits into from
Feb 25, 2024
Merged

Use crypto/sha256 #29386

merged 3 commits into from
Feb 25, 2024

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Feb 24, 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

@KN4CK3R KN4CK3R added type/refactoring Existing code has been cleaned up. There should be no new functionality. performance/speed performance issues with slow downs labels Feb 24, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 24, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 24, 2024
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/migrations labels Feb 24, 2024
@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 24, 2024
@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 24, 2024
@lunny
Copy link
Member

lunny commented Feb 24, 2024

but the improvement is only for amd64?

@wxiaoguang
Copy link
Contributor

IIRC it lacks arm64 optimization.

lunny
lunny previously requested changes Feb 25, 2024
View reviewed changes
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

#29386 (comment)

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Feb 25, 2024
@silverwind
Copy link
Member

silverwind commented Feb 25, 2024 •
edited
Loading

I would still use stdlib over a dependency even if it means slightly worse perf on the other archs as those are bound to eventually improve on other archs. amd64 likely accounts for 95%+ of installations. Also, is sha256 really a bottleneck for gitea?

@wxiaoguang
Copy link
Contributor

I didn't mean objection or blocker (I could be neutral for it). Yes, sha256 is not a bottleneck for gitea, but I just didn't find a reason worth to do so:

  1. Does the stdlib improve performance? No, and maybe it degrades for arm64 instances.
  2. Does the stdlib improve maintainability? IMO no because the minio version is a 100% drop-in replacement, it could be easily replaced at any time.
  3. Does it drop the minio dependency or decrease binary side? No, because as long as minio package still uses their sha256 package, the code is still there ....

So, I just didn't know whether it is worth to use stdlib to replace minio version.

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Feb 25, 2024

Comparing https://github.com/minio/sha256-simd/blob/master/sha256block_arm64.s with https://cs.opensource.google/go/go/+/refs/tags/go1.22.0:src/crypto/sha256/sha256block_arm64.s they use the same intrinsics so they could have similar performance. Does someone have an arm64 machine and could do a benchmark?

@wxiaoguang
Copy link
Contributor

Comparing https://github.com/minio/sha256-simd/blob/master/sha256block_arm64.s with https://cs.opensource.google/go/go/+/refs/tags/go1.22.0:src/crypto/sha256/sha256block_arm64.s they use the same intrinsics so they could have similar performance. Does someone have an arm64 machine and could do a benchmark?

Is it a new enhancement in Go 1.22? In Go 1.21, they only mentions "amd64".

If the stdlib also have the same performance, then I don't have any question.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Feb 25, 2024
@silverwind
Copy link
Member

silverwind commented Feb 25, 2024 •
edited
Loading

I would prefer stdlib because it will likely be better maintained long-term and they also have better processes in place for security issues and such.

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Feb 25, 2024

Is it a new enhancement in Go 1.22? In Go 1.21, they only mentions "amd64".

No, minio announced 2016 their improved ARM64 support. A year later hardware support for ARM64 was added to Go too (https://cs.opensource.google/go/go/+/7b8a7f8272fd1941a199af1adb334bd9996e8909) So both should have similar performance since Go 1.10 (?) With Go 1.21 they just added support for sha-ni for x64 cpus.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 25, 2024
@lunny lunny enabled auto-merge (squash) February 25, 2024 12:45
@lunny lunny merged commit f79c9e8 into go-gitea:main Feb 25, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Feb 25, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 25, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 26, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
2575481 
* giteaofficial/main: (45 commits)
  Include resource state events in Gitlab downloads (go-gitea#29382)
  Add API to get PR by base/head (go-gitea#29242)
  [skip ci] Updated translations via Crowdin
  Improve Documentation for Restoration from backup (go-gitea#29321)
  Refactor "user/active" related logic (go-gitea#29390)
  Remove jQuery AJAX from the archive download links (go-gitea#29380)
  Add tailwindcss (go-gitea#29357)
  Add missing space (go-gitea#29393)
  Integrate alpine `noarch` packages into other architectures index (go-gitea#29137)
  enforce maxlength in frontend (go-gitea#29389)
  Remove incorrect and unnecessary Escape from templates (go-gitea#29394)
  Make actions animation rotate counterclockwisely (go-gitea#29378)
  Use `crypto/sha256` (go-gitea#29386)
  Add `io.Closer` guidelines (go-gitea#29387)
  Remove jQuery AJAX from the notice selection deletion button (go-gitea#29381)
  Refactor Safe modifier (go-gitea#29392)
  Add attachment support for code review comments (go-gitea#29220)
  Refactor modules/git global variables (go-gitea#29376)
  Remove jQuery from the code diff expansion buttons (go-gitea#29385)
  Remove jQuery AJAX from the markdown editor preview (go-gitea#29384)
  ...
@KN4CK3R KN4CK3R deleted the refactor-crypto-sha256 branch February 28, 2024 21:28
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@silverwind silverwind silverwind approved these changes

@lunny lunny lunny left review comments

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/migrations performance/speed performance issues with slow downs size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
6 participants
@KN4CK3R @lunny @wxiaoguang @silverwind @delvh @GiteaBot