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.

Sign up for GitHub

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

Jump to bottom

labels/cidr: On the fly char replacement for IPv6 #28647

Merged
merged 1 commit into from Oct 20, 2023
Merged

labels/cidr: On the fly char replacement for IPv6 #28647

merged 1 commit into from Oct 20, 2023

Conversation

pippolo84
Copy link
Member

@pippolo84 pippolo84 commented Oct 17, 2023 •
edited

Suggested by @squeed here

When generating labels for IPv6 addresses, ':' characters need to be
replaced with '-'. Also, a leading or trailing '0' is added to avoid
labels starting or ending with '-'.  To avoid the allocation of an
additional string, replace the character on the fly while accumulating
the label key in the strings.Builder.

IPStringToLabel/0.0.0.0/0-8           104ns ±11%      94ns ± 4%  -10.25%  (p=0.008 n=5+5)
IPStringToLabel/192.0.2.3-8           146ns ± 3%     128ns ±27%     ~     (p=0.841 n=5+5)
IPStringToLabel/192.0.2.3/32-8        175ns ±11%     190ns ± 8%     ~     (p=0.056 n=5+5)
IPStringToLabel/192.0.2.3/24-8        176ns ±12%     167ns ± 7%     ~     (p=0.095 n=5+5)
IPStringToLabel/192.0.2.0/24-8        169ns ± 2%     162ns ± 1%   -4.08%  (p=0.008 n=5+5)
IPStringToLabel/::/0-8                309ns ± 2%     232ns ±11%  -24.91%  (p=0.008 n=5+5)
IPStringToLabel/fdff::ff-8            412ns ± 1%     264ns ± 1%  -35.90%  (p=0.008 n=5+5)
IPStringToLabel/f00d:42::ff/128-8     469ns ± 2%     305ns ± 1%  -34.96%  (p=0.016 n=5+4)
IPStringToLabel/f00d:42::ff/96-8      405ns ±11%     293ns ± 7%  -27.58%  (p=0.008 n=5+5)

name                               old alloc/op   new alloc/op   delta
IPStringToLabel/0.0.0.0/0-8           24.0B ± 0%     24.0B ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3-8           32.0B ± 0%     32.0B ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3/32-8        32.0B ± 0%     32.0B ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3/24-8        32.0B ± 0%     32.0B ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.0/24-8        32.0B ± 0%     32.0B ± 0%     ~     (all equal)
IPStringToLabel/::/0-8                16.0B ± 0%     16.0B ± 0%     ~     (all equal)
IPStringToLabel/fdff::ff-8            56.0B ± 0%     32.0B ± 0%  -42.86%  (p=0.008 n=5+5)
IPStringToLabel/f00d:42::ff/128-8     80.0B ± 0%     32.0B ± 0%  -60.00%  (p=0.008 n=5+5)
IPStringToLabel/f00d:42::ff/96-8      48.0B ± 0%     32.0B ± 0%  -33.33%  (p=0.008 n=5+5)

name                               old allocs/op  new allocs/op  delta
IPStringToLabel/0.0.0.0/0-8            2.00 ± 0%      2.00 ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3-8            2.00 ± 0%      2.00 ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3/32-8         2.00 ± 0%      2.00 ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3/24-8         2.00 ± 0%      2.00 ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.0/24-8         2.00 ± 0%      2.00 ± 0%     ~     (all equal)
IPStringToLabel/::/0-8                 3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
IPStringToLabel/fdff::ff-8             5.00 ± 0%      3.00 ± 0%  -40.00%  (p=0.008 n=5+5)
IPStringToLabel/f00d:42::ff/128-8      5.00 ± 0%      3.00 ± 0%  -40.00%  (p=0.008 n=5+5)
IPStringToLabel/f00d:42::ff/96-8       3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)

Depends on #28465

@pippolo84 pippolo84 added area/daemon Impacts operation of the Cilium daemon. kind/performance There is a performance impact of this. 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 mentioned this pull request Oct 17, 2023
Merged
@pippolo84 pippolo84 requested a review from squeed October 17, 2023 12:48
@pippolo84
labels/cidr: On the fly char replacement for IPv6
7f9e2e4 
When generating labels for IPv6 addresses, ':' characters need to be
replaced with '-'. Also, a leading or trailing '0' is added to avoid
labels starting or ending with '-'.  To avoid the allocation of an
additional string, replace the character on the fly while accumulating
the label key in the strings.Builder.

