Skip to content
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

labels: Use slices.Sort instead of sort.Strings #28649

Merged

Conversation

pippolo84
Copy link
Member

As suggested by package "sort" documentation (https://pkg.go.dev/sort#Strings), slices.Sort should be preferred to sort.Strings since it runs faster.

Comparison between sort.Strings ("old") and slices.Sort ("new"):

name                        old time/op    new time/op    delta
pkg:github.com/cilium/cilium/pkg/labels goos:linux goarch:amd64
Labels_SortedList-8            787ns ± 4%     736ns ±12%   -6.42%  (p=0.014 n=7+8)
pkg:github.com/cilium/cilium/pkg/labels/cidr goos:linux goarch:amd64
Labels_SortedListCIDRIDs-8    5.22µs ± 8%    4.67µs ± 4%  -10.63%  (p=0.000 n=10+10)

name                        old alloc/op   new alloc/op   delta
pkg:github.com/cilium/cilium/pkg/labels goos:linux goarch:amd64
Labels_SortedList-8             360B ± 0%      336B ± 0%   -6.67%  (p=0.000 n=10+10)
pkg:github.com/cilium/cilium/pkg/labels/cidr goos:linux goarch:amd64
Labels_SortedListCIDRIDs-8    1.62kB ± 0%    1.60kB ± 0%   -1.48%  (p=0.000 n=10+10)

name                        old allocs/op  new allocs/op  delta
pkg:github.com/cilium/cilium/pkg/labels goos:linux goarch:amd64
Labels_SortedList-8             3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.000 n=10+10)
pkg:github.com/cilium/cilium/pkg/labels/cidr goos:linux goarch:amd64
Labels_SortedListCIDRIDs-8      3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.000 n=10+10)

As suggested by package "sort" documentation
(https://pkg.go.dev/sort#Strings), slices.Sort should be preferred to
sort.Strings since it runs faster.

Comparison between sort.Strings ("old") and slices.Sort ("new"):

name                        old time/op    new time/op    delta
pkg:github.com/cilium/cilium/pkg/labels goos:linux goarch:amd64
Labels_SortedList-8            787ns ± 4%     736ns ±12%   -6.42%  (p=0.014 n=7+8)
pkg:github.com/cilium/cilium/pkg/labels/cidr goos:linux goarch:amd64
Labels_SortedListCIDRIDs-8    5.22µs ± 8%    4.67µs ± 4%  -10.63%  (p=0.000 n=10+10)

name                        old alloc/op   new alloc/op   delta
pkg:github.com/cilium/cilium/pkg/labels goos:linux goarch:amd64
Labels_SortedList-8             360B ± 0%      336B ± 0%   -6.67%  (p=0.000 n=10+10)
pkg:github.com/cilium/cilium/pkg/labels/cidr goos:linux goarch:amd64
Labels_SortedListCIDRIDs-8    1.62kB ± 0%    1.60kB ± 0%   -1.48%  (p=0.000 n=10+10)

name                        old allocs/op  new allocs/op  delta
pkg:github.com/cilium/cilium/pkg/labels goos:linux goarch:amd64
Labels_SortedList-8             3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.000 n=10+10)
pkg:github.com/cilium/cilium/pkg/labels/cidr goos:linux goarch:amd64
Labels_SortedListCIDRIDs-8      3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.000 n=10+10)

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 added area/daemon Impacts operation of the Cilium daemon. kind/performance There is a performance impact of this. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. release-note/misc This PR makes changes that have no direct user impact. area/fqdn Affects the FQDN policies feature labels Oct 17, 2023
@pippolo84 pippolo84 requested review from a team as code owners October 17, 2023 13:15
@pippolo84
Copy link
Member Author

/test

@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 17, 2023
@joestringer joestringer merged commit d6e3eda into cilium:main Oct 17, 2023
62 of 63 checks passed
@pippolo84 pippolo84 added needs-backport/1.12 affects/v1.12 This issue affects v1.12 branch needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Nov 1, 2023
@derailed derailed mentioned this pull request Nov 2, 2023
6 tasks
@pippolo84 pippolo84 mentioned this pull request Nov 2, 2023
2 tasks
@pippolo84 pippolo84 added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Nov 2, 2023
@pippolo84 pippolo84 mentioned this pull request Nov 2, 2023
2 tasks
@pippolo84 pippolo84 added backport-pending/1.12 backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed needs-backport/1.12 labels Nov 2, 2023
@pippolo84 pippolo84 removed the backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. label Nov 6, 2023
@pippolo84 pippolo84 mentioned this pull request Nov 7, 2023
2 tasks
@pippolo84 pippolo84 added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Nov 7, 2023
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch area/daemon Impacts operation of the Cilium daemon. area/fqdn Affects the FQDN policies feature backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/performance There is a performance impact of this. 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. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants