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

pkg/policy/api: Optimize FQDNSelector String() #19570

Merged

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented Apr 26, 2022

Use strings.Builder instead of fmt.Sprintf() and preallocate the size of
the string so that Go doesn't need to over-allocate if the string ends
up longer than what the buffer growth algorithm predicts.

Results:

$ go test -v -run '^$' -bench 'BenchmarkFQDNSelectorString' -benchmem ./pkg/policy/api > old.txt
$ go test -v -run '^$' -bench 'BenchmarkFQDNSelectorString' -benchmem ./pkg/policy/api > new.txt
$ benchcmp old.txt new.txt
benchmark                         old ns/op     new ns/op     delta
BenchmarkFQDNSelectorString-8     690           180           -73.97%

benchmark                         old allocs     new allocs     delta
BenchmarkFQDNSelectorString-8     9              4              -55.56%

benchmark                         old bytes     new bytes     delta
BenchmarkFQDNSelectorString-8     288           208           -27.78%

Signed-off-by: Chris Tarazi chris@isovalent.com

Related: #19571

Use strings.Builder instead of fmt.Sprintf() and preallocate the size of
the string so that Go doesn't need to over-allocate if the string ends
up longer than what the buffer growth algorithm predicts.

Results:

```
$ go test -v -run '^$' -bench 'BenchmarkFQDNSelectorString' -benchmem ./pkg/policy/api > old.txt
$ go test -v -run '^$' -bench 'BenchmarkFQDNSelectorString' -benchmem ./pkg/policy/api > new.txt
$ benchcmp old.txt new.txt
benchmark                         old ns/op     new ns/op     delta
BenchmarkFQDNSelectorString-8     690           180           -73.97%

benchmark                         old allocs     new allocs     delta
BenchmarkFQDNSelectorString-8     9              4              -55.56%

benchmark                         old bytes     new bytes     delta
BenchmarkFQDNSelectorString-8     288           208           -27.78%
```

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi requested a review from a team as a code owner April 26, 2022 00:48
@christarazi christarazi added area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. 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. labels Apr 26, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 26, 2022
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Nice find!

Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

🚢

@christarazi
Copy link
Member Author

christarazi commented Apr 26, 2022

/test

Job 'Cilium-PR-K8s-1.23-kernel-net-next' hit: #18895 (94.74% similarity)

@christarazi
Copy link
Member Author

Runtme hit #19598. Rest of CI failures are flakes. Marking ready to merge since we have approving reviews as well.

@christarazi christarazi added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. needs-backport/1.11 labels Apr 27, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.5 Apr 27, 2022
@joestringer joestringer merged commit 4c2c244 into cilium:master Apr 27, 2022
@christarazi christarazi deleted the pr/christarazi/optimize-fqdn-selector branch April 27, 2022 18:08
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.11.5 May 2, 2022
@aditighag aditighag added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels May 4, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.11 in 1.11.5 May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. backport-done/1.11 The backport for Cilium 1.11.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
No open projects
1.11.5
Backport done to v1.11
Development

Successfully merging this pull request may close these issues.

None yet

6 participants