Conversation
The Benchmarks workflow ran `go test -bench=. ./...` sequentially over every package, and because `-timeout` applies per package the wall-clock time summed to roughly three hours. Split the run so it can be parallelized: - bench.sh gains --run (benchmark a subset of packages into a .dat) and --aggregate (combine .dat files into one JSON) modes; the legacy "<dir> [--json]" interface is unchanged. - bench_shard.sh emits a GitHub Actions matrix that buckets the packages containing benchmarks into shards. - benchmark.yml becomes setup -> benchmark (shard matrix) -> combine. Each shard uploads its .dat; combine merges them into a single bench_stats.json and, on push to main, uploads once to Conbench. - bench_adapt.py reuses an existing bench_stats.json instead of re-running the suite. Aggregating into one JSON preserves a single Conbench run (no fragmentation).
8b6901a to
8fb9c4c
Compare
Shard 0 was cancelled at the 6h GitHub Actions limit. Two causes: 1. generateJSONData padded each record with make([]byte, 500), i.e. 500 NUL bytes. json.Marshal escapes every one of them as a 6-character \u0000 sequence, so a "500 byte" field became ~3KB of escapes, and goccy/go-json's decodeUnicode memmoves the remainder of the buffer once per escape. Profiling shows 99.5% of the run in runtime.memmove under decodeUnicode. That quadratic blowup accounted for 249 of shard 0's 290 observed minutes, with two cases still unfinished at cancellation. Padding with a printable string instead drops the whole set from hours to ~19s, and the benchmarks now get enough iterations (63-627 vs 1) to report meaningful numbers. 2. go test -timeout does not cover benchmarks -- the testing package calls stopAlarm() before runBenchmarks() -- so the 40m timeout was inert and a runaway shard burned the full 6h instead of failing fast. Wrap the run in timeout(1) instead, signalling QUIT so the test binary dumps goroutines and names the stuck benchmark. The remaining work in shard 0 is ~40m, so shards get a 90m budget and the job a 100m cap.
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Since we've been trying to reduce the CI usage...the aggregate runner minutes used is still about the same, right? 3 hours of worker resources/commit seems pretty heavy
|
Pushed a follow-up in 7978914:
Verified with two real Go benchmark shards, per-package machine attribution and one run ID, missing/conflicting provenance rejection, spaced paths, stale-result replacement, and watchdog completion/expiry. Shell/workflow checks pass; no Conbench upload was performed locally. On total runner use: the last completed benchmark run before this follow-up totaled about 120.9 minutes across the six benchmark jobs, plus about 0.48 minute for setup/combine, with a 41.4-minute benchmark wall span. Those are elapsed job durations, not billed-minute accounting. That is below the previously cited 198–208-minute run, but sharding itself reduces wall time rather than eliminating aggregate work. |
| # The slowest shard (./arrow/array) takes ~40m, so 90m leaves headroom for | ||
| # runner variance while still failing a pathological benchmark ~4x sooner | ||
| # than the 6h GitHub default. | ||
| timeout-minutes: 100 |
There was a problem hiding this comment.
Good catch. There are two budgets and the comment described the wrong one:
shards run under a 90m watchdog (the --timeout passed to bench.sh below),
while the job cap is 100m so the watchdog trips first and names the stuck
benchmark, with the extra 10m covering checkout, setup and upload. The comment
sat on the cap but explained the watchdog. Rewritten to cover both and why they
differ.
| - name: Install Go for Benchmarks | ||
| uses: actions/setup-go@v7.0.0 | ||
| with: | ||
| go-version: '1.26.1' |
There was a problem hiding this comment.
We can't take this from go.mod?
There was a problem hiding this comment.
Done, and this was a regression introduced here: the version used to come from
the one-value go: ['1.26.1'] matrix that this branch replaced with shard data,
which left 1.26.1 duplicated as a literal in both the benchmark and
combine jobs, free to drift apart. Both now use go-version-file: go.mod,
matching rc.yml.
One heads-up on the consequence, since it may not be what you expected. Our
go.mod has no toolchain directive and declares go 1.25.0, and setup-go
uses the go directive verbatim when it carries a patch
version,
so this pins the benchmarks to exactly Go 1.25.0 rather than the 1.26.1 they
were running — our compatibility floor, not the toolchain we had been
benchmarking on, and a patch behind the GO=1.25.8 in .env. For Conbench that
means a one-time step in the timeseries and benchmarking a minor behind what
test.yml already covers (1.25/1.26/1.27).
I went with what you asked since it removes the duplication and is easy to
revert. If you would rather keep the benchmark toolchain where it was, I am
happy to switch to a workflow-level env: GO_VERSION used by both jobs, or read
GO from .env the way test.yml does.
Two review comments on the benchmark job: The timeout-minutes comment described the wrong number. Shards run under a 90m watchdog (--timeout) while the job cap is 100m; the comment sat on the cap but explained the watchdog, reading as though 100m were the value that leaves headroom. Describe both budgets and why they differ: the watchdog is what trips first and names the stuck benchmark, and the extra 10m covers checkout, setup and upload. The Go version was hardcoded. Before sharding, the version came from the one-value matrix that this branch replaced with shard data, which left '1.26.1' duplicated as a literal in the benchmark and combine jobs, free to drift apart. Take it from go.mod instead, matching rc.yml. Note this moves the benchmark toolchain from 1.26.1 to 1.25.0: go.mod has no toolchain directive, and setup-go uses the go directive verbatim when it carries a patch version. Signed-off-by: Matt Topol <matt@columnar.tech>
2 participants
Rationale for this change
The
Benchmarksworkflow runsgo test -bench=. ./...sequentially across everypackage. Because
go test's-timeoutis applied per package, the wall-clock timeis the sum over all packages — recent
mainruns have taken roughly 3.3 hours(198–208 min).
What changes are included in this PR?
Split the benchmark run so it can be parallelized, then combine the results into a
single upload:
ci/scripts/bench.sh— adds--run(benchmark a subset of packages, writingraw output to a
.dat) and--aggregate(merge one or more.datfiles into asingle
bench_stats.jsonviagobenchdata) modes. The existingbench.sh <dir> [--json|-json]interface is unchanged, so nothing else that callsit needs to change.
ci/scripts/bench_shard.sh(new) — prints a GitHub Actions matrix that bucketsthe packages containing benchmarks into N shards.
.github/workflows/benchmark.yml— reworked into three jobs:setup(computethe shard matrix) →
benchmark(matrix; each shard runs its packages and uploadsits
.dat) →combine(download all.dat, aggregate into onebench_stats.json,and — only on push to
main— upload once to Conbench).ci/scripts/bench_adapt.py— reuses an existingbench_stats.json(produced bycombine) instead of re-running the whole suite.Because the shards are merged into one JSON and uploaded once, Conbench still sees a
single run (no
run_idfragmentation).Are these changes tested?
Locally:
shellcheckclean on both scripts;actionlintclean on the workflow;py_compileOK on
bench_adapt.py.--runon two packages, then--aggregateproduced one
bench_stats.jsoncontaining both suites, in the exact shapebench_adapt.pyconsumes.bench.sh <dir> --jsonpath still runs → aggregates → cleans up.Opened as a draft to exercise the reworked workflow in CI end to end (it triggers
on changes to these files).
Are there any user-facing changes?
No. This only touches CI / benchmark tooling.
Notes / follow-ups
shard can hold two heavy packages (e.g.
arrow/compute+parquet/internal/encoding)and become the long pole. The per-package
-timeout(40m) remains the hard floor forany single package. Once this runs, per-shard timings can seed a runtime-weighted
split or tune the shard count.