Skip to content

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented May 16, 2022

  • pkg/labels/cidr: Add benchmarks for CIDR label functions
  • pkg/labels/cidr: Optimize maskedIPToLabelString()
  • pkg/labels/cidr: Optimize GetCIDRLabels()

See commit msgs.

Related: #19571

@christarazi christarazi added area/daemon Impacts operation of the Cilium daemon. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. kind/performance There is a performance impact of this. release-note/misc This PR makes changes that have no direct user impact. labels May 16, 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 May 16, 2022
@christarazi
Copy link
Member Author

/test

@christarazi christarazi changed the title pr/christarazi/optimize cidr labels str Optimize CIDR label functions May 16, 2022
@christarazi christarazi marked this pull request as ready for review May 16, 2022 23:49
@christarazi christarazi requested a review from a team May 16, 2022 23:49
@christarazi christarazi requested a review from a team as a code owner May 16, 2022 23:49
@christarazi christarazi force-pushed the pr/christarazi/optimize-cidr-labels-str branch from 439c4c7 to f9af34e Compare May 24, 2022 21:46
@christarazi
Copy link
Member Author

christarazi commented May 24, 2022

/test

Job 'Cilium-PR-K8s-GKE' failed:

Click to show.

Test Name

K8sDatapathConfig Etcd Check connectivity

Failure Output

FAIL: Failure while waiting for connectivity test pods to start

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create one.

@christarazi
Copy link
Member Author

/mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next

This will come in handy for upcoming commits that will try to improve
the performance of these functions.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Use strings.Builder instead of fmt.Sprintf() which is known to be not
performant.

This optimization was identified by inspecting a pprof (memory profile)
of a cluster with a wildcard L7 DNS rule policy (matchPattern: '*') and
a workload selected by the policy making a large amount of unique DNS
requests.

```
$ go test -v -run '^$' -bench 'Benchmark_maskedIPNetToLabelString' -benchtime 50000x -benchmem ./pkg/labels/cidr > old.txt
$ go test -v -run '^$' -bench 'Benchmark_maskedIPNetToLabelString' -benchtime 50000x -benchmem ./pkg/labels/cidr > 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
Benchmark_maskedIPNetToLabelString-8     8122          4450          -45.21%

benchmark                                old allocs     new allocs     delta
Benchmark_maskedIPNetToLabelString-8     39             32             -17.95%

benchmark                                old bytes     new bytes     delta
Benchmark_maskedIPNetToLabelString-8     488           368           -24.59%
```

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Avoid using fmt.Sprintf() when a simple string concatentation does the
same job, and preallocate slices where possible.

These optimizations were identified by inspecting a pprof (memory
profile) of a cluster with a wildcard L7 DNS rule policy (matchPattern:
'*') and a workload selected by the policy making a large amount of
unique DNS requests.

Without the previous commit that optimizes the maskedIPToLabelString():

```
$ go test -v -run '^$' -bench 'Benchmark_GetCIDRLabels' -benchtime 5000x -benchmem ./pkg/labels/cidr > old.txt
$ go test -v -run '^$' -bench 'Benchmark_GetCIDRLabels' -benchtime 5000x -benchmem ./pkg/labels/cidr > 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
Benchmark_GetCIDRLabels-8     391590        390163        -0.36%

benchmark                     old allocs     new allocs     delta
Benchmark_GetCIDRLabels-8     1455           1424           -2.13%

benchmark                     old bytes     new bytes     delta
Benchmark_GetCIDRLabels-8     64143         63189         -1.49%
```

With previous commit:

```
benchmark                     old ns/op     new ns/op     delta
Benchmark_GetCIDRLabels-8     390163        295259        -24.32%

benchmark                     old allocs     new allocs     delta
Benchmark_GetCIDRLabels-8     1424           1131           -20.58%

benchmark                     old bytes     new bytes     delta
Benchmark_GetCIDRLabels-8     63189         58707         -7.09%
```

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/christarazi/optimize-cidr-labels-str branch from f9af34e to 4ec97d9 Compare May 27, 2022 23:59
@christarazi
Copy link
Member Author

/test

@christarazi
Copy link
Member Author

Marking ready to merge given CI passed except for known aws-cni failure (fixed in #20049) and approving reviews.

@christarazi christarazi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 2, 2022
@joamaki joamaki merged commit 46e3d07 into cilium:master Jun 3, 2022
@christarazi christarazi deleted the pr/christarazi/optimize-cidr-labels-str branch June 3, 2022 16:13
@joestringer joestringer added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Jun 9, 2022
@christarazi
Copy link
Member Author

Marking for backport to v1.10 to improve performance given that #19570 & #19423 were backported as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon Impacts operation of the Cilium daemon. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. 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