Skip to content

lib/sha256: Remove it #9643

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
Merged
merged 1 commit into from
Aug 10, 2024
Merged

lib/sha256: Remove it #9643

merged 1 commit into from
Aug 10, 2024

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Aug 10, 2024 •
edited
Loading

Purpose

Remove the lib/sha256 package, because it's no longer necessary. Go's standard library now has the same performance and is on par with sha256-simd since Since Go 1.21. Therefore using sha256-simd has no benefits anymore.

ARM already has optimized sha256 assembly code since golang/go@7b8a7f8, sha256-simd published their results before that optimized assembly was implemented, minio/sha256-simd@f941fed. The assembly looks very similar and the benchmarks in the Go commit match that of sha256-simd.

This patch removes all of the related code of lib/sha256 and makes crypto/sha256 the 'default'.

Benchmark of sha256-simd and crypto/sha256:

cpu: AMD Ryzen 5 3600X 6-Core Processor
                │  simd.txt   │               go.txt                │
                │   sec/op    │    sec/op     vs base               │
Hash/8Bytes-12    63.25n ± 1%    73.38n ± 1%  +16.02% (p=0.002 n=6)
Hash/64Bytes-12   98.73n ± 1%   105.30n ± 1%   +6.65% (p=0.002 n=6)
Hash/1K-12        567.2n ± 1%    572.8n ± 1%   +0.99% (p=0.002 n=6)
Hash/8K-12        4.062µ ± 1%    4.062µ ± 1%        ~ (p=0.396 n=6)
Hash/1M-12        512.1µ ± 0%    510.6µ ± 1%        ~ (p=0.485 n=6)
Hash/5M-12        2.556m ± 1%    2.564m ± 0%        ~ (p=0.093 n=6)
Hash/10M-12       5.112m ± 0%    5.127m ± 0%        ~ (p=0.093 n=6)
geomean           13.82µ         14.27µ        +3.28%

                │   simd.txt   │               go.txt                │
                │     B/s      │     B/s       vs base               │
Hash/8Bytes-12    120.6Mi ± 1%   104.0Mi ± 1%  -13.81% (p=0.002 n=6)
Hash/64Bytes-12   618.2Mi ± 1%   579.8Mi ± 1%   -6.22% (p=0.002 n=6)
Hash/1K-12        1.682Gi ± 1%   1.665Gi ± 1%   -0.98% (p=0.002 n=6)
Hash/8K-12        1.878Gi ± 1%   1.878Gi ± 1%        ~ (p=0.310 n=6)
Hash/1M-12        1.907Gi ± 0%   1.913Gi ± 1%        ~ (p=0.485 n=6)
Hash/5M-12        1.911Gi ± 1%   1.904Gi ± 0%        ~ (p=0.093 n=6)
Hash/10M-12       1.910Gi ± 0%   1.905Gi ± 0%        ~ (p=0.093 n=6)
geomean           1.066Gi        1.032Gi        -3.18%

Testing

Compiled and tested on Linux.

Documentation

syncthing/docs#874

@Gusted
lib/sha256: Remove it
d0b601f 
Remove the `lib/sha256` package, because it's no longer necessary. Go's
standard library now has the same performance and is on par with
`sha256-simd` since [Since Go
1.21](golang/go@1a64574).
Therefore using `sha256-simd` has no benefits anymore.

ARM already has optimized sha256 assembly code since
golang/go@7b8a7f8,
`sha256-simd` published their results before that optimized assembly was
implemented, minio/sha256-simd@f941fed.
The assembly looks very similar and the benchmarks in the Go commit
match that of `sha256-simd`.

This patchs removes all of the related code of `lib/sha256` and makes
`crypto/sha256` the 'default'.

Benchmark of `sha256-simd` and `crypto/sha256`:
```
cpu: AMD Ryzen 5 3600X 6-Core Processor
                │  simd.txt   │               go.txt                │
                │   sec/op    │    sec/op     vs base               │
Hash/8Bytes-12    63.25n ± 1%    73.38n ± 1%  +16.02% (p=0.002 n=6)
Hash/64Bytes-12   98.73n ± 1%   105.30n ± 1%   +6.65% (p=0.002 n=6)
Hash/1K-12        567.2n ± 1%    572.8n ± 1%   +0.99% (p=0.002 n=6)
Hash/8K-12        4.062µ ± 1%    4.062µ ± 1%        ~ (p=0.396 n=6)
Hash/1M-12        512.1µ ± 0%    510.6µ ± 1%        ~ (p=0.485 n=6)
Hash/5M-12        2.556m ± 1%    2.564m ± 0%        ~ (p=0.093 n=6)
Hash/10M-12       5.112m ± 0%    5.127m ± 0%        ~ (p=0.093 n=6)
geomean           13.82µ         14.27µ        +3.28%

                │   simd.txt   │               go.txt                │
                │     B/s      │     B/s       vs base               │
Hash/8Bytes-12    120.6Mi ± 1%   104.0Mi ± 1%  -13.81% (p=0.002 n=6)
Hash/64Bytes-12   618.2Mi ± 1%   579.8Mi ± 1%   -6.22% (p=0.002 n=6)
Hash/1K-12        1.682Gi ± 1%   1.665Gi ± 1%   -0.98% (p=0.002 n=6)
Hash/8K-12        1.878Gi ± 1%   1.878Gi ± 1%        ~ (p=0.310 n=6)
Hash/1M-12        1.907Gi ± 0%   1.913Gi ± 1%        ~ (p=0.485 n=6)
Hash/5M-12        1.911Gi ± 1%   1.904Gi ± 0%        ~ (p=0.093 n=6)
Hash/10M-12       1.910Gi ± 0%   1.905Gi ± 0%        ~ (p=0.093 n=6)
geomean           1.066Gi        1.032Gi        -3.18%
```
@rasa rasa mentioned this pull request Aug 10, 2024
Merged
calmh
calmh approved these changes Aug 10, 2024
View reviewed changes
Copy link
Member

