nat: optimizations for SortPortMap#147
Conversation
Add a minimal benchmark for SortPortMap
goos: darwin
goarch: arm64
pkg: github.com/docker/go-connections/nat
cpu: Apple M1 Pro
BenchmarkSortPortMap
BenchmarkSortPortMap-10 4366 272020 ns/op 97817 B/op 1757 allocs/op
PASS
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- Skip ParsePortRange as intermediate (as we don't need an uint64)
- Use strings.Equalfold, which doesn't allocate.
No real diff in benchmark;
BenchmarkSortPortMap-10 4366 272020 ns/op 97817 B/op 1757 allocs/op
BenchmarkSortPortMap-10 4484 265482 ns/op 97818 B/op 1757 allocs/op
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Slightly more memory, but ~40% faster;
BenchmarkSortPortMap-10 4484 265482 ns/op 97818 B/op 1757 allocs/op
BenchmarkSortPortMap-10 6727 174735 ns/op 108873 B/op 1757 allocs/op
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Before/after:
BenchmarkSortPortMap-10 6727 174735 ns/op 108873 B/op 1757 allocs/op
BenchmarkSortPortMap-10 8618 137899 ns/op 70073 B/op 523 allocs/op
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
5154610 to
f65d0cd
Compare
|
FWIW, I suspect there's a bug in the sorting algorithm, or at least it's surprising that the ports are not sorted by port-number first. But there's some odd logic where the mapped port is taken into account (and used for sorting); I vaguely recall I ran into that in the past, and kept the behavior, but wasn't sure if correct. go-connections/nat/sort_test.go Lines 61 to 74 in fa986a4 |
There was a problem hiding this comment.
Pull Request Overview
This pull request optimizes the SortPortMap function through a series of incremental improvements that collectively reduce execution time by approximately 50% and allocations by ~70%. The changes include memoizing port parsing results, using pointers to reduce allocations, and replacing less efficient string operations.
- Add benchmark to measure SortPortMap performance
- Cache parsed port numbers and protocols to avoid repeated parsing
- Use pointer references and more efficient string comparisons to reduce allocations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| nat/sort_test.go | Adds comprehensive benchmark for SortPortMap with realistic test data |
| nat/sort.go | Implements performance optimizations including memoization, pointer usage, and efficient string operations |
| s = append(s, portMapEntry{port: p, binding: b}) | ||
| b := b // capture loop variable for go < 1.22 | ||
| s = append(s, portMapEntry{ | ||
| port: p, binding: &b, |
There was a problem hiding this comment.
The loop variable capture comment is misleading. In Go 1.22+, loop variables are automatically captured per iteration, but this code creates a new variable 'b' that shadows the original 'b' from the range. Consider using a different variable name like 'binding := b' to make the intent clearer.
| port: p, binding: &b, | |
| binding := b // create a new variable for clarity | |
| s = append(s, portMapEntry{ | |
| port: p, binding: &binding, |
There was a problem hiding this comment.
I deliberately did this because using someVar := someVar is a common pattern for this (e.g. tc := tc in table-tests).
| if err != nil { | ||
| i = 0 | ||
| } | ||
| func toInt(s string) int { |
There was a problem hiding this comment.
The return type changed from uint64 to int but the function is used to parse port numbers which are inherently non-negative. Consider keeping uint64 or using a specific port number type to maintain type safety and prevent potential issues with negative values.
| func toInt(s string) int { | |
| func toInt(s string) uint64 { |
There was a problem hiding this comment.
Parsing validates the ports to be in a valid range (for ports) and non-negative. Using an int makes handling these more convenient (e.g. using strconv.Itoa() instead of more verbose options like strconv.FormatUint(value, 10)
austinvazquez
left a comment
There was a problem hiding this comment.
Wow nice results. Changes LGTM.
nat: add BenchmarkSortPortMap
Add a minimal benchmark for SortPortMap
nat: portMapSorter: slight optimization
No real diff in benchmark;
nat: portMapSorter: memoize port(int) and proto
Slightly more memory, but ~40% faster;
nat: SortPortMap: use pointer to reduce allocs
Before/after:
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)