pkg/bgpv1/gobgp: Optimize reconcileDiff String()

Avoiding to use fmt.Sprintf() so that Go won't over-allocate the memory.

issue reference : cilium#19571

Result:

$ go test -benchmem -run=^$ -bench ^BenchmarkReconcileDiffString$ github.com/cilium/cilium/pkg/bgpv1/gobgp > BenchmarkReconcileDiffString_old.txt
$ go test -benchmem -run=^$ -bench ^BenchmarkReconcileDiffString$ github.com/cilium/cilium/pkg/bgpv1/gobgp > BenchmarkReconcileDiffString_new.txt
$ benchcmp  BenchmarkReconcileDiffString_old.txt BenchmarkReconcileDiffString_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
BenchmarkReconcileDiffString-12     1592          350           -78.01%

benchmark                           old allocs     new allocs     delta
BenchmarkReconcileDiffString-12     18             5              -72.22%

benchmark                           old bytes     new bytes     delta
BenchmarkReconcileDiffString-12     264           152           -42.42%

Signed-off-by: MikeLing [email protected]

@MikeLing

Are these optimizations worth wild? My assumption is that the String() method isn't called particularly often, but measuring this would seem to be the first step to determine if this makes a difference or not?

Are these optimizations worth wild? My assumption is that the String() method isn't called particularly often, but measuring this would seem to be the first step to determine if this makes a difference or not?

@ldelossa Measuring the called frequency make sense to me. Actually, like https://github.com/cilium/cilium/issues/19571#issuecomment-1203188316 mentioned, we. still have more than 200 places of code need to be changed from fmt.Sprintf to the others. Do we have any way to measure the function called frequency? Because some functions' called frequency are depend on the scenery. Like String(), we may need to call it every time if the user need to inspect the cluster status, otherwise it won't be called like forever.

@MikeLing Not all fmt.Sprintf instances need to be optimized as some of them are not in the hot path. I'd imagine this BGP code path is not necessarily in the hot path, but @ldelossa can confirm that.

Yes, I don't think we need to remove all instances of fmt.Sprintf and re-invent string building and the "%+v" format specifier. A better way to go about this would be to run Cilium in some common configurations and pprof fmt.Sprintf usages, then remove where they spin hot.

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

This pull request has not seen any activity since it was marked stale. Closing.