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
fqdn: Fix notifyOnDNSMsg benchmark #32454
Merged
julianwiedmann
merged 2 commits into
cilium:main
from
pippolo84:pr/pippolo84/fix-notifyondnsmsg-bench
May 21, 2024
Merged
fqdn: Fix notifyOnDNSMsg benchmark #32454
julianwiedmann
merged 2 commits into
cilium:main
from
pippolo84:pr/pippolo84/fix-notifyondnsmsg-bench
May 21, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pippolo84
added
kind/bug/CI
This is a bug in the testing code.
area/fqdn
Affects the FQDN policies feature
labels
May 10, 2024
maintainer-s-little-helper
bot
added
dont-merge/needs-release-note-label
The author needs to describe the release impact of these changes.
labels
May 10, 2024
github-actions
bot
added
the
sig/policy
Impacts whether traffic is allowed or denied based on user-defined policies.
label
May 10, 2024
pippolo84
added
the
release-note/misc
This PR makes changes that have no direct user impact.
label
May 10, 2024
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
May 10, 2024
pippolo84
force-pushed
the
pr/pippolo84/fix-notifyondnsmsg-bench
branch
from
May 13, 2024 09:31
fd1cbbf
to
70df921
Compare
doniacld
reviewed
May 14, 2024
doniacld
approved these changes
May 14, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit comment, otherwise lgtm 🚀
NB: Particularly appreciate the commit message.
Add defaults values for DNSProxyLockCount and DNSProxyLockTimeout in order to refer them both in the global options initialization and in the notifyOnDNSMessage benchmark. Signed-off-by: Fabio Falzoi <[email protected]>
The design of BenchmarkNotifyOnDNSMsg was flawed due to the usage of b.N as benchmark input size. b.N is used by the Go benchmarking framework to find a reliable timing for the code under test. Changing the amount of work done at each benchmark call leads to unreliable numbers. For more info refer to the Go documentation: The benchmark function must run the target code b.N times. During benchmark execution, b.N is adjusted until the benchmark function lasts long enough to be timed reliably. And the section `Traps for young players` in the blog post https://dave.cheney.net/2013/06/30/how-to-write-benchmarks-in-go To fix it, the benchmark has been updated to setup a reasonable fixed number of endpoints. Also, since the code was behind the latest development in the policy and fqdn subsystems, it has been updated to run correctly. Specifically: - the global LocalNodeStore is set once at startup - the FQDN selectors are added to the selector cache with a dummy user - the daemon context is properly initialized - the FQDN related global opts are set to their defaults to avoid noisy logs Example run: goos: linux goarch: amd64 pkg: github.com/cilium/cilium/daemon/cmd cpu: 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz BenchmarkNotifyOnDNSMsg-8 21 47992143 ns/op 30757230 B/op 357646 allocs/op BenchmarkNotifyOnDNSMsg-8 24 48773334 ns/op 30590877 B/op 355639 allocs/op BenchmarkNotifyOnDNSMsg-8 22 47940489 ns/op 30719252 B/op 358088 allocs/op BenchmarkNotifyOnDNSMsg-8 24 48019454 ns/op 30364074 B/op 353470 allocs/op BenchmarkNotifyOnDNSMsg-8 24 66607283 ns/op 30634838 B/op 356352 allocs/op PASS Signed-off-by: Fabio Falzoi <[email protected]>
pippolo84
force-pushed
the
pr/pippolo84/fix-notifyondnsmsg-bench
branch
from
May 14, 2024 17:00
70df921
to
2aa79d5
Compare
/test |
vipul-21
reviewed
May 15, 2024
doniacld
approved these changes
May 17, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good! Thanks for the changes!
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
May 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area/fqdn
Affects the FQDN policies feature
kind/bug/CI
This is a bug in the testing code.
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.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The design of BenchmarkNotifyOnDNSMsg was flawed due to the usage of b.N as benchmark input size. b.N is used by the Go benchmarking framework to find a reliable timing for the code under test. Changing the amount of work done at each benchmark call leads to unreliable numbers. For more info refer to the Go documentation:
And the section
Traps for young players
in the blog post https://dave.cheney.net/2013/06/30/how-to-write-benchmarks-in-goTo fix it, the benchmark has been updated to setup a reasonable fixed number of endpoints.
Also, since the code was behind the latest development in the policy and fqdn subsystems, it has been updated to run correctly. Specifically:
Example run:
Related: #32297 (comment)