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

storage: use sha256-simd from minio #347

Merged
merged 1 commit into from
Dec 29, 2021
Merged

Conversation

rchincha
Copy link
Contributor

Signed-off-by: Ramkumar Chinchani rchincha@cisco.com

What type of PR is this?
performance enchancement

Which issue does this PR fix:
#306

What does this PR do / Why do we need it:
Uses the sha_ni extensions for CPUs that support it.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>
@codecov
Copy link

codecov bot commented Dec 24, 2021

Codecov Report

Merging #347 (dda9b16) into main (8183e14) will increase coverage by 0.00%.
The diff coverage is 66.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #347   +/-   ##
=======================================
  Coverage   80.28%   80.28%           
=======================================
  Files          46       46           
  Lines        6294     6296    +2     
=======================================
+ Hits         5053     5055    +2     
  Misses        873      873           
  Partials      368      368           
Impacted Files Coverage Δ
pkg/storage/storage_fs.go 80.02% <66.66%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8183e14...dda9b16. Read the comment docs.

@rchincha
Copy link
Contributor Author

Linux kernel already has included support for this.
http://lkml.iu.edu/hypermail/linux/kernel/1511.0/00383.html

Copy link
Collaborator

@adodon2go adodon2go left a comment

Choose a reason for hiding this comment

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

LGTM
I did not have CPU support for this new feature on my VM (both locally and in the lab). The improvement is working only on bare metal? Did you have chance to try this on a machine that supports the feature and does the benchmarking shows a huge improvement for large blobs? Currently it takes between 3-4 sec to compute hash for blob with size 1000MB (includes the reading of the file which I guess takes a lot) using crypto/sha256 (fallback from sha256-simd without CPU SHA features support). Maybe there is still room for improvements here like caching the digest when the blob is first uploaded into .uploads (for systems that does not have CPU support for sha acceleration).

adodon@adodon-cs-bld:/data/ssd/work/benchmark$ go test -bench=. -benchtime=60s
goos: linux
goarch: amd64
pkg: github.com/benchmark
cpu: Intel(R) Xeon(R) Gold 5218 CPU @ 2.30GHz
BenchmarkNewSha100MB-20 205 350024972 ns/op
BenchmarkNewSha1000MB-20 19 3503338046 ns/op
PASS
ok github.com/benchmark 177.248s

adodon@adodon-cs-bld:/data/ssd/work/benchmark$ go test -bench=. -benchtime=60s
goos: linux
goarch: amd64
pkg: github.com/benchmark
cpu: Intel(R) Xeon(R) Gold 5218 CPU @ 2.30GHz
BenchmarkNewSha100MB-20 205 350024972 ns/op
BenchmarkNewSha1000MB-20 19 3503338046 ns/op
PASS
ok github.com/benchmark 177.248s

@rchincha rchincha merged commit cac7fe4 into project-zot:main Dec 29, 2021
@rchincha
Copy link
Contributor Author

#347 (review)
^ make sure "lscpu | grep sha_ni" is found.
Raw sha_ni improvement is huge (3-4X), but again the total time for zot use case will depend on whether network latency dominates.

vrajashkr pushed a commit to vrajashkr/zot that referenced this pull request Apr 11, 2024
…roject-zot#347)

* test(test-data): add layers information to the image metadata json

Signed-off-by: Andrei Aaron <aaaron@luxoft.com>

* fix(tests): fix username userd as password, fix prerequisite validation

Signed-off-by: Andrei Aaron <aaaron@luxoft.com>

* fix(tests): auto-confirm cosign upload to private registry

Signed-off-by: Andrei Aaron <aaaron@luxoft.com>

---------

Signed-off-by: Andrei Aaron <aaaron@luxoft.com>
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.

None yet

2 participants