@calmh calmh left a comment

Choose a reason for hiding this comment

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

Nice

@imsodin
Copy link
Member

imsodin commented Aug 10, 2024

That go ticket only talks about one intel instructions, while sha256-simd talks about multiple and also ARM specific things. Are we sure we aren't losing optimisations for some hardware by dropping this?

imsodin
imsodin requested changes Aug 10, 2024
View reviewed changes
Copy link
Member

@imsodin imsodin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Actually I'll make this blocking. I don't have much prior knowledge on sha256 hardware optimization, so it's very possible I am just entirely wrong, I'd just like some acknowledgement that this does indeed cover all improvements made by sha256-simd, not just one (to a reasonable extent of certainty of course, something being missed is always possible).

Examples of what I picked up (again, just dumb pattern matching, I haven't looked into what any of this means exactly):

https://github.com/minio/sha256-simd?tab=readme-ov-file#arm-sha-extensions

calmh pushed a commit to syncthing/docs that referenced this pull request Aug 10, 2024
Remove STHASHING environment (#874)
5ffb9f2 
Complementary of syncthing/syncthing#9643
@calmh
Copy link
Member

calmh commented Aug 10, 2024

At least on macOS ARM64 it's the same.

[I6KAH] 2024/08/10 11:36:30 INFO: Single thread SHA256 performance is 2504 MB/s using crypto/sha256 (2494 MB/s using minio/sha256-simd).

@AudriusButkevicius
Copy link
Member

AudriusButkevicius commented Aug 10, 2024 •
edited
Loading

Go version for amd64 doesn't seem to use native instructions, just uses avx2, where as minio does use explicit sha extensions.
This is only likely be visible on more modern cpus.

imsodin
imsodin approved these changes Aug 10, 2024
View reviewed changes
Copy link
Member

@imsodin imsodin left a comment

Choose a reason for hiding this comment

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

Thanks for testing Jakob, was about to try to run sha256-simd benchs myself on arm :)

Also just found this quote by the author of sha256-simd, in answer to Audrius saying the package is obsolte:

Correct, it is obsolete.

minio/sha256-simd#72 (comment)

@AudriusButkevicius
Copy link
Member

lemme just verify quickly on my machine

@imsodin
Copy link
Member

imsodin commented Aug 10, 2024 •
edited
Loading

Ah yeah, I just missed Audrius' message before approving myself. I should have just retracted my request for changes review instead of approving, that would have represented my intent better :)

@AudriusButkevicius
Copy link
Member 

[2ERMY] 2024/08/10 12:37:04 INFO: Single thread SHA256 performance is 1740 MB/s using crypto/sha256 (1740 MB/s using minio/sha256-simd).

so we're good

@AudriusButkevicius AudriusButkevicius merged commit 356c505 into syncthing:main Aug 10, 2024
20 of 21 checks passed
@Gusted Gusted deleted the rm-minio-sha256 branch August 10, 2024 12:54
@calmh calmh added this to the v1.27.11 milestone Aug 13, 2024
calmh added a commit to calmh/syncthing that referenced this pull request Aug 19, 2024
@calmh
Merge branch 'main' into indexrefactor2
4392474 
* main: (46 commits)
  build: use Go 1.23, require minimum 1.22 (syncthing#9651)
  gui, man, authors: Update docs, translations, and contributors
  lib/fs: Put the caseFS as the outermost layer (syncthing#9648)
  gui: Add Irish (ga) translation template (syncthing#9646)
  gui, man, authors: Update docs, translations, and contributors
  lib/syncthing: Add wrapper for access to model (syncthing#9627)
  cli: Remove `go-shlex` dependency (syncthing#9644)
  lib/sha256: Remove it (syncthing#9643)
  build: Update dependencies (syncthing#9640)
  Chmod -x non-executable files (fixes syncthing#9629) (syncthing#9630)
  gui, man, authors: Update docs, translations, and contributors
  all: minimal set of changes for iOS app (syncthing#9619)
  gui, man, authors: Update docs, translations, and contributors
  gui, man, authors: Update docs, translations, and contributors
  etc: Remove restart on suspend systemd service (ref syncthing#8448) (syncthing#9611)
  gui, man, authors: Update docs, translations, and contributors
  lib/fs: Add missing locks to fakeFile methods (fixes syncthing#9499) (syncthing#9603)
  lib/api: Increase test request timeout (fixes syncthing#9455) (syncthing#9602)
  gui, man, authors: Update docs, translations, and contributors
  lib/ignore: Remove unused patterns in cache (syncthing#9601)
  ...
@yonas
Copy link

yonas commented Sep 4, 2024

@Gusted Would it be possible for you to add BLAKE-3 to your comparison?

@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Aug 10, 2025
@syncthing syncthing locked and limited conversation to collaborators Aug 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@calmh calmh calmh approved these changes

@AudriusButkevicius AudriusButkevicius AudriusButkevicius approved these changes

@imsodin imsodin imsodin approved these changes

Assignees
No one assigned
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Milestone
v1.27.11
Development

Successfully merging this pull request may close these issues.
6 participants
@Gusted @imsodin @calmh @AudriusButkevicius @yonas @st-review