cli/command/formatter: sort published ports numerically by IP - #7144
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
18f640d to
6c83546
Compare
|
|
||
| if i.IP != j.IP { | ||
| return i.IP.String() < j.IP.String() | ||
| return i.IP.Less(j.IP) |
There was a problem hiding this comment.
I wondered why this was there, but looks like previously IP was just a basic string, so when it changed, this was probably used as closest match (f81816e);
There was a problem hiding this comment.
Yes, I think you're right.
| b.ReportAllocs() | ||
| b.ResetTimer() | ||
|
|
||
| for i := 0; i < b.N; i++ { |
There was a problem hiding this comment.
nit; we can probably use for b.Loop() { now
| func BenchmarkDisplayablePorts(b *testing.B) { | ||
| // Every port shares a container port, so | ||
| // each comparison falls through to the | ||
| // host-IP comparison. | ||
| // Descending order keeps the input unsorted. | ||
| const ( | ||
| sharedContainerPort = 80 | ||
| firstHostPort = 30000 | ||
| ) |
There was a problem hiding this comment.
I'm a bit on the fence on the benchmark; it's nice, but maybe a bit too much for this code (and mostly we're now benchmarking stdlib).
@vvoland WDYT?
There was a problem hiding this comment.
Yeah I don't think it's worth benchmarking
There was a problem hiding this comment.
Okay, I'll remove it right now.
There was a problem hiding this comment.
Thanks! It was useful to show the difference, but probably "too much" for this part of the code.
(appreciate the work! ❤️)
There was a problem hiding this comment.
You're welcome! And no worries.
`comparePorts` compared host IPs with `i.IP.String() < j.IP.String()`, which sorts them lexicographically, so "10.0.0.2" ordered before "9.0.0.1" in `docker ps` output. This also avoids formatting both addresses as strings on every comparison. ports before after 64 1540 allocs/op 394 allocs/op 256 5364 allocs/op 1548 allocs/op That's a ~74% (3.9x) reduction in allocations. Signed-off-by: Hamir <hirehamir@gmail.com>
6c83546 to
2dd7b7e
Compare
4 participants
- What I did
Fixed #7143 -
docker psordering published ports by the string form of the host IP rather than numerically, so10.0.0.2was listed before9.0.0.1.- How I did it
comparePortscompared IPs withPortSummary.IPis anetip.Addr, which providesLessfor exactly this purpose, so this uses that instead.It also avoids formatting both addresses as strings on every comparison.
- How to verify it
Before:
127.0.0.10:8080->80/tcp, 127.0.0.9:8081->80/tcpAfter:
127.0.0.9:8081->80/tcp, 127.0.0.10:8080->80/tcpA new case in
TestDisplayablePortscovers this.That's a ~74% (3.9x) reduction in allocations in Docker CLI's port-sorting path.
- Human readable description for the release notes