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

hubble: Optimize namespace tracking #26547

Merged
merged 1 commit into from Jul 11, 2023

Conversation

glibsm
Copy link
Member

@glibsm glibsm commented Jun 28, 2023

Avoiding the use of varargs saves some allocations

Before

Benchmark_TrackNamespaces-12    	 5122684	       231.8 ns/op	     192 B/op	       6 allocs/op

After

Benchmark_TrackNamespaces-12    	 6114062	       187.9 ns/op	     168 B/op	       4 allocs/op

@glibsm glibsm added area/daemon Impacts operation of the Cilium daemon. kind/performance There is a performance impact of this. labels Jun 28, 2023
@glibsm glibsm requested a review from chancez June 28, 2023 23:09
@glibsm glibsm requested a review from a team as a code owner June 28, 2023 23:09
@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. labels Jun 28, 2023
@glibsm glibsm added the release-note/misc This PR makes changes that have no direct user impact. label Jun 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 28, 2023
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @glibsm!

pkg/hubble/relay/observer/server.go needs to be adapted accordingly.

Note that after the patch, adding a flow's src/dst namespaces is no longer "atomic" from the namespaceManager point of view but I don't think that's an issue.

@chancez
Copy link
Contributor

chancez commented Jul 5, 2023

We should back port this to 1.14 since this functionality is new in 1.14 and that memory increase could be viewed as a performance regression.

@chancez chancez added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Jul 5, 2023
Avoiding the use of varargs saves some allocations

Before
```
Benchmark_TrackNamespaces-12    	 5122684	       231.8 ns/op	     192 B/op	       6 allocs/op
```

After
```
Benchmark_TrackNamespaces-12    	 6114062	       187.9 ns/op	     168 B/op	       4 allocs/op
```

Signed-off-by: Glib Smaga <code@gsmaga.com>
@glibsm glibsm force-pushed the pr/glibsm/optimize-namespace-tracking branch from ec5c36c to f2ca387 Compare July 10, 2023 17:18
@kaworu
Copy link
Member

kaworu commented Jul 11, 2023

/test

@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 Jul 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.0 Jul 11, 2023
@rolinh rolinh merged commit 9410932 into cilium:main Jul 11, 2023
65 checks passed
@jibi jibi mentioned this pull request Jul 13, 2023
13 tasks
@jibi jibi 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 Jul 13, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.0 Jul 13, 2023
@aanm aanm 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 Jul 14, 2023
@aanm aanm moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.0 Jul 26, 2023
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. 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
No open projects
1.14.0
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

None yet

6 participants