Skip to content

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented Apr 12, 2022

  • pkg/labels: Add benchmark for hot labels code
  • pkg/labels: Optimize SortedList() and FormatForKVStore()

Related: #19571

SortedList() and FormatForKVStore() can be very hot code in environments
where there's constant policy churn, especially CIDR policies where
there can be a large number of CIDR labels.

This commit adds benchmarks for later commits to use as a baseline.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi requested a review from a team April 12, 2022 22:31
@christarazi christarazi requested a review from a team as a code owner April 12, 2022 22:31
@christarazi christarazi added area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. 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 12, 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 12, 2022
@christarazi christarazi changed the title pr/christarazi/optimize labels sorted pkg/labels: Optimize SortedList() and FormatForKVStore() Apr 12, 2022
@christarazi christarazi added kind/performance There is a performance impact of this. needs-backport/1.11 labels Apr 12, 2022
FormatForKVStore() previously returned a string for no reason as every
caller converted the return value to a byte slice. This allows us to
eliminate string concatenation entirely and use the bytes.Buffer
directly.

Building on the above, given that SortedList() returns a byte slice and
calls FormatForKVStore() for its output, we can optimize it with the
same technique to eliminate string concatenation.

Here are the benchmark comparisons:

```
$ go test -v -run '^$' -bench 'BenchmarkLabels_SortedList|BenchmarkLabel_FormatForKVStore' -benchmem ./pkg/labels > old.txt
$ go test -v -run '^$' -bench 'BenchmarkLabels_SortedList|BenchmarkLabel_FormatForKVStore' -benchmem ./pkg/labels > new.txt
$ benchcmp old.txt new.txt
benchcmp is deprecated in favor of benchstat: https://pkg.go.dev/golang.org/x/perf/cmd/benchstat
benchmark                             old ns/op     new ns/op     delta
BenchmarkLabels_SortedList-8          2612          1120          -57.12%
BenchmarkLabel_FormatForKVStore-8     262           54.5          -79.18%

benchmark                             old allocs     new allocs     delta
BenchmarkLabels_SortedList-8          35             13             -62.86%
BenchmarkLabel_FormatForKVStore-8     4              1              -75.00%

benchmark                             old bytes     new bytes     delta
BenchmarkLabels_SortedList-8          1112          664           -40.29%
BenchmarkLabel_FormatForKVStore-8     96            48            -50.00%
```

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/christarazi/optimize-labels-sorted branch from fbe7370 to cbde641 Compare April 12, 2022 22:46
@christarazi christarazi requested a review from a team as a code owner April 12, 2022 22:46
@christarazi christarazi requested a review from jrajahalme April 12, 2022 22:46
@christarazi
Copy link
Member Author

christarazi commented Apr 12, 2022

/test

Job 'Cilium-PR-K8s-GKE' hit: #17270 (92.08% similarity)

Edit:

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.

how does the benchmark output look like?

Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

code checks out 🚤

@christarazi
Copy link
Member Author

@michi-covalent It should be in the second commit msg.

Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

Nice improvement!

@christarazi
Copy link
Member Author

/test-1.21-5.4

@christarazi
Copy link
Member Author

/test-gke

@christarazi
Copy link
Member Author

christarazi commented Apr 27, 2022

/test-1.21-5.4

Edit: failed pulling images presumably because the images have expired

@christarazi
Copy link
Member Author

Marking ready to merge as we have approving reviews and all CI failure hit known flakes. Ignoring test-1.21-5.4 because it is not going to provide any more signal than all the other Jenkins run anyway.

@christarazi christarazi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 28, 2022
@aanm aanm merged commit 0790e07 into cilium:master Apr 28, 2022
@christarazi christarazi deleted the pr/christarazi/optimize-labels-sorted branch April 28, 2022 23:42
@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.

7 participants