From 2f92525a70730fc0a6649579d186f09ae55988fc Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Tue, 14 Jul 2026 16:32:04 -0700 Subject: [PATCH 01/11] test(integration): add GetChangedTargets benchmarks 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) --- Makefile | 17 ++++++++++- integration/BUILD.bazel | 5 +++- integration/benchmark_test.go | 52 +++++++++++++++++++++++++++++++++ integration/integration_test.go | 10 +++---- 4 files changed, 77 insertions(+), 7 deletions(-) create mode 100644 integration/benchmark_test.go diff --git a/Makefile b/Makefile index 3b15eb30..dcb74c80 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build test test-integration proto gazelle clean clean-proto run-server run-client-get-graph run-client-changed-targets help +.PHONY: build test test-integration benchmark proto gazelle clean clean-proto run-server run-client-get-graph run-client-changed-targets help # Bazel wrapper BAZEL = ./tools/bazel @@ -21,6 +21,20 @@ test-integration: @$(BAZEL) test //integration:integration_test --test_output=errors --test_env=TANGO_REPO_REMOTE=$$(git rev-parse --show-toplevel) --test_env=HOME=$$HOME @echo "Integration tests passed!" +# Run GetChangedTargets benchmarks against fixed, checked-in commit pairs. +# Measurement only: not part of `make test` / `make test-integration` and not +# run in CI, so a slow benchmark never fails the build. +benchmark: + @echo "Running benchmarks..." + @$(BAZEL) test //integration:integration_test \ + --test_output=all \ + --test_arg=-test.run=^$$ \ + --test_arg=-test.bench=. \ + --test_arg=-test.benchtime=3x \ + --test_env=TANGO_REPO_REMOTE=$$(git rev-parse --show-toplevel) \ + --test_env=HOME=$$HOME + @echo "Benchmarks complete!" + # Generate protobuf files using protoc proto: @echo "Generating protobuf files with protoc..." @@ -90,6 +104,7 @@ help: @echo " make build - Build all targets" @echo " make test - Run all tests" @echo " make test-integration - Run integration tests (slow)" + @echo " make benchmark - Run GetChangedTargets benchmarks (measurement only, not in CI)" @echo " make gazelle - Update BUILD.bazel files" @echo " make clean - Clean generated files and binaries" @echo "" diff --git a/integration/BUILD.bazel b/integration/BUILD.bazel index 40f7eaa2..0d5668f6 100644 --- a/integration/BUILD.bazel +++ b/integration/BUILD.bazel @@ -4,7 +4,10 @@ go_test( name = "integration_test", size = "large", timeout = "long", - srcs = ["integration_test.go"], + srcs = [ + "benchmark_test.go", + "integration_test.go", + ], data = glob(["testdata/**"]), tags = [ "integration", diff --git a/integration/benchmark_test.go b/integration/benchmark_test.go new file mode 100644 index 00000000..6fff770d --- /dev/null +++ b/integration/benchmark_test.go @@ -0,0 +1,52 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Benchmarks for GetChangedTargets against fixed, checked-in commit pairs of +// increasing diff size. These are for local performance measurement only: +// run via `make benchmark`, never via `make test` / `make test-integration`, +// and never from CI. They assert nothing about how fast the call must be. +package integration_test + +import "testing" + +// runChangedTargetsBenchmark starts one server for the whole benchmark and +// repeats the same GetChangedTargets call b.N times. Only the first call +// computes the diff from scratch; every subsequent call hits the +// orchestrator's in-memory cache for the same treehash pair, so this +// measures steady-state/cache-hit latency rather than cold-cache cost. +func runChangedTargetsBenchmark(b *testing.B, firstSHA, secondSHA string) { + remote := repoRemote(b) + addr := startServer(b, remote) + client := newClient(b, addr) + + b.ResetTimer() // exclude clone/server-startup cost from the measurement + for i := 0; i < b.N; i++ { + getChangedTargets(b, client, remote, firstSHA, secondSHA) + } +} + +func BenchmarkGetChangedTargets_SmallDiff(b *testing.B) { + // 5716262 -> 74d1cd5: 2 files changed, 13 lines. + runChangedTargetsBenchmark(b, "57162624a45965a7e783072c56561f91c5d4084d", "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96") +} + +func BenchmarkGetChangedTargets_MediumDiff(b *testing.B) { + // 046de2c -> 1f2e3e9: 8 files changed, 461 lines. + runChangedTargetsBenchmark(b, "046de2c20b5492cd5606d32fd632a38b8b70c8f6", "1f2e3e9245b159006cf2103becd51c5c1b6ec868") +} + +func BenchmarkGetChangedTargets_LargeDiff(b *testing.B) { + // 684a6d0 (repo setup) -> 9bf997c: 162 files changed, ~19.9k lines. + runChangedTargetsBenchmark(b, "684a6d06e82cff50a4f1a5addb8f64a45aaa6c26", "9bf997cf709b1ee38a71fa0d1423e66d0494e9bc") +} diff --git a/integration/integration_test.go b/integration/integration_test.go index c3373596..1625687d 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -48,14 +48,14 @@ const ( configTemplateFile = "testdata/tango-config.yaml.tmpl" ) -func repoRemote(t *testing.T) string { +func repoRemote(t testing.TB) string { t.Helper() remote := os.Getenv("TANGO_REPO_REMOTE") require.NotEmpty(t, remote, "TANGO_REPO_REMOTE must be set (pass --test_env=TANGO_REPO_REMOTE=... to bazel test)") return remote } -func writeConfig(t *testing.T, dir, remote, clonePath, workerPath string) string { +func writeConfig(t testing.TB, dir, remote, clonePath, workerPath string) string { t.Helper() tmpl, err := template.ParseFiles(configTemplateFile) @@ -82,7 +82,7 @@ func writeConfig(t *testing.T, dir, remote, clonePath, workerPath string) string return configPath } -func startServer(t *testing.T, remote string) string { +func startServer(t testing.TB, remote string) string { t.Helper() configDir := t.TempDir() @@ -142,7 +142,7 @@ func startServer(t *testing.T, remote string) string { return listener.Addr().String() } -func newClient(t *testing.T, addr string) pb.TangoYARPCClient { +func newClient(t testing.TB, addr string) pb.TangoYARPCClient { t.Helper() grpcTransport := yarpcgrpc.NewTransport() @@ -251,7 +251,7 @@ type parsedChangedTargets struct { Distances map[string]int32 } -func getChangedTargets(t *testing.T, client pb.TangoYARPCClient, remote, firstSHA, secondSHA string) parsedChangedTargets { +func getChangedTargets(t testing.TB, client pb.TangoYARPCClient, remote, firstSHA, secondSHA string) parsedChangedTargets { t.Helper() ctx, cancel := context.WithTimeout(context.Background(), requestTimeout) From dde009df321d956ddc8a3a5240a30db1628bd3d8 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Wed, 15 Jul 2026 10:29:02 -0700 Subject: [PATCH 02/11] test(integration): report benchmark timing in sec/op too 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). --- integration/benchmark_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/integration/benchmark_test.go b/integration/benchmark_test.go index 6fff770d..b19a4e0d 100644 --- a/integration/benchmark_test.go +++ b/integration/benchmark_test.go @@ -34,6 +34,7 @@ func runChangedTargetsBenchmark(b *testing.B, firstSHA, secondSHA string) { for i := 0; i < b.N; i++ { getChangedTargets(b, client, remote, firstSHA, secondSHA) } + b.ReportMetric(b.Elapsed().Seconds()/float64(b.N), "sec/op") } func BenchmarkGetChangedTargets_SmallDiff(b *testing.B) { From e972920a018aa67437865fa41c88f005a2c4c283 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 16 Jul 2026 21:10:51 -0700 Subject: [PATCH 03/11] fix(makefile): rename benchmark target to bench Address review comment: shorter target name consistent with Go convention. Co-Authored-By: Claude Sonnet 5 --- Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index dcb74c80..3bc21ffc 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build test test-integration benchmark proto gazelle clean clean-proto run-server run-client-get-graph run-client-changed-targets help +.PHONY: build test test-integration bench proto gazelle clean clean-proto run-server run-client-get-graph run-client-changed-targets help # Bazel wrapper BAZEL = ./tools/bazel @@ -24,7 +24,7 @@ test-integration: # Run GetChangedTargets benchmarks against fixed, checked-in commit pairs. # Measurement only: not part of `make test` / `make test-integration` and not # run in CI, so a slow benchmark never fails the build. -benchmark: +bench: @echo "Running benchmarks..." @$(BAZEL) test //integration:integration_test \ --test_output=all \ @@ -104,7 +104,7 @@ help: @echo " make build - Build all targets" @echo " make test - Run all tests" @echo " make test-integration - Run integration tests (slow)" - @echo " make benchmark - Run GetChangedTargets benchmarks (measurement only, not in CI)" + @echo " make bench - Run all benchmark tests" @echo " make gazelle - Update BUILD.bazel files" @echo " make clean - Clean generated files and binaries" @echo "" From 94986c15f6bf18c8243c917eb70fd5e163d7ae0f Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 17 Jul 2026 10:36:11 -0700 Subject: [PATCH 04/11] Update --- Makefile | 8 +++++-- integration/BUILD.bazel | 2 ++ integration/benchmark_test.go | 37 ++++++++++++++++++++------------- integration/integration_test.go | 7 ++++++- 4 files changed, 37 insertions(+), 17 deletions(-) diff --git a/Makefile b/Makefile index 3bc21ffc..c1f6ca2c 100644 --- a/Makefile +++ b/Makefile @@ -32,7 +32,11 @@ bench: --test_arg=-test.bench=. \ --test_arg=-test.benchtime=3x \ --test_env=TANGO_REPO_REMOTE=$$(git rev-parse --show-toplevel) \ - --test_env=HOME=$$HOME + --test_env=HOME=$$HOME \ + --test_env=GIT_CONFIG_COUNT=1 \ + --test_env=GIT_CONFIG_KEY_0=advice.detachedHead \ + --test_env=GIT_CONFIG_VALUE_0=false \ + --cache_test_results=no @echo "Benchmarks complete!" # Generate protobuf files using protoc @@ -104,7 +108,7 @@ help: @echo " make build - Build all targets" @echo " make test - Run all tests" @echo " make test-integration - Run integration tests (slow)" - @echo " make bench - Run all benchmark tests" + @echo " make bench - Run GetChangedTargets benchs (measurement only, not in CI)" @echo " make gazelle - Update BUILD.bazel files" @echo " make clean - Clean generated files and binaries" @echo "" diff --git a/integration/BUILD.bazel b/integration/BUILD.bazel index 0d5668f6..c71ce849 100644 --- a/integration/BUILD.bazel +++ b/integration/BUILD.bazel @@ -26,6 +26,8 @@ go_test( "@org_uber_go_yarpc//:yarpc", "@org_uber_go_yarpc//api/transport", "@org_uber_go_yarpc//transport/grpc", + "@org_uber_go_zap//:zap", + "@org_uber_go_zap//zapcore", "@org_uber_go_zap//zaptest", ], ) diff --git a/integration/benchmark_test.go b/integration/benchmark_test.go index b19a4e0d..50b58d64 100644 --- a/integration/benchmark_test.go +++ b/integration/benchmark_test.go @@ -14,40 +14,49 @@ // Benchmarks for GetChangedTargets against fixed, checked-in commit pairs of // increasing diff size. These are for local performance measurement only: -// run via `make benchmark`, never via `make test` / `make test-integration`, +// run via `make bench`, never via `make test` / `make test-integration`, // and never from CI. They assert nothing about how fast the call must be. package integration_test -import "testing" +import ( + "testing" + "time" -// runChangedTargetsBenchmark starts one server for the whole benchmark and -// repeats the same GetChangedTargets call b.N times. Only the first call -// computes the diff from scratch; every subsequent call hits the -// orchestrator's in-memory cache for the same treehash pair, so this -// measures steady-state/cache-hit latency rather than cold-cache cost. -func runChangedTargetsBenchmark(b *testing.B, firstSHA, secondSHA string) { + "go.uber.org/zap" + "go.uber.org/zap/zapcore" +) + +func runChangedTargetsBenchmark(b *testing.B, name, firstSHA, secondSHA string) { remote := repoRemote(b) - addr := startServer(b, remote) + logger := zap.New(zapcore.NewNopCore()) + addr := startServerWithLogger(b, remote, logger) client := newClient(b, addr) - b.ResetTimer() // exclude clone/server-startup cost from the measurement + coldStart := time.Now() + getChangedTargets(b, client, remote, firstSHA, secondSHA) + coldDuration := time.Since(coldStart) + + b.ResetTimer() for i := 0; i < b.N; i++ { getChangedTargets(b, client, remote, firstSHA, secondSHA) } - b.ReportMetric(b.Elapsed().Seconds()/float64(b.N), "sec/op") + cachedDuration := b.Elapsed() / time.Duration(b.N) + + b.ReportMetric(coldDuration.Seconds(), "first_uncached_call_sec") + b.ReportMetric(cachedDuration.Seconds(), "avg_cached_call_sec/op") } func BenchmarkGetChangedTargets_SmallDiff(b *testing.B) { // 5716262 -> 74d1cd5: 2 files changed, 13 lines. - runChangedTargetsBenchmark(b, "57162624a45965a7e783072c56561f91c5d4084d", "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96") + runChangedTargetsBenchmark(b, "SmallDiff", "57162624a45965a7e783072c56561f91c5d4084d", "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96") } func BenchmarkGetChangedTargets_MediumDiff(b *testing.B) { // 046de2c -> 1f2e3e9: 8 files changed, 461 lines. - runChangedTargetsBenchmark(b, "046de2c20b5492cd5606d32fd632a38b8b70c8f6", "1f2e3e9245b159006cf2103becd51c5c1b6ec868") + runChangedTargetsBenchmark(b, "MediumDiff", "046de2c20b5492cd5606d32fd632a38b8b70c8f6", "1f2e3e9245b159006cf2103becd51c5c1b6ec868") } func BenchmarkGetChangedTargets_LargeDiff(b *testing.B) { // 684a6d0 (repo setup) -> 9bf997c: 162 files changed, ~19.9k lines. - runChangedTargetsBenchmark(b, "684a6d06e82cff50a4f1a5addb8f64a45aaa6c26", "9bf997cf709b1ee38a71fa0d1423e66d0494e9bc") + runChangedTargetsBenchmark(b, "LargeDiff", "684a6d06e82cff50a4f1a5addb8f64a45aaa6c26", "9bf997cf709b1ee38a71fa0d1423e66d0494e9bc") } diff --git a/integration/integration_test.go b/integration/integration_test.go index 1625687d..44b83f56 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -40,6 +40,7 @@ import ( "go.uber.org/yarpc" "go.uber.org/yarpc/api/transport" yarpcgrpc "go.uber.org/yarpc/transport/grpc" + "go.uber.org/zap" "go.uber.org/zap/zaptest" ) @@ -84,6 +85,11 @@ func writeConfig(t testing.TB, dir, remote, clonePath, workerPath string) string func startServer(t testing.TB, remote string) string { t.Helper() + return startServerWithLogger(t, remote, zaptest.NewLogger(t)) +} + +func startServerWithLogger(t testing.TB, remote string, zl *zap.Logger) string { + t.Helper() configDir := t.TempDir() clonePath := t.TempDir() @@ -91,7 +97,6 @@ func startServer(t testing.TB, remote string) string { configPath := writeConfig(t, configDir, remote, clonePath, workerPath) - zl := zaptest.NewLogger(t) logger := zl.Sugar() appCtx, cancel := context.WithCancel(context.Background()) From 91616b3bbe1d9e3efd6523f20317aa9f3bd35985 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 17 Jul 2026 13:00:54 -0700 Subject: [PATCH 05/11] fix(makefile): increase benchtime to 20x Address review comment: run more iterations for stable measurements. Co-Authored-By: Claude Sonnet 5 --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index c1f6ca2c..a6329fb7 100644 --- a/Makefile +++ b/Makefile @@ -30,7 +30,7 @@ bench: --test_output=all \ --test_arg=-test.run=^$$ \ --test_arg=-test.bench=. \ - --test_arg=-test.benchtime=3x \ + --test_arg=-test.benchtime=20x \ --test_env=TANGO_REPO_REMOTE=$$(git rev-parse --show-toplevel) \ --test_env=HOME=$$HOME \ --test_env=GIT_CONFIG_COUNT=1 \ From b35d7e6d2c4dd050c6a3fc83a5f7f4161869aa7f Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 17 Jul 2026 14:02:55 -0700 Subject: [PATCH 06/11] fix(makefile): set benchtime to 15x Address review comment: run 15 cached iterations for stable measurements. Co-Authored-By: Claude Sonnet 5 --- Makefile | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Makefile b/Makefile index a6329fb7..4d644012 100644 --- a/Makefile +++ b/Makefile @@ -30,13 +30,9 @@ bench: --test_output=all \ --test_arg=-test.run=^$$ \ --test_arg=-test.bench=. \ - --test_arg=-test.benchtime=20x \ + --test_arg=-test.benchtime=15x \ --test_env=TANGO_REPO_REMOTE=$$(git rev-parse --show-toplevel) \ --test_env=HOME=$$HOME \ - --test_env=GIT_CONFIG_COUNT=1 \ - --test_env=GIT_CONFIG_KEY_0=advice.detachedHead \ - --test_env=GIT_CONFIG_VALUE_0=false \ - --cache_test_results=no @echo "Benchmarks complete!" # Generate protobuf files using protoc From 0c0edaed7ac58c18f60adca2c681df1f0b986450 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 17 Jul 2026 14:05:42 -0700 Subject: [PATCH 07/11] fix(makefile): use test.count=15 for benchmark iterations 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 --- Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 4d644012..d5ba0f7a 100644 --- a/Makefile +++ b/Makefile @@ -30,7 +30,8 @@ bench: --test_output=all \ --test_arg=-test.run=^$$ \ --test_arg=-test.bench=. \ - --test_arg=-test.benchtime=15x \ + --test_arg=-test.benchtime=1x \ + --test_arg=-test.count=15 \ --test_env=TANGO_REPO_REMOTE=$$(git rev-parse --show-toplevel) \ --test_env=HOME=$$HOME \ @echo "Benchmarks complete!" From 079e09143e67e591f8e50fe6bd8279577cf6555d Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 17 Jul 2026 14:13:56 -0700 Subject: [PATCH 08/11] fix(makefile): remove stray line continuation in bench target Co-Authored-By: Claude Sonnet 5 --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index d5ba0f7a..fdea067e 100644 --- a/Makefile +++ b/Makefile @@ -33,7 +33,7 @@ bench: --test_arg=-test.benchtime=1x \ --test_arg=-test.count=15 \ --test_env=TANGO_REPO_REMOTE=$$(git rev-parse --show-toplevel) \ - --test_env=HOME=$$HOME \ + --test_env=HOME=$$HOME @echo "Benchmarks complete!" # Generate protobuf files using protoc From ce06b4bbc4b2b835f8967be2afb11b4f9c76adc0 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 17 Jul 2026 14:30:35 -0700 Subject: [PATCH 09/11] refactor(integration): use unique commit pairs for cold benchmarks 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 --- Makefile | 3 +- integration/benchmark_test.go | 59 ++++++++++++++++++----------------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/Makefile b/Makefile index fdea067e..0480f664 100644 --- a/Makefile +++ b/Makefile @@ -30,8 +30,7 @@ bench: --test_output=all \ --test_arg=-test.run=^$$ \ --test_arg=-test.bench=. \ - --test_arg=-test.benchtime=1x \ - --test_arg=-test.count=15 \ + --test_arg=-test.benchtime=15x \ --test_env=TANGO_REPO_REMOTE=$$(git rev-parse --show-toplevel) \ --test_env=HOME=$$HOME @echo "Benchmarks complete!" diff --git a/integration/benchmark_test.go b/integration/benchmark_test.go index 50b58d64..a5923916 100644 --- a/integration/benchmark_test.go +++ b/integration/benchmark_test.go @@ -12,51 +12,52 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Benchmarks for GetChangedTargets against fixed, checked-in commit pairs of -// increasing diff size. These are for local performance measurement only: -// run via `make bench`, never via `make test` / `make test-integration`, -// and never from CI. They assert nothing about how fast the call must be. +// Benchmarks for GetChangedTargets against fixed, checked-in commit pairs. +// Each iteration uses a unique pair of commits so every call is a cache miss, +// measuring true cold-path performance without restarting the server. +// Run via `make bench`; not part of `make test` / CI. package integration_test import ( "testing" - "time" "go.uber.org/zap" "go.uber.org/zap/zapcore" ) -func runChangedTargetsBenchmark(b *testing.B, name, firstSHA, secondSHA string) { +// commitPairs are consecutive commits from the repo history. Each pair +// produces a unique treehash, guaranteeing a cache miss per iteration. +var commitPairs = []struct{ first, second string }{ + {"f0a1fae0786faa4b52cdd0753e93a5e6761fcf6c", "aa03d2c7a2404c88d880a879ca8c44c9fcc75b30"}, + {"aa03d2c7a2404c88d880a879ca8c44c9fcc75b30", "7af31074dfa45a0216f5cc8f1866cecc12708dbc"}, + {"7af31074dfa45a0216f5cc8f1866cecc12708dbc", "124cb6cd0c814007d282981efd79598679b1e6c8"}, + {"124cb6cd0c814007d282981efd79598679b1e6c8", "8116956921fb0dc022150d4c11f3bdd629f8da7c"}, + {"8116956921fb0dc022150d4c11f3bdd629f8da7c", "9a5ddf73c73d5e3e17f07a335ed891c1761ed7f1"}, + {"9a5ddf73c73d5e3e17f07a335ed891c1761ed7f1", "83cfae7cd03f4c09a46ff67a5e4db557e3d52bbd"}, + {"83cfae7cd03f4c09a46ff67a5e4db557e3d52bbd", "1991c274dcd9ba790d883745f0dd6ccd8263dec0"}, + {"1991c274dcd9ba790d883745f0dd6ccd8263dec0", "9f33b50aae84ec5b8cfc796f7405991d98423354"}, + {"9f33b50aae84ec5b8cfc796f7405991d98423354", "461bec17db1142f04ff24a653b7e3020dbda4295"}, + {"461bec17db1142f04ff24a653b7e3020dbda4295", "46b851595b0a1eabfa797bd07fde31cee88c6a1d"}, + {"46b851595b0a1eabfa797bd07fde31cee88c6a1d", "4f246dc46d2d3b2e2bf0126a0908ad7176c07833"}, + {"4f246dc46d2d3b2e2bf0126a0908ad7176c07833", "aa4376860c5601ccfcd3b3e39a27f848acd54814"}, + {"aa4376860c5601ccfcd3b3e39a27f848acd54814", "1d99dd6cbe1749f7ae8239db3b606ac5f298a838"}, + {"1d99dd6cbe1749f7ae8239db3b606ac5f298a838", "16ac2aa8dbe8610f575bce5a5fa6834291379885"}, + {"16ac2aa8dbe8610f575bce5a5fa6834291379885", "82a1e99a8ebf9c993e1416e3a3fd91b254d6ef46"}, +} + +func BenchmarkGetChangedTargets(b *testing.B) { remote := repoRemote(b) logger := zap.New(zapcore.NewNopCore()) addr := startServerWithLogger(b, remote, logger) client := newClient(b, addr) - coldStart := time.Now() - getChangedTargets(b, client, remote, firstSHA, secondSHA) - coldDuration := time.Since(coldStart) + if b.N > len(commitPairs) { + b.Fatalf("benchtime=%dx exceeds available commit pairs (%d)", b.N, len(commitPairs)) + } b.ResetTimer() for i := 0; i < b.N; i++ { - getChangedTargets(b, client, remote, firstSHA, secondSHA) + pair := commitPairs[i] + getChangedTargets(b, client, remote, pair.first, pair.second) } - cachedDuration := b.Elapsed() / time.Duration(b.N) - - b.ReportMetric(coldDuration.Seconds(), "first_uncached_call_sec") - b.ReportMetric(cachedDuration.Seconds(), "avg_cached_call_sec/op") -} - -func BenchmarkGetChangedTargets_SmallDiff(b *testing.B) { - // 5716262 -> 74d1cd5: 2 files changed, 13 lines. - runChangedTargetsBenchmark(b, "SmallDiff", "57162624a45965a7e783072c56561f91c5d4084d", "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96") -} - -func BenchmarkGetChangedTargets_MediumDiff(b *testing.B) { - // 046de2c -> 1f2e3e9: 8 files changed, 461 lines. - runChangedTargetsBenchmark(b, "MediumDiff", "046de2c20b5492cd5606d32fd632a38b8b70c8f6", "1f2e3e9245b159006cf2103becd51c5c1b6ec868") -} - -func BenchmarkGetChangedTargets_LargeDiff(b *testing.B) { - // 684a6d0 (repo setup) -> 9bf997c: 162 files changed, ~19.9k lines. - runChangedTargetsBenchmark(b, "LargeDiff", "684a6d06e82cff50a4f1a5addb8f64a45aaa6c26", "9bf997cf709b1ee38a71fa0d1423e66d0494e9bc") } From 36b209b707380f157f05061a65896e6436a6dd98 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 17 Jul 2026 14:35:57 -0700 Subject: [PATCH 10/11] refactor(integration): revert to per-size benchmarks with benchtime=15x Co-Authored-By: Claude Sonnet 5 --- integration/benchmark_test.go | 59 +++++++++++++++++------------------ 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/integration/benchmark_test.go b/integration/benchmark_test.go index a5923916..f0d2b936 100644 --- a/integration/benchmark_test.go +++ b/integration/benchmark_test.go @@ -12,52 +12,51 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Benchmarks for GetChangedTargets against fixed, checked-in commit pairs. -// Each iteration uses a unique pair of commits so every call is a cache miss, -// measuring true cold-path performance without restarting the server. -// Run via `make bench`; not part of `make test` / CI. +// Benchmarks for GetChangedTargets against fixed, checked-in commit pairs of +// increasing diff size. These are for local performance measurement only: +// run via `make bench`, never via `make test` / `make test-integration`, +// and never from CI. They assert nothing about how fast the call must be. package integration_test import ( "testing" + "time" "go.uber.org/zap" "go.uber.org/zap/zapcore" ) -// commitPairs are consecutive commits from the repo history. Each pair -// produces a unique treehash, guaranteeing a cache miss per iteration. -var commitPairs = []struct{ first, second string }{ - {"f0a1fae0786faa4b52cdd0753e93a5e6761fcf6c", "aa03d2c7a2404c88d880a879ca8c44c9fcc75b30"}, - {"aa03d2c7a2404c88d880a879ca8c44c9fcc75b30", "7af31074dfa45a0216f5cc8f1866cecc12708dbc"}, - {"7af31074dfa45a0216f5cc8f1866cecc12708dbc", "124cb6cd0c814007d282981efd79598679b1e6c8"}, - {"124cb6cd0c814007d282981efd79598679b1e6c8", "8116956921fb0dc022150d4c11f3bdd629f8da7c"}, - {"8116956921fb0dc022150d4c11f3bdd629f8da7c", "9a5ddf73c73d5e3e17f07a335ed891c1761ed7f1"}, - {"9a5ddf73c73d5e3e17f07a335ed891c1761ed7f1", "83cfae7cd03f4c09a46ff67a5e4db557e3d52bbd"}, - {"83cfae7cd03f4c09a46ff67a5e4db557e3d52bbd", "1991c274dcd9ba790d883745f0dd6ccd8263dec0"}, - {"1991c274dcd9ba790d883745f0dd6ccd8263dec0", "9f33b50aae84ec5b8cfc796f7405991d98423354"}, - {"9f33b50aae84ec5b8cfc796f7405991d98423354", "461bec17db1142f04ff24a653b7e3020dbda4295"}, - {"461bec17db1142f04ff24a653b7e3020dbda4295", "46b851595b0a1eabfa797bd07fde31cee88c6a1d"}, - {"46b851595b0a1eabfa797bd07fde31cee88c6a1d", "4f246dc46d2d3b2e2bf0126a0908ad7176c07833"}, - {"4f246dc46d2d3b2e2bf0126a0908ad7176c07833", "aa4376860c5601ccfcd3b3e39a27f848acd54814"}, - {"aa4376860c5601ccfcd3b3e39a27f848acd54814", "1d99dd6cbe1749f7ae8239db3b606ac5f298a838"}, - {"1d99dd6cbe1749f7ae8239db3b606ac5f298a838", "16ac2aa8dbe8610f575bce5a5fa6834291379885"}, - {"16ac2aa8dbe8610f575bce5a5fa6834291379885", "82a1e99a8ebf9c993e1416e3a3fd91b254d6ef46"}, -} - -func BenchmarkGetChangedTargets(b *testing.B) { +func runChangedTargetsBenchmark(b *testing.B, name, firstSHA, secondSHA string) { remote := repoRemote(b) logger := zap.New(zapcore.NewNopCore()) addr := startServerWithLogger(b, remote, logger) client := newClient(b, addr) - if b.N > len(commitPairs) { - b.Fatalf("benchtime=%dx exceeds available commit pairs (%d)", b.N, len(commitPairs)) - } + coldStart := time.Now() + getChangedTargets(b, client, remote, firstSHA, secondSHA) + coldDuration := time.Since(coldStart) b.ResetTimer() for i := 0; i < b.N; i++ { - pair := commitPairs[i] - getChangedTargets(b, client, remote, pair.first, pair.second) + getChangedTargets(b, client, remote, firstSHA, secondSHA) } + cachedDuration := b.Elapsed() / time.Duration(b.N) + + b.ReportMetric(coldDuration.Seconds(), "cold-s") + b.ReportMetric(cachedDuration.Seconds(), "cached-s/op") +} + +func BenchmarkGetChangedTargets_SmallDiff(b *testing.B) { + // 5716262 -> 74d1cd5: 2 files changed, 13 lines. + runChangedTargetsBenchmark(b, "SmallDiff", "57162624a45965a7e783072c56561f91c5d4084d", "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96") +} + +func BenchmarkGetChangedTargets_MediumDiff(b *testing.B) { + // 046de2c -> 1f2e3e9: 8 files changed, 461 lines. + runChangedTargetsBenchmark(b, "MediumDiff", "046de2c20b5492cd5606d32fd632a38b8b70c8f6", "1f2e3e9245b159006cf2103becd51c5c1b6ec868") +} + +func BenchmarkGetChangedTargets_LargeDiff(b *testing.B) { + // 684a6d0 (repo setup) -> 9bf997c: 162 files changed, ~19.9k lines. + runChangedTargetsBenchmark(b, "LargeDiff", "684a6d06e82cff50a4f1a5addb8f64a45aaa6c26", "9bf997cf709b1ee38a71fa0d1423e66d0494e9bc") } From e7b5ff15d7f35c30273ebbd0fee51b02b7216287 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 17 Jul 2026 16:12:03 -0700 Subject: [PATCH 11/11] fix(integration): use stable commit pairs for cold benchmarks 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 --- integration/benchmark_test.go | 71 ++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/integration/benchmark_test.go b/integration/benchmark_test.go index f0d2b936..7997bdf3 100644 --- a/integration/benchmark_test.go +++ b/integration/benchmark_test.go @@ -12,51 +12,68 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Benchmarks for GetChangedTargets against fixed, checked-in commit pairs of -// increasing diff size. These are for local performance measurement only: -// run via `make bench`, never via `make test` / `make test-integration`, -// and never from CI. They assert nothing about how fast the call must be. +// Benchmarks for GetChangedTargets. Run via `make bench`; not part of +// `make test` / `make test-integration` and never from CI. package integration_test import ( "testing" - "time" "go.uber.org/zap" "go.uber.org/zap/zapcore" ) -func runChangedTargetsBenchmark(b *testing.B, name, firstSHA, secondSHA string) { +// coldCommitPairs are distinct consecutive commit pairs from repo history. +// Each pair produces a unique treehash, guaranteeing a cache miss per call. +var coldCommitPairs = []struct{ first, second string }{ + {"57162624a45965a7e783072c56561f91c5d4084d", "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96"}, + {"74d1cd55155e5f4f43aa92b4e0146a0c528a0d96", "3d54234a3d4c0d940d651e002c5c79d71f01b120"}, + {"3d54234a3d4c0d940d651e002c5c79d71f01b120", "821c885d304811652fefeeeb2c21e1907bebf7f6"}, + {"821c885d304811652fefeeeb2c21e1907bebf7f6", "ea874d203f37c58b9ba52cde81309d1e28827eaa"}, + {"ea874d203f37c58b9ba52cde81309d1e28827eaa", "c0cd90a8c35e2cc981e32f074f9a55657f44d7dd"}, + {"c0cd90a8c35e2cc981e32f074f9a55657f44d7dd", "8d7e7f93ea68bfb5e37f29df289a54ab8ff79ae1"}, + {"8d7e7f93ea68bfb5e37f29df289a54ab8ff79ae1", "4d41a6f57d2215db7e78c284f7f8f5e13f6ff07d"}, + {"4d41a6f57d2215db7e78c284f7f8f5e13f6ff07d", "b4591e33a40135ed5c2fef8ee4f96db8ab231904"}, + {"b4591e33a40135ed5c2fef8ee4f96db8ab231904", "f2b15a5e058ed0a678f97111080806e26d0239d4"}, + {"f2b15a5e058ed0a678f97111080806e26d0239d4", "fc7289244106a1a21a146729c6f75eb6d17f2649"}, + {"3662f8acd9fffb535e56b670beb0de0d811e4181", "381c6585a765e350326520d4e0a588bc604fa656"}, + {"381c6585a765e350326520d4e0a588bc604fa656", "450d404d4a337ec2809f28c630b126871a88987e"}, + {"450d404d4a337ec2809f28c630b126871a88987e", "3a07829426dc621d37bdbe2038ac85a20bec5ead"}, + {"3a07829426dc621d37bdbe2038ac85a20bec5ead", "7876d3870dd0da4ec94cb49362bb85247b373358"}, + {"7876d3870dd0da4ec94cb49362bb85247b373358", "9cc890fb7a91de8b129a3cc86f362b393655de5b"}, +} + +// BenchmarkGetChangedTargets_Cold measures uncached GetChangedTargets calls. +// Each iteration uses a different commit pair so every call is a cache miss, +// exercising the full pipeline: git checkout, bazel query, compare, stream. +func BenchmarkGetChangedTargets_Cold(b *testing.B) { remote := repoRemote(b) logger := zap.New(zapcore.NewNopCore()) addr := startServerWithLogger(b, remote, logger) client := newClient(b, addr) - - coldStart := time.Now() - getChangedTargets(b, client, remote, firstSHA, secondSHA) - coldDuration := time.Since(coldStart) - b.ResetTimer() for i := 0; i < b.N; i++ { - getChangedTargets(b, client, remote, firstSHA, secondSHA) + pair := coldCommitPairs[i] + getChangedTargets(b, client, remote, pair.first, pair.second) } - cachedDuration := b.Elapsed() / time.Duration(b.N) - - b.ReportMetric(coldDuration.Seconds(), "cold-s") - b.ReportMetric(cachedDuration.Seconds(), "cached-s/op") } -func BenchmarkGetChangedTargets_SmallDiff(b *testing.B) { - // 5716262 -> 74d1cd5: 2 files changed, 13 lines. - runChangedTargetsBenchmark(b, "SmallDiff", "57162624a45965a7e783072c56561f91c5d4084d", "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96") -} +// BenchmarkGetChangedTargets_Cached measures cached GetChangedTargets calls. +// The cache is primed with a single call before the timed loop, so every +// iteration in the loop is a cache hit measuring streaming overhead only. +func BenchmarkGetChangedTargets_Cached(b *testing.B) { + remote := repoRemote(b) + logger := zap.New(zapcore.NewNopCore()) + addr := startServerWithLogger(b, remote, logger) + client := newClient(b, addr) -func BenchmarkGetChangedTargets_MediumDiff(b *testing.B) { - // 046de2c -> 1f2e3e9: 8 files changed, 461 lines. - runChangedTargetsBenchmark(b, "MediumDiff", "046de2c20b5492cd5606d32fd632a38b8b70c8f6", "1f2e3e9245b159006cf2103becd51c5c1b6ec868") -} + firstSHA := "57162624a45965a7e783072c56561f91c5d4084d" + secondSHA := "74d1cd55155e5f4f43aa92b4e0146a0c528a0d96" + + getChangedTargets(b, client, remote, firstSHA, secondSHA) -func BenchmarkGetChangedTargets_LargeDiff(b *testing.B) { - // 684a6d0 (repo setup) -> 9bf997c: 162 files changed, ~19.9k lines. - runChangedTargetsBenchmark(b, "LargeDiff", "684a6d06e82cff50a4f1a5addb8f64a45aaa6c26", "9bf997cf709b1ee38a71fa0d1423e66d0494e9bc") + b.ResetTimer() + for i := 0; i < b.N; i++ { + getChangedTargets(b, client, remote, firstSHA, secondSHA) + } }