Improve performance of v1.NewHash#2194
Conversation
Previously, NewHash() made multiple unnecessary heap allocations that have been removed. I've also added a simple benchmark test to measure the impact. Results below: Before: ``` go test ./pkg/v1 -run XXX -bench BenchmarkNewHash goos: darwin goarch: arm64 pkg: github.com/google/go-containerregistry/pkg/v1 cpu: Apple M1 Pro BenchmarkNewHash-10 8977393 123.1 ns/op 160 B/op 2 allocs/op PASS ok github.com/google/go-containerregistry/pkg/v1 1.711s ``` After: ``` $ go test ./pkg/v1 -run XXX -bench BenchmarkNewHash goos: darwin goarch: arm64 pkg: github.com/google/go-containerregistry/pkg/v1 cpu: Apple M1 Pro BenchmarkNewHash-10 15214291 76.09 ns/op 0 B/op 0 allocs/op PASS ok github.com/google/go-containerregistry/pkg/v1 1.805s ```
2d90a7c to
baf6dbc
Compare
| func (h *Hash) parse(unquoted string) error { | ||
| parts := strings.Split(unquoted, ":") | ||
| if len(parts) != 2 { | ||
| algo, body, ok := strings.Cut(unquoted, ":") |
There was a problem hiding this comment.
First removed allocation: strings.Split creates a slice but we know we expect two parts.
There was a problem hiding this comment.
Previously, sha256:abc:def would have failed. Do we still have that behavior with this change?
There was a problem hiding this comment.
@imjasonh thanks for looking, yes! But just to be sure, I've added additional cases to TestBadHashes.
| var wantBytes int | ||
| switch algo { | ||
| case "sha256": | ||
| wantBytes = crypto.SHA256.Size() |
There was a problem hiding this comment.
Second removed allocation: Hasher calls sha256.New(), which can be avoided if we only need the size.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2194 +/- ##
===========================================
- Coverage 71.67% 53.06% -18.61%
===========================================
Files 123 164 +41
Lines 9935 10946 +1011
===========================================
- Hits 7121 5809 -1312
- Misses 2115 4431 +2316
- Partials 699 706 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Previously, NewHash() made multiple unnecessary heap allocations that I have been removed in this PR. I've also added a simple benchmark test to measure the impact. Results below:
Before:
After: