Skip to content

test(integration): add GetChangedTargets benchmarks#204

Open
yushan8 wants to merge 12 commits into
mainfrom
add-getchangedtargets-benchmarks
Open

test(integration): add GetChangedTargets benchmarks#204
yushan8 wants to merge 12 commits into
mainfrom
add-getchangedtargets-benchmarks

Conversation

@yushan8

@yushan8 yushan8 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a make bench target that measures GetChangedTargets latency with two benchmarks:
    • Cold — 15 iterations over unique commit pairs, each a cache miss exercising the full pipeline (git checkout, bazel query, compare, stream)
    • Cached — 15 iterations hitting the in-memory cache, measuring streaming overhead only
  • Adds startServerWithLogger to allow benchmarks to inject a nop logger, suppressing service log noise
  • Measurement only — no pass/fail thresholds, not part of make test / make test-integration, and never invoked from CI
  • Widens repoRemote/startServer/newClient/getChangedTargets in integration_test.go from *testing.T to testing.TB so the existing integration harness can be reused from Benchmark* functions

Test plan

  • make gazelleintegration/BUILD.bazel picked up the new benchmark_test.go
  • make test-integration — existing integration tests still pass unchanged
  • make bench — both benchmarks run 15 iterations and report ns/op, exits 0

Add a `make benchmark` target that measures GetChangedTargets latency
against three fixed, checked-in commit pairs of increasing diff size
(small/medium/large). This is measurement-only: it has no pass/fail
thresholds, is not part of `make test` / `make test-integration`, and
is never invoked from CI, so a slow run can never fail the build.

- Widen repoRemote/startServer/newClient/getChangedTargets in
  integration_test.go from *testing.T to testing.TB so the existing
  integration test harness can be reused from Benchmark* functions.
- Add integration/benchmark_test.go with three benchmarks pinned to
  fixed commit SHAs in this repo's own history.
- Add benchmark_test.go to the integration_test BUILD.bazel target via
  `make gazelle`.
- Add the `make benchmark` target, which runs the same bazel go_test
  binary with -test.run=^$ -test.bench=. so only benchmarks execute.

Test Plan:
- make gazelle (confirmed BUILD.bazel picked up the new file)
- make test-integration (existing tests still pass unchanged)
- make benchmark (all three benchmarks run and report ns/op, exits 0)
@yushan8
yushan8 marked this pull request as ready for review July 15, 2026 16:47
@yushan8
yushan8 requested review from a team as code owners July 15, 2026 16:47
ns/op is hard to eyeball at these durations (calls take seconds, not
nanoseconds). Add a sec/op custom metric via b.ReportMetric alongside
the built-in ns/op, computed from b.Elapsed() so it reflects the same
timer window (post-ResetTimer).

@sywhang sywhang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the current measured benchmark on this on your local box?

Comment thread Makefile Outdated
yushan8 and others added 2 commits July 16, 2026 21:10
Address review comment: shorter target name consistent with Go convention.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@yushan8

yushan8 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

What's the current measured benchmark on this on your local box?

BenchmarkGetChangedTargets_SmallDiff-10       3   503320 ns/op   0.0005030 avg_cached_call_sec/op   44.90 first_uncached_call_sec
BenchmarkGetChangedTargets_MediumDiff-10      3   466181 ns/op   0.0004658 avg_cached_call_sec/op   41.07 first_uncached_call_sec
BenchmarkGetChangedTargets_LargeDiff-10       3  1451639 ns/op   0.001451 avg_cached_call_sec/op    38.21 first_uncached_call_sec

uncached run is the initial request, cached run is the subsequent calls with the same request.

Comment thread Makefile
@echo "Running benchmarks..."
@$(BAZEL) test //integration:integration_test \
--test_output=all \
--test_arg=-test.run=^$$ \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's run it with higher iteration by default. like 15 or 20.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I decided changed the test to run getChangedTargets on different commits to test cold cache calls. Since each iteration needs a unique commit pair for a cache miss, b.N is capped at the number of pairs we have. BenchmarkGetChangedTargets_Cold tests for the requests with cache miss (full bazel query) with 15 distinctive commit pairs, while the other BenchmarkGetChangedTargets_Cached tests for the duration of cache hits.
This lets us measure the endpoint latency without the server startup.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated benchtime to run 15x

yushan8 and others added 7 commits July 17, 2026 13:00
Address review comment: run more iterations for stable measurements.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address review comment: run 15 cached iterations for stable measurements.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Run the full benchmark function 15 times (including cold call) instead
of repeating only the cached b.N loop.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Each iteration uses a distinct pair of consecutive commits, guaranteeing
a cache miss per call. This measures true cold-path performance (git
checkout + bazel query + compare) without restarting the server 15 times.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace early-history commits (broken Bazel configs, README-only diffs)
with pairs from the stable post-5716262 range that all touch Go source.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Keep both `cover` (from main) and `bench` (from this branch) in .PHONY.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants