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
libnetwork/resolvconf: some cleaning up and optimisations #44234
Conversation
d3c060b
to
191d532
Compare
6ba5b9b
to
eb74235
Compare
eb74235
to
78f765e
Compare
78f765e
to
6140f52
Compare
6140f52
to
f53639d
Compare
@corhere ptal 🤗 |
func hashData(data []byte) []byte { | ||
f := sha256.Sum256(data) | ||
out := make([]byte, 2*sha256.Size) | ||
hex.Encode(out, f[:]) | ||
return append([]byte("sha256:"), out...) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be done with even fewer allocs:
func hashData(data []byte) []byte { | |
f := sha256.Sum256(data) | |
out := make([]byte, 2*sha256.Size) | |
hex.Encode(out, f[:]) | |
return append([]byte("sha256:"), out...) | |
} | |
const hashPrefix = "sha256:" | |
func hashData(data []byte) []byte { | |
out := make([]byte, len(hashPrefix)+hex.EncodedLen(sha256.Size)) | |
copy(out, hashPrefix) | |
f := sha256.Sum256(data) | |
hex.Encode(out[len(hashPrefix):], f[:]) | |
return out | |
} |
The code seemed overly complicated, requiring a reader to be constructed, where in all cases, the data was already available in a variable. This patch simplifies the utility to not require a reader, which also makes it a bit more performant: go install golang.org/x/perf/cmd/benchstat@latest GO111MODULE=off go test -run='^$' -bench=. -count=20 > old.txt GO111MODULE=off go test -run='^$' -bench=. -count=20 > new.txt benchstat old.txt new.txt name old time/op new time/op delta HashData-10 201ns ± 1% 128ns ± 1% -36.16% (p=0.000 n=18+20) name old alloc/op new alloc/op delta HashData-10 416B ± 0% 208B ± 0% -50.00% (p=0.000 n=20+20) name old allocs/op new allocs/op delta HashData-10 6.00 ± 0% 3.00 ± 0% -50.00% (p=0.000 n=20+20) A small change was made in `Build()`, which previously returned the resolv.conf data, even if the function failed to write it. In the new variation, `nil` is consistently returned on failures. Note that in various places, the hash is not even used, so we may be able to simplify things more after this. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
After my last change, I noticed that the hash is used as a []byte in most cases (other than tests). This patch updates the type to use a []byte, which (although unlikely very important) also improves performance: Compared to the previous version: benchstat new.txt new2.txt name old time/op new time/op delta HashData-10 128ns ± 1% 116ns ± 1% -9.77% (p=0.000 n=20+20) name old alloc/op new alloc/op delta HashData-10 208B ± 0% 88B ± 0% -57.69% (p=0.000 n=20+20) name old allocs/op new allocs/op delta HashData-10 3.00 ± 0% 2.00 ± 0% -33.33% (p=0.000 n=20+20) And compared to the original version: benchstat old.txt new2.txt name old time/op new time/op delta HashData-10 201ns ± 1% 116ns ± 1% -42.39% (p=0.000 n=18+20) name old alloc/op new alloc/op delta HashData-10 416B ± 0% 88B ± 0% -78.85% (p=0.000 n=20+20) name old allocs/op new allocs/op delta HashData-10 6.00 ± 0% 2.00 ± 0% -66.67% (p=0.000 n=20+20) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The test was assuming that the "source" file was always "/etc/resolv.conf", but the `Get()` function uses `Path()` to find the location of resolv.conf, which may be different. While at it, also changed some `t.Fatalf()` to `t.Errorf()`, and renamed some variables for clarity. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use t.TempDir() for convenience, and change some t.Fatal's to Errors, so that all tests can run instead of failing early. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Looks like the intent is to exclude windows (which wouldn't have /etc/resolv.conf nor systemd), but most tests would run fine elsewhere. This allows running the tests on macOS for local testing. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- Verify the content to be equal, not "contains"; this output should be predictable. - Also verify the content returned by the function to match. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
f53639d
to
56fbbde
Compare
Forgot about this one; did a rebase to get a fresh run of CI, and will merge when it completes (I might do a follow up with that last bit of optimisation from #44234 (comment)) |
Some Windows failures are unrelated (may be an issue with Docker Hub; there was a similar issue earlier today);
|
confirmed that those failures are unrelated; see #45415. bringing this one in 👍 |
See individual commits for details
- A picture of a cute animal (not mandatory but encouraged)