libnet/internal/resolvconf: optimize Generate() without text/template#50428
libnet/internal/resolvconf: optimize Generate() without text/template#50428thaJeztah merged 4 commits intomoby:masterfrom
Conversation
739097a to
ef126dc
Compare
ef126dc to
d93c53d
Compare
|
Thanks for opening this! We will definitely need the buildkit version in 29. Should we just take this in here and in a buildkit fork for 29 and what repo might this logic move to eventually? |
d93c53d to
b5b37b5
Compare
| return fmt.Sprintf("host(%s)", ed.Addr) | ||
| return "host(" + ed.Addr.String() + ")" |
There was a problem hiding this comment.
Updated this one in the last iteration as well to shave off some bits;
Before/after;
BenchmarkGenerate-10 2617477 453 ns/op 736 B/op 8 allocs/op
BenchmarkGenerate-10 3148328 373 ns/op 712 B/op 7 allocs/op
| fields := strings.Fields(line) | ||
|
|
||
| // Strip blank lines and comments. | ||
| if len(fields) == 0 || fields[0][0] == '#' || fields[0][0] == ';' { | ||
| if line == "" || line[0] == '#' || line[0] == ';' { | ||
| return | ||
| } | ||
|
|
||
| fields := strings.Fields(line) | ||
| if len(fields) == 0 { |
There was a problem hiding this comment.
Pushed an extra commit for these; it occurred to me that we were calling string.Fields() before checking if it was an empty line or comment; better to skip early.
| k, v, hasSep := strings.Cut(opt, ":") | ||
| if k == "ndots" { | ||
| if !hasSep || v == "" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Similarly here; no need to strconv.Atoi() empty values.
| b.WriteString(rc.md.Header + "\n") | ||
| b.WriteByte('\n') |
There was a problem hiding this comment.
Probably not worth changing, but ...
| b.WriteString(rc.md.Header + "\n") | |
| b.WriteByte('\n') | |
| b.WriteString(rc.md.Header) | |
| b.WriteString("\n\n") |
There was a problem hiding this comment.
I went back-and-forth a bit on this one; contemplated to just append \n\n to the write string, but then chose to stick with a separate b.WriteByte('\n') after it, to align more with all other places we insert a newline; I'll probably keep this one as-is for now.
goos: darwin
goarch: arm64
pkg: github.com/docker/docker/daemon/libnetwork/internal/resolvconf
cpu: Apple M1 Pro
BenchmarkGenerate
BenchmarkGenerate-10 42550 27439 ns/op 18083 B/op 394 allocs/op
PASS
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Rewrite this function to not use text/template, which is ~74× faster,
~25× less memory, and ~56× fewer allocations.
Before/After:
BenchmarkGenerate-10 42550 27439 ns/op 18083 B/op 394 allocs/op
BenchmarkGenerate-10 3148328 373 ns/op 712 B/op 7 allocs/op
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
CI is green (Windows failure is unrelated); I'll push that last nit, and merge when build is happy |
b5b37b5 to
60a3a28
Compare
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
| type errSystem struct{ error } | ||
| // systemErr implements [github.com/docker/docker/errdefs.ErrSystem]. | ||
| type systemErr struct{ error } |
There was a problem hiding this comment.
As CI is restarting; I pushed one more commit that made linting fail on BuildKit; only changing the name of the type, so should have no impact otherwise.
|
Build and unit-tests are happy; bringing this one in |
libnet/internal/resolvconf: optimize Generate() without text/template
Rewrite this function to not use text/template, which is ~74× faster,
~25× less memory, and ~56× fewer allocations.
Before/After:
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)