Skip to content

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
@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
@aditighag aditighag added backport-pending/1.11 backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed needs-backport/1.11 labels May 2, 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
None yet
Development

Successfully merging this pull request may close these issues.

6 participants