IPStringToLabel/0.0.0.0/0-8           104ns ±11%      94ns ± 4%  -10.25%  (p=0.008 n=5+5)
IPStringToLabel/192.0.2.3-8           146ns ± 3%     128ns ±27%     ~     (p=0.841 n=5+5)
IPStringToLabel/192.0.2.3/32-8        175ns ±11%     190ns ± 8%     ~     (p=0.056 n=5+5)
IPStringToLabel/192.0.2.3/24-8        176ns ±12%     167ns ± 7%     ~     (p=0.095 n=5+5)
IPStringToLabel/192.0.2.0/24-8        169ns ± 2%     162ns ± 1%   -4.08%  (p=0.008 n=5+5)
IPStringToLabel/::/0-8                309ns ± 2%     232ns ±11%  -24.91%  (p=0.008 n=5+5)
IPStringToLabel/fdff::ff-8            412ns ± 1%     264ns ± 1%  -35.90%  (p=0.008 n=5+5)
IPStringToLabel/f00d:42::ff/128-8     469ns ± 2%     305ns ± 1%  -34.96%  (p=0.016 n=5+4)
IPStringToLabel/f00d:42::ff/96-8      405ns ±11%     293ns ± 7%  -27.58%  (p=0.008 n=5+5)

name                               old alloc/op   new alloc/op   delta
IPStringToLabel/0.0.0.0/0-8           24.0B ± 0%     24.0B ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3-8           32.0B ± 0%     32.0B ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3/32-8        32.0B ± 0%     32.0B ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3/24-8        32.0B ± 0%     32.0B ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.0/24-8        32.0B ± 0%     32.0B ± 0%     ~     (all equal)
IPStringToLabel/::/0-8                16.0B ± 0%     16.0B ± 0%     ~     (all equal)
IPStringToLabel/fdff::ff-8            56.0B ± 0%     32.0B ± 0%  -42.86%  (p=0.008 n=5+5)
IPStringToLabel/f00d:42::ff/128-8     80.0B ± 0%     32.0B ± 0%  -60.00%  (p=0.008 n=5+5)
IPStringToLabel/f00d:42::ff/96-8      48.0B ± 0%     32.0B ± 0%  -33.33%  (p=0.008 n=5+5)

name                               old allocs/op  new allocs/op  delta
IPStringToLabel/0.0.0.0/0-8            2.00 ± 0%      2.00 ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3-8            2.00 ± 0%      2.00 ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3/32-8         2.00 ± 0%      2.00 ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.3/24-8         2.00 ± 0%      2.00 ± 0%     ~     (all equal)
IPStringToLabel/192.0.2.0/24-8         2.00 ± 0%      2.00 ± 0%     ~     (all equal)
IPStringToLabel/::/0-8                 3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
IPStringToLabel/fdff::ff-8             5.00 ± 0%      3.00 ± 0%  -40.00%  (p=0.008 n=5+5)
IPStringToLabel/f00d:42::ff/128-8      5.00 ± 0%      3.00 ± 0%  -40.00%  (p=0.008 n=5+5)
IPStringToLabel/f00d:42::ff/96-8       3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 force-pushed the pr/pippolo84/ip-to-label-improvement branch from 0db21b6 to 7f9e2e4 Compare October 18, 2023 07:58
@pippolo84 pippolo84 marked this pull request as ready for review October 18, 2023 08:01
@pippolo84 pippolo84 requested review from a team as code owners October 18, 2023 08:01
@pippolo84
Copy link
Member Author

/test

@pippolo84 pippolo84 mentioned this pull request Oct 19, 2023
Closed
@pippolo84
Copy link
Member Author

Conformance EKS failure tracked here, rerunning.

@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 19, 2023
@joestringer joestringer merged commit b448f92 into cilium:main Oct 20, 2023
60 of 62 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
Closed
6 tasks
@pippolo84 pippolo84 mentioned this pull request Nov 2, 2023
Merged
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
Merged
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 backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Nov 2, 2023
@pippolo84 pippolo84 mentioned this pull request Nov 7, 2023
Merged
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
@thorn3r thorn3r mentioned this pull request Nov 9, 2023
Merged
This was referenced Nov 12, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@squeed squeed squeed approved these changes

Assignees
No one assigned
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pippolo84 @nathanjsweet @squeed @joestringer