ROX-33758: Fix quadratic runtime in endpointsStore.addToHistory#19572
ROX-33758: Fix quadratic runtime in endpointsStore.addToHistory#19572rhacs-bot wants to merge 1 commit intorelease-4.10from
endpointsStore.addToHistory#19572Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
benchmarkSeedEndpointsStore,for i := range numEndpointswill not compile becauserangedoes not work over anint; use a standard counter loop likefor i := 0; i < numEndpoints; i++instead. - In
BenchmarkEndpointsStoreAddToHistory, verify thatb.Loop()is intentional and supported in your test harness; in standard Go benchmarks this should be afor i := 0; i < b.N; i++loop.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `benchmarkSeedEndpointsStore`, `for i := range numEndpoints` will not compile because `range` does not work over an `int`; use a standard counter loop like `for i := 0; i < numEndpoints; i++` instead.
- In `BenchmarkEndpointsStoreAddToHistory`, verify that `b.Loop()` is intentional and supported in your test harness; in standard Go benchmarks this should be a `for i := 0; i < b.N; i++` loop.
## Individual Comments
### Comment 1
<location path="sensor/common/clusterentities/store_endpoints_benchmark_test.go" line_range="17" />
<code_context>
+ epSet := set.NewSet[net.NumericEndpoint]()
+
+ var firstEndpoint net.NumericEndpoint
+ for i := range numEndpoints {
+ ep := buildEndpoint(fmt.Sprintf("10.%d.%d.%d", (i/65536)%256, (i/256)%256, i%256), 80)
+ if i == 0 {
</code_context>
<issue_to_address>
**issue (bug_risk):** Fix the loop over numEndpoints so the benchmark compiles and exercises the intended number of endpoints
`range` can't be used on an `int`, so this code won't compile and the benchmark can't run. Use an indexed loop instead:
```go
for i := 0; i < numEndpoints; i++ {
// ...
}
```
This will correctly seed `numEndpoints` entries into the store and keep the benchmark at the intended scale.
</issue_to_address>
### Comment 2
<location path="sensor/common/clusterentities/store_endpoints_benchmark_test.go" line_range="44-47" />
<code_context>
+ {name: "endpoints_1000", numEndpoints: 1000},
+ {name: "endpoints_5000", numEndpoints: 5000},
+ } {
+ b.Run(tc.name, func(b *testing.B) {
+ store, deploymentID, endpoint := benchmarkSeedEndpointsStore(tc.numEndpoints)
+ b.ResetTimer()
+ for b.Loop() {
+ // Keep the current-map shape constant and measure addToHistory only.
+ store.historicalEndpoints = make(map[net.NumericEndpoint]map[string]map[EndpointTargetInfo]*entityStatus)
</code_context>
<issue_to_address>
**issue (testing):** Use the standard `for i := 0; i < b.N; i++` pattern instead of `b.Loop()` in the benchmark
`testing.B` doesn’t define `Loop()`, so this code won’t compile. To integrate with Go’s benchmark harness, use the standard `for i := 0; i < b.N; i++ { ... }` pattern with `b.ResetTimer()` instead of `for b.Loop()`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| epSet := set.NewSet[net.NumericEndpoint]() | ||
|
|
||
| var firstEndpoint net.NumericEndpoint | ||
| for i := range numEndpoints { |
There was a problem hiding this comment.
issue (bug_risk): Fix the loop over numEndpoints so the benchmark compiles and exercises the intended number of endpoints
range can't be used on an int, so this code won't compile and the benchmark can't run. Use an indexed loop instead:
for i := 0; i < numEndpoints; i++ {
// ...
}This will correctly seed numEndpoints entries into the store and keep the benchmark at the intended scale.
| b.Run(tc.name, func(b *testing.B) { | ||
| store, deploymentID, endpoint := benchmarkSeedEndpointsStore(tc.numEndpoints) | ||
| b.ResetTimer() | ||
| for b.Loop() { |
There was a problem hiding this comment.
issue (testing): Use the standard for i := 0; i < b.N; i++ pattern instead of b.Loop() in the benchmark
testing.B doesn’t define Loop(), so this code won’t compile. To integrate with Go’s benchmark harness, use the standard for i := 0; i < b.N; i++ { ... } pattern with b.ResetTimer() instead of for b.Loop().
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new benchmark code does not compile as written (
for i := range numEndpointsandfor b.Loop()are invalid); update the loops to use standard forms likefor i := 0; i < numEndpoints; i++andfor i := 0; i < b.N; i++. - Consider removing or moving the large commented benchmark output block from
store_endpoints_benchmark_test.goto keep the test file focused on code rather than runtime logs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new benchmark code does not compile as written (`for i := range numEndpoints` and `for b.Loop()` are invalid); update the loops to use standard forms like `for i := 0; i < numEndpoints; i++` and `for i := 0; i < b.N; i++`.
- Consider removing or moving the large commented benchmark output block from `store_endpoints_benchmark_test.go` to keep the test file focused on code rather than runtime logs.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Images are ready for the commit at 71d9ed5. To use with deploy scripts, first |
|
/konflux-retest operator-bundle-on-push |
2 similar comments
|
/konflux-retest operator-bundle-on-push |
|
/konflux-retest operator-bundle-on-push |
|
/konflux-retest main-on-push |
|
@rhacs-bot: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Backport bfd67f2 from #19566.