cilium icon indicating copy to clipboard operation
cilium copied to clipboard

fqdn: Fix notifyOnDNSMsg benchmark

Open pippolo84 opened this issue 9 months ago • 2 comments

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

Related: https://github.com/cilium/cilium/pull/32297#discussion_r1588937381

pippolo84 avatar May 10 '24 09:05 pippolo84

/test

pippolo84 avatar May 14 '24 17:05 pippolo84

Given this PR I'm removing @danehans as reviewer. @doniacld do you mind taking another look at this for the sig-agent approval? I've just defined the additional constants. Thanks! :pray:

pippolo84 avatar May 15 '24 17:05 pippolo84