Skip to content

Commit 3109178

Browse files
committed
fix(bench): make the gate loud on dropped data and the Makefile its single source
Review fixes for the bench-gate PR: - Silence the sync engine's global logger inside the sync benchmarks: interleaved log.Printf output corrupted the 'BenchmarkX' result lines, so benchfmt parsed zero sync results and all three sync benchmarks silently dropped out of the gate on both sides. - benchgate now treats unparseable result lines as a corrupted capture (report + exit 2) instead of skipping benchfmt syntax errors, and reports a gated unit missing from one side ('missing from baseline/candidate, not gated') instead of a bare continue. Custom ReportMetric units are reported as ungated. - Split the sample-count policy: too few candidate samples stays a config error, but a short baseline (a legitimately partial base run) is reported and not gated. - Print detected regressions even when a config issue forces exit 2, so a real violation is never hidden behind a configuration error. - bench.yml now runs 'make bench-gate' on both sides; the Makefile's BENCH_GATE_PACKAGES/COUNT/TIME are the single source of truth, and each side benchmarks its own commit's package list, so growing the gate cannot empty the baseline. A failing base run degrades to its partial output plus a workflow warning instead of truncating the whole baseline into a vacuous pass. Run steps use set -euo pipefail and the workflow gains a Go-paths filter. - Absorb the leading-edge O(history) signal recompute before the timer in BenchmarkSyncPathsIncrementalAppend (it was ~58% of the measured allocs, masking steady-state regressions), stretch the debounce window so the flush timer cannot fire nondeterministically inside a worst-run-gated loop, and pre-build appended lines. - Build BenchmarkInsertMessagesBatch's 200-message fixture once outside the timed loop; only the SessionID is rewritten per iteration. - Widen testDB to testing.TB, drop the bench-only openBenchDB copy, and fix BenchmarkGetDailyUsage's detached testing.T hack (its require failures and cleanups were wired to nothing). - Split benchgate's compare()/main() into evalGate/compareUnits/ render/parseFlags to meet function-size and complexity limits, and drop the dead NaN guard. - Document the new mechanics in docs/internal/performance-gates.md and reformat it with mdformat --wrap 80.
1 parent 6e01b26 commit 3109178

9 files changed

Lines changed: 604 additions & 263 deletions

File tree

.github/workflows/bench.yml

Lines changed: 34 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -11,19 +11,33 @@ name: Bench Gate
1111
# - bulk ingest throughput (#411)
1212
# - per-row query-shape regressions in usage aggregation (#309)
1313
#
14-
# cmd/benchgate gates on allocs/op and B/op (deterministic on a given
15-
# machine, tight thresholds) and on ns/op with a loose 2x threshold
16-
# that only catches algorithmic blowups; both sides run on the same
17-
# runner within one job, so the comparison is apples to apples.
14+
# Both sides run `make bench-gate` — the Makefile is the single
15+
# source of truth for the gated package list, sample count, and
16+
# iteration count — and cmd/benchgate compares the outputs. It gates
17+
# on allocs/op and B/op (deterministic on a given machine, tight
18+
# thresholds) and on ns/op with a loose 2x threshold that only
19+
# catches algorithmic blowups; both sides run on the same runner
20+
# within one job, so the comparison is apples to apples.
21+
#
1822
# Benchmarks that only exist on one side are reported but never fail
19-
# the gate — which is what makes running every benchmark in the
20-
# gated packages safe: a benchmark added by a PR has no baseline and
21-
# is reported without gating, then gates automatically once merged.
22-
# BENCH_PACKAGES is the gate boundary; keep it in sync with the
23-
# Makefile's bench-gate target.
23+
# the gate: a benchmark added by a PR has no baseline and is reported
24+
# without gating, then gates automatically once merged. Because each
25+
# side benchmarks its own Makefile's package list, a PR that adds a
26+
# package to the gate cannot break the base run. A partially failing
27+
# base run degrades to a partial baseline (whatever benchmarks it
28+
# produced still gate) rather than silently disabling the whole gate.
2429

2530
on:
2631
pull_request:
32+
# Docs/frontend-only PRs cannot change the gated Go paths. If
33+
# this check is ever made required on branch protection, pair it
34+
# with a no-op sibling workflow on the inverse paths.
35+
paths:
36+
- "**.go"
37+
- "go.mod"
38+
- "go.sum"
39+
- "Makefile"
40+
- ".github/workflows/bench.yml"
2741

2842
concurrency:
2943
group: bench-${{ github.head_ref || github.ref }}
@@ -32,19 +46,6 @@ concurrency:
3246
permissions:
3347
contents: read
3448

35-
env:
36-
BENCH_PACKAGES: ./internal/sync ./internal/db ./internal/secrets
37-
# Must stay >= 5: benchgate's time gate needs at least 5 samples
38-
# per side for the significance test and treats fewer as a
39-
# configuration error.
40-
BENCH_COUNT: "6"
41-
# Fixed iterations (not a duration) so the baseline and candidate
42-
# run identical workloads: two of the gated benchmarks grow their
43-
# fixture as they iterate (appending to a session, adding sessions
44-
# to a DB), and a duration-based -benchtime would let a slower side
45-
# run fewer iterations against a different final size.
46-
BENCH_TIME: 20x
47-
4849
jobs:
4950
bench-gate:
5051
name: Benchmark Gate
@@ -59,38 +60,26 @@ jobs:
5960
with:
6061
go-version-file: go.mod
6162

62-
- name: Restore pricing snapshot
63-
run: go run ./internal/pricing/cmd/litellm-snapshot -restore
64-
6563
- name: Run benchmarks (PR head)
66-
env:
67-
CGO_ENABLED: "1"
6864
run: |
69-
set -o pipefail
70-
go test -tags "fts5" -run '^$' -bench . \
71-
-benchmem -count "$BENCH_COUNT" -benchtime "$BENCH_TIME" \
72-
-timeout 25m $BENCH_PACKAGES | tee /tmp/bench-new.txt
65+
set -euo pipefail
66+
make -s bench-gate | tee /tmp/bench-new.txt
7367
7468
- name: Run benchmarks (merge base)
7569
env:
76-
CGO_ENABLED: "1"
7770
BASE_REF: ${{ github.base_ref }}
78-
# A failing merge-base run (or one with none of the gated
79-
# benchmarks yet) degrades to an empty baseline: benchgate
80-
# then reports every benchmark as new and passes, so a broken
81-
# base never blocks a PR. The baseline file is truncated on
82-
# failure so partial output from packages that ran before the
83-
# failure cannot gate against incomplete data.
71+
# A failing merge-base run keeps whatever benchmark output it
72+
# produced: go test emits results per package, so one broken
73+
# package (or a base predating the bench-gate target) leaves
74+
# a partial or empty baseline and benchgate gates only what
75+
# exists on both sides. The warning makes the degraded run
76+
# visible instead of a silently green vacuous pass.
8477
run: |
78+
set -euo pipefail
8579
base=$(git merge-base HEAD "origin/$BASE_REF")
8680
git worktree add /tmp/bench-base "$base"
87-
cd /tmp/bench-base
88-
go run ./internal/pricing/cmd/litellm-snapshot -restore || true
89-
if ! go test -tags "fts5" -run '^$' -bench . \
90-
-benchmem -count "$BENCH_COUNT" -benchtime "$BENCH_TIME" \
91-
-timeout 25m $BENCH_PACKAGES > /tmp/bench-old.txt; then
92-
echo "merge-base benchmarks failed; comparing against empty baseline"
93-
: > /tmp/bench-old.txt
81+
if ! make -s -C /tmp/bench-base bench-gate > /tmp/bench-old.txt; then
82+
echo "::warning title=Bench Gate::merge-base benchmark run exited non-zero; gating against its partial output"
9483
fi
9584
9685
- name: Compare against merge base

Makefile

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -267,14 +267,15 @@ bench-backends: pricing-snapshot ensure-embed-dir
267267
CGO_ENABLED=1 go test -tags "fts5,benchdb" ./internal/backendbench $(BENCH_BACKENDS_FLAGS)
268268

269269
# Hot-path benchmark gate. Runs every benchmark in the gated packages
270-
# — the same set CI's bench.yml compares against a PR's merge base
271270
# (sync engine warm/cold/append, message write paths, usage
272-
# aggregation, secret scanning). Run it before and after touching a
273-
# sync or DB hot path, then compare with
274-
# `go run ./cmd/benchgate -old old.txt -new new.txt`. Keep the
275-
# package list in sync with BENCH_PACKAGES in bench.yml.
271+
# aggregation, secret scanning). This target is the single source of
272+
# truth for the gate configuration: CI's bench.yml runs it on both
273+
# the PR head and the merge base, then compares the outputs with
274+
# `go run ./cmd/benchgate -old old.txt -new new.txt`. Run it before
275+
# and after touching a sync or DB hot path.
276+
BENCH_GATE_PACKAGES ?= ./internal/sync ./internal/db ./internal/secrets
276277
# Count must stay >= 5: benchgate's time gate needs at least 5
277-
# samples per side for its significance test.
278+
# candidate samples for its significance test.
278279
BENCH_GATE_COUNT ?= 6
279280
# Fixed iterations, not a duration: some gated benchmarks grow their
280281
# fixture as they iterate, so baseline and candidate must run the
@@ -284,7 +285,7 @@ bench-gate: pricing-snapshot ensure-embed-dir
284285
CGO_ENABLED=1 go test -tags "fts5" -run '^$$' \
285286
-bench . -benchmem \
286287
-count $(BENCH_GATE_COUNT) -benchtime $(BENCH_GATE_TIME) \
287-
-timeout 25m ./internal/sync ./internal/db ./internal/secrets
288+
-timeout 25m $(BENCH_GATE_PACKAGES)
288289

289290
# Start test PostgreSQL container
290291
postgres-up:

0 commit comments

Comments
 (0)