ci: compare benchmarks on the same runner - #2271
Conversation
There was a problem hiding this comment.
Review summary
Solid refactor: extracting the benchmark steps into benchmark/baseline/run.sh and running the base + current suites sequentially on the same runner is a clean way to eliminate cross-machine variance. Shell hygiene is good — set -euo pipefail, per-stage subshells for cd isolation, quoted expansions, arg-count validation, and pipefail correctly makes the | tee pipelines fail-fast. The README changes accurately match the workflow and script behavior, and the CI security posture is correct (pull_request trigger + contents: read + no secrets + persist-credentials: false); this PR does not change that posture since PR code was already built/executed here.
The findings below are about CI cost, measurement bias, and a couple of robustness nits — none are blocking.
CI cost & measurement (not inline — spans the workflow)
-
Doubled runtime vs. the 20-minute cap (
.github/workflows/benchmark.yml:28,:52-65). The new "Measure pull request base" step adds a full secondrun.shpass on every PR: anothergo build -p=1 ./cmd/llgoof the compiler plus all collector runs (3 workloads × 3 builds + 7 runs) and threego test/llgo testbenchmark stages. With-p=1and mostlyGOMAXPROCS=1there is little parallelism to absorb the doubling, andmacos-latestis slower. Worth confirming both runners have comfortable headroom undertimeout-minutes: 20, or bumping it / caching the base compiler build (build time is pure overhead, not a measured metric). -
Fixed base→current ordering bias (
.github/workflows/benchmark.yml:52-65). Running base then current back-to-back removes cross-machine variance but introduces a systematic warm-up bias: the first suite runs cold (disk/thermal/toolchain caches), the second warm. Since the order is always base-first, sub-few-percent deltas may be dominated by ordering noise (short-benchtime=250ms, small sample counts give little power to average it out). Consider documenting that small deltas are within noise, or alternating order.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
|
Review follow-up:
A complete local relative-path run with warm-up and three samples took 83.49s versus 69.93–73s before the review changes. Moving from three to five samples should add only a few more seconds per suite; the refreshed hosted jobs will provide the final cost and variance check. |
|
Five-sample local follow-up: the complete suite finished in 87.72s and exported exactly five samples per microbenchmark. Compared with the 69.93s one-sample warm-cache run, this adds 17.79s per suite, so a paired PR job should add roughly 36s—not minutes—over the first hosted paired run. The refreshed estimate is about 5m43s on Linux and 6m14s on macOS, still with wide headroom under 20 minutes. |
cpunion
left a comment
There was a problem hiding this comment.
Direct response to the review on 2326692:
-
CI cost / timeout: confirmed on both hosted runners. The original paired run was 5m07s on Linux and 5m38s on macOS. The five-sample revision was 5m01s on Linux and 7m03s on macOS, so both retain substantial headroom under the unchanged 20-minute timeout. No timeout increase is needed.
-
Fixed base→current ordering: confirmed as a real limitation. The original single-sample run produced false 20–48% changes. Commits 49ba5ee and df0bb22 now use the same source path, warm program builds before timing, record five microbenchmark samples, and explicitly document that small deltas require repeated workflow confirmation. Linux false deltas fell to 0–1.23% and all size metrics became identical. macOS still shows a phase/core-scheduling effect across the two whole-suite processes, so increasing benchtime alone would not solve it; the PR now documents this remaining noise rather than treating every delta as conclusive. Alternating at sample granularity would require a larger paired-runner refactor and materially more runner time, so it is not added here.
-
Inline robustness comments:
go.txtreset plus append-onlytee, absolute path normalization with a complete relative-path test, and the current-harness/base compatibility comment are implemented in 49ba5ee. Each original inline thread has a direct reply and is resolved.
Summary
setup-benchmark-go-action@v1, so the PR comment comparesvs basefrom the same runner instead of historical data from another machine.benchmark/baseline/run.shso both revisions use the same current-checkout harness.The paired artifact/publisher support was added in xgo-dev/setup-benchmark-go-action#3.
Runtime impact
Across the latest 15 successful benchmark runs before this change:
The first hosted paired run completed in 5m07s on Linux and 5m38s on macOS, about 50% above the previous medians rather than twice as long. After the review stability changes, Linux completed in 5m01s and macOS in 7m03s.
A complete local five-sample suite on macOS/Apple M4 Max took 87.72s, versus 69.93–73s with one sample. Thus main remains a single suite with roughly 15–20s added for warm-up and sampling, while PR jobs remain around 5–6 minutes with wide headroom under 20 minutes.
Stability checks
The first paired report exposed false changes even though compiler code was unchanged: macOS
fmtprintfbuild showed -48.6%, several microbenchmarks moved 20–35%, and binary sizes differed. The review update now:On the refreshed Linux artifact, all binary size metrics are identical, microbenchmark deltas are 0–1.23%, and the largest program build/run delta is 2.50%.
Validation
bash -n benchmark/baseline/run.shshellcheck benchmark/baseline/run.shactionlint .github/workflows/benchmark.ymlgo test ./benchmark/baseline -count=1vs base, trusted same-runner footer)