Skip to content

Conversation

bimmlerd
Copy link
Member

@bimmlerd bimmlerd commented Oct 1, 2024

A few small drive-by improvements to pkg/ip.

ip: remove unneeded IPTestSuite

Likely left over from a conversion from checkmate to go testing.
ip: benchmark SortAddrList

To create a baseline for the next commit.
ip: use stdlib slices package

This gives us a speedup of 30% in a microbenchmark for sorting IPs and
moves us closer to stdlib.

pkg: github.com/cilium/cilium/pkg/ip
                │     pre      │                post                 │
                │    sec/op    │   sec/op     vs base                │
SortAddrList-10   105.06µ ± 4%   72.75µ ± 1%  -30.75% (p=0.000 n=10)
slices: drop SortedUniqueFunc

The type constraints of slices.Sort and slices.Compact are different -
the former requires cmp.Ordered whereas the latter only needs
comparable. The only caller for SortedUniqueFunc was pkg/ip, for
[]netip.Addr. The usage is inefficient, though, since netip.Addr is
comparable, just not cmp.Ordered - the use of slices.CompactFunc is thus
unnecessary.

Since there's no consumers, and use can be needlessly inefficient, let's
just drop the function, even though it breaks API symmetry.

Likely left over from a conversion from checkmate to go testing.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
To create a baseline for the next commit.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
@bimmlerd bimmlerd requested a review from a team as a code owner October 1, 2024 06:49
@bimmlerd bimmlerd requested a review from sayboras October 1, 2024 06:49
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 1, 2024
@bimmlerd bimmlerd added the release-note/misc This PR makes changes that have no direct user impact. label Oct 1, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 1, 2024
This gives us a speedup of 30% in a microbenchmark for sorting IPs and
moves us closer to stdlib.

pkg: github.com/cilium/cilium/pkg/ip
                │     pre      │                post                 │
                │    sec/op    │   sec/op     vs base                │
SortAddrList-10   105.06µ ± 4%   72.75µ ± 1%  -30.75% (p=0.000 n=10)

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/minor-ip-fixes branch from 23e78a1 to 661d068 Compare October 1, 2024 07:08
@bimmlerd
Copy link
Member Author

bimmlerd commented Oct 1, 2024

/test

@bimmlerd bimmlerd changed the title Pr/bimmlerd/minor ip fixes minor pkg/ip fixes Oct 1, 2024
@bimmlerd bimmlerd requested a review from a team as a code owner October 1, 2024 08:15
@bimmlerd bimmlerd requested a review from ovidiutirla October 1, 2024 08:15
The type constraints of slices.Sort and slices.Compact are different -
the former requires cmp.Ordered whereas the latter only needs
comparable. The only caller for SortedUniqueFunc was pkg/ip, for
[]netip.Addr. The usage is inefficient, though, since netip.Addr is
comparable, just not cmp.Ordered - the use of slices.CompactFunc is thus
unnecessary.

Since there's no consumers, and use can be needlessly inefficient, let's
just drop the function, even though it breaks API symmetry.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/minor-ip-fixes branch from 9475d22 to 4606ccb Compare October 1, 2024 08:27
Copy link
Contributor

@ovidiutirla ovidiutirla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the performance benchmark and the number of usages, agree to remove the SortedUniqueFunc.

@bimmlerd
Copy link
Member Author

bimmlerd commented Oct 1, 2024

/test

Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@tklauser tklauser enabled auto-merge October 1, 2024 09:26
@tklauser tklauser added this pull request to the merge queue Oct 1, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 1, 2024
Merged via the queue into cilium:main with commit 8166d02 Oct 1, 2024
62 of 63 checks passed
@bimmlerd bimmlerd deleted the pr/bimmlerd/minor-ip-fixes branch October 2, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants