Repository navigation
ci(perf): GOMEMLIMIT with GOGC=off, pinned toolchain, cachegrind I1, matrix mode [W0-15] - #921
ajitpratap0 wants to merge 9 commits into
Conversation
…matrix mode [W0-15] Perf-gate methodology fixes from checkpoint C0: - Every GOGC=off measurement also sets GOMEMLIMIT (default 64MiB, shared in scripts/perf/lib.sh; PERF_GOMEMLIMIT / the gomemlimit dispatch input override it): ab.sh primary arms and instruction counts, profile.sh, and the competitor harness (cmd/publish -gomemlimit, recorded in configs[].gomemlimit). Default-GC arms unset it. - perf-gate.yml and bench-compare.yml build with one pinned Go release, PERF_GO_VERSION 1.26.9, check-latest off, GOTOOLCHAIN=local. - Profile mode runs cachegrind cache simulation (32 KiB 8-way L1i) of the gap queries and lists the functions with the most L1i misses. - New matrix dispatch mode: interleaved A/B over the full benchmark matrix generated by scripts/perf/matrix-suite.sh. - Every artifact records the runner CPU model; the gate report names it and states that only in-job ratios are compared; the competitor harness summary leads with GoSQLX/peer time ratios (-ratios). - setupTestServer in pkg/lsp discards output so benchmarks that publish diagnostics stop timing a growing buffer (W0-06 follow-up). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @ajitpratap0's task in 43s —— View job Review of PR #921: perf-gate methodology (W0-15)I read the diff for Overall: the change is well structured. Putting the GOMEMLIMIT default in one Things I checked that look correct
Suggestions
Test coverageCoverage of the shell helpers through Go tests is strong. Testing No blocking issues found. |
Perf gate: PASS (shadow mode: report only, this check never fails)Base Runner CPU:
Rules: time regresses at >= +3% with p < 0.01 (Mann-Whitney U), or a suite geomean >= +1.5% (per-round geomeans, same test); allocs/op must not increase; B/op must not rise more than 2%; instructions/op must not rise more than 1.5%. An A/A control with more than 10% of benchmarks moving over 3% is noisy: re-run once, then inconclusive (neutral). Gated suite (GOGC=off, GOMAXPROCS=1)
A/A control: 2 of 32 benchmarks moved more than 3%. Instructions per operation (cmd/perfdriver)
Secondary metric: default GC, GOMAXPROCS=1 (informational)
Environmentbenchstat (primary metric) |
The words of one `local` are expanded before its assignments, so `local q=$1 n=$2 log=...-$n.log` read an unset $n and set -u aborted every cachegrind run (the first profile dispatch reported "cachegrind failed" for all queries). Split the declaration and add an end-to-end test that runs profile.sh with a stub valgrind and checks the per-operation table. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 21s —— View job Review of PR #921: perf-gate methodology (W0-15)I read Overall: the change is sound and stays inside the tooling. The CI perf-gate shadow run passes. With GOMEMLIMIT 64MiB, the A/A control shows 0 of 26 benchmarks moving more than 3%. Checked and correct
Suggestions
Test coverageCoverage is strong. It includes the workflow pin checks, the GOMEMLIMIT-on-every-GOGC=off check, the lib.sh and |
Resolves the CHANGELOG.md [Unreleased] conflict with #917 (W0-12) by keeping both its ### Added block and the W0-15 ### Changed block. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
…trix reports [W0-15] Review fixes after merging #917 (W0-12): - task bench:matrix sets GOMEMLIMIT=64MiB next to GOGC=off; the GOMEMLIMIT lint now understands YAML env blocks with the keys on separate lines. - docs/PERFORMANCE_METHODOLOGY.md documents GOMEMLIMIT and its effects (GC share in time and Ir/op, pool drains), the pinned PERF_GO_VERSION, the matrix and profile (cachegrind) dispatch modes and ratio-only reports. - scripts/perf/baseline keeps GOGC=off without GOMEMLIMIT on purpose (it gates absolute allocs/op and B/op, exact only without pool drains), drops an inherited GOGC/GOMEMLIMIT, and is covered by the lint through an explicit, file-restricted exemption marker. - ab.sh comment on allocs/op corrected: the limit lets GC and pool drains run; allocs/op stays exact for the gated suite only. - Matrix dispatch: job "Matrix A/B (report only)", report heading "Matrix A/B: VERDICT (report only, not a gate verdict)" via ab.sh -T and gate -report-only/-title. - PERF_GOMEMLIMIT / the gomemlimit input are validated up front (perf_check_gomemlimit) instead of every benchmark binary panicking. - ab.sh die() no longer appends the exit code to its message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 21s —— View job Review of PR #921: perf-gate methodology (W0-15)I read Overall: the change is sound and stays inside tooling. The shadow gate passed (geomean -0.19%, A/A 0 of 26). I found no blocking issues. Checked and correct
Suggestions (non-blocking)
Test coverageCoverage is strong. The |
CodeQL flagged the new "Validate inputs" step (actions/cache-poisoning): it ran scripts/perf/lib.sh from the checked-out, dispatch-chosen ref. The check is now inlined and runs before checkout; a test runs the inlined block against perf_check_gomemlimit so the two accept the same values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 18s —— View job Review of PR #921: perf-gate methodology (W0-15)I read Overall: the change is sound and stays inside the tooling. The shadow gate passes (no gated metric moved, A/A 0 of 26). I found no blocking issues. Checked and correct
Suggestions (non-blocking)
Test coverageCoverage is strong for tooling that otherwise only fails in CI. The |
…0-15] The GOMEMLIMIT lint checked the whole line, so the one-line if/else in ab.sh run_arm let the default-GC branch's `-u GOMEMLIMIT` cover a bare GOGC=off in the gcoff branch. Check each GOGC=off token's own segment (delimited by ( ) ; | &) and pin the shapes in TestGCOffSegmentLint. Add 1PiB/1EiB to the GOMEMLIMIT validation lists so a [KMGTP] suffix mutation is caught, and point the ab.sh allocs caveat at docs/PERFORMANCE_METHODOLOGY.md instead of the PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2
|
Claude finished @ajitpratap0's task in 21s —— View job Review of PR #921: perf-gate methodology (W0-15)I read the Confirmed in the code (all non-blocking)
Test coverageCoverage is strong for tooling that otherwise only fails in CI. That includes the |
|
Claude finished @ajitpratap0's task in 18s —— View job Review of PR #921: perf-gate methodology (W0-15)I confirmed one point in the code, Still open (all non-blocking, repeated from earlier reviews)
The CHANGELOG diff adds 41 lines. I did not open it, so I can't say whether it states that GOGC=off timings from before and after this change are not comparable. The PR body does state it. Test coverageCoverage is strong for tooling that otherwise only fails in CI. That includes the |
Summary
Perf-gate methodology fixes from checkpoint C0 ("Methodology fixes", 2.4). Tooling and benchmark harness only; no library code changes.
GOMEMLIMIT next to every GOGC=off measurement (default
64MiB, one definition in the newscripts/perf/lib.sh; override withPERF_GOMEMLIMITor the newgomemlimitdispatch input,offunsets it). Applied inab.sh(primary arms and perfdriver instruction counts),profile.sh(gcoff profiles, perf stat) and the competitor harnesscmd/publish(-gomemlimit, recorded asconfigs[].gomemlimit, shown incommand). Default-GC arms now unset GOMEMLIMIT so an inherited value cannot leak in. Why 64MiB: in the C0 A/A run,ParseE2E/simple_select, the first benchmark of its binary, gained nothing from GOGC=off while every other query gained 21-25%. That first benchmark grows the fresh heap and pays the first-touch page faults (ParseRW/postgresql/medium, the first of its line, showed ±23% spread). The limit has to sit below what one 200 ms run allocates (about 100 MB) or it changes nothing. Each collection of the tiny live heap costs about 0.5 ms (gctrace), so 64MiB keeps the collector to about 1% of a GOMAXPROCS=1 run. The CI A/A runs below compare off, 64MiB and 256MiB.cachegrind cache simulation in profile mode:
--cache-sim=yes --I1=32768,8,64 --D1=32768,8,64 --LL=33554432,16,64(the Zen 3/4/5 L1i geometry, so counts compare across runner CPUs) overcmd/perfdriver -case e2e/<q>, two iteration counts differenced. The summary shows Ir, I1 and LLi misses per op and the I1 miss rate.cg-top-<q>.txtlists functions by I1 misses. The profile job now installs valgrind.One pinned Go toolchain:
perf-gate.ymlandbench-compare.ymlboth usePERF_GO_VERSION: '1.26.9'(check-latest: false,GOTOOLCHAIN=local). The gate used to resolve 1.26.1 fromgo.mod.Full-matrix dispatch mode (
mode=matrix): the same interleaved A/B (no A/A arm, no Ir) over every benchmark package thattask bench:matrixruns.scripts/perf/matrix-suite.shgenerates the list (about 900 benchmarks), so new benchmark packages join without a file edit. The defaults are 5 rounds of 100 ms, roughly an hour, with a job timeout of 240 min. It only reports. With base = head it is a whole-matrix A/A. It was cheap to add, so it is in.CPU model in every artifact, ratio-only reports:
env.txtof the gate, matrix and profile gets acpu model:line, and bench-compare gets a newenv.txt. The gate report now opens withRunner CPU: ...and says only in-job ratios are compared. The profile summary names the CPU. The bench-compare job summary leads with a GoSQLX/peer time-ratio table (cmd/publish -ratios: geomean per cell × peer × config, plus the worst group). Absolute benchstat output moves into collapsed sections labelled "this job and this CPU only".W0-06 follow-up:
setupTestServer(used by the existing LSP benchmarks and by W0-06'sBenchmarkLSPmatrix in test(bench): lint, security and LSP benchmark matrix [W0-06] #910) now writes toio.Discardinstead of a growingbytes.Buffer.Review fixes after merging feat(bench): per-arch baselines, matrix/pgo tasks, methodology doc [W0-12] #917 (W0-12) (commits
35a036ecand44249405, mergeb0a3df4a):task bench:matrix(added by feat(bench): per-arch baselines, matrix/pgo tasks, methodology doc [W0-12] #917) setsGOMEMLIMIT: '64MiB'next toGOGC: 'off'; its desc says why. The GOMEMLIMIT lint (TestGCOffMeasurementsSetGOMEMLIMIT) now reads YAML env blocks whose keys sit on separate lines; deleting the new line makes it fail again (mutation checked).docs/PERFORMANCE_METHODOLOGY.mddocuments the primary metric asGOGC=off GOMEMLIMIT=64MiB GOMAXPROCS=1, why the limit is there and what it costs (GC share in time and in instructions/op, pool drains), the pinnedPERF_GO_VERSION1.26.9, thematrixandprofile(cachegrind) dispatch modes, and ratio-only reporting.scripts/perf/baselinekeepsGOGC=offwith no GOMEMLIMIT on purpose. It gates absolute allocs/op and B/op only, and those are exact for every benchmark only when no collection drains async.Pool; its ns/op are trend-only. It now also drops an inheritedGOGC/GOMEMLIMIT(gcOffEnv,TestGCOffEnvHasNoMemoryLimit), and the file comment it writes says "no GOMEMLIMIT". The lint now scansscripts/perf/baseline/main.gotoo; the exemption is an explicitperf:no-gomemlimitmarker that the test allows only in that file.amd64.json/arm64.jsonare not re-recorded (orchestrator's call).scripts/perf/ab.shcomment corrected: the limit lets the collector and pool drains run, so allocs/op is exact for the gated suite but not for large-input or low-N benchmarks (documented indocs/PERFORMANCE_METHODOLOGY.md).perfdriverand cachegrind run with the limit too, so Ir/op carries a small GC share proportional to B/op (about 0.25% forjoin). Stated in the CHANGELOG and the methodology doc.Matrix A/B (report only), headingMatrix A/B: VERDICT (report only, not a gate verdict)(newab.sh -T,gate -report-only -title;-Twith-eis refused).TestReportOnlyHeadingIsNotAGateVerdict.PERF_GOMEMLIMIT/thegomemlimitinput are validated up front (perf_check_gomemlimitin lib.sh:offor digits with an optional B/KiB/MiB/GiB/TiB suffix, the forms the runtime accepts), inab.sh/profile.sh, and in a new "Validate inputs" workflow step. That step is inlined and runs before checkout (commit44249405): the first version sourced lib.sh from the dispatch-chosen checkout and CodeQL flagged it asactions/cache-poisoning(high).TestGOMEMLIMITIsValidatedandTestWorkflowGOMEMLIMITCheckMatchesLib(runs the inlined block againstperf_check_gomemlimitfor 14 values; a mutation of the workflow block is caught).ab.sh'sdieno longer appends the exit code to its message.Second review round (commit
b1156f37, merge of origin/maina9a2d60dbringing in perf(tokenizer): read the clock only when metrics are enabled [W1-02] #923):GOGC=offtoken's own shell segment (delimited by( ) ; | &), not the whole line. Before, the one-line if/else inab.shrun_armlet the default-GC branch's-u GOMEMLIMITcover a bareGOGC=offin the gcoff branch.TestGCOffSegmentLintpins the shapes, including that exact line.1PiBand1EiB(suffixes the runtime rejects) are in the reject lists ofTestGOMEMLIMITIsValidatedandTestWorkflowGOMEMLIMITCheckMatchesLib(now 17 values).ab.shheader points the allocs/op caveat atdocs/PERFORMANCE_METHODOLOGY.mdinstead of this PR.Mutations checked (each makes
go test ./scripts/perf/...fail, then reverted): dropping the TaskfileGOMEMLIMIT; reverting the ab.sh perfdriver env; reverting the profile.sh gcoff env; breaking the workflow validate block; dropping pkg/lsp from matrix-suite; reverting the ab.shrun_armgcoff arm toenvs=(GOGC=off GOMAXPROCS=1)(caught byTestGCOffMeasurementsSetGOMEMLIMITatab.sh:242sinceb1156f37); widening the lib.sh suffix to[KMGTP]i(caught by both GOMEMLIMIT validation tests via1PiBsinceb1156f37).Tests (written first, each seen failing before the fix;
TestProfileCachegrindTablewas added after the first profile dispatch caught thelocalbug):scripts/perf/gate/config_test.go:PERF_GO_VERSION, with nogo-version-fileandcheck-latest: falseon every setup-go;cmd/publishdefault;perf_gcoff_envoutput works throughenv(1), includingoffunsetting an inherited limit;perf_cpu_modelis non-empty;perf_cachegrind_countsparses a valgrind 3.22 summary, an Ir-only summary, and fails withoutI refs;matrix-suite.shcovers exactly the benchmark packages found by an independent walk. A mutation that drops pkg/lsp is caught.gate_test.goTestReportNamesTheRunnerCPU: thecpu model:line, the lscpu fallback andunknown.cmd/publish:TestEnvForGOGC(off sets GOMEMLIMIT and replaces an inherited one, default drops both),TestMethodCommandIsCopyPasteable(off configs carry theGOGC=off GOMEMLIMIT=64MiBprefix), andTestRatioMarkdown(geomean ratios, worst group, CPU line, no absolute ns).pkg/lspTestSetupTestServerDiscardsOutput.Test Plan
Current head
a9a2d60d(second review roundb1156f37+ merge of origin/maind29c6446):task check: pass (EXIT 0)task test:cbinding: passgo test -count=1 ./pkg/compatibility/...: passgo run honnef.co/go/tools/cmd/staticcheck@latest ./scripts/perf/gate ./scripts/perf/baseline: cleango test -count=1 ./scripts/perf/...andbenchmarks/compare:go test -count=1 ./cmd/publish: passMatrix A/B (report only), heading## Matrix A/B: REGRESSION (report only, not a gate verdict), Runner CPU line, go1.26.9. The REGRESSION label in this A/A comes only from "allocs/op increased in 31 benchmark(s)": at 10 ms and one round N is tiny, so the documented low-N allocs/op variance under the limit shows in more benchmarks than the 52/907 seen at 50 ms. Matrix mode is report-only; nothing fails.env.txt(cpu model:line, go1.26.9) andratios.md(Runner CPU line, GoSQLX/peer ratio table over the four configs).Earlier heads:
After merging origin/main (
b0a3df4a) and the review fixes (35a036ec,44249405; gate re-run on each):task check: pass (EXIT 0)task test:cbinding: passgo test -count=1 ./pkg/compatibility/...: passgo run honnef.co/go/tools/cmd/staticcheck@latest ./scripts/perf/gate/ ./scripts/perf/baseline/: cleango run github.com/rhysd/actionlint/cmd/actionlint@v1.7.7 .github/workflows/perf-gate.yml .github/workflows/bench-compare.yml: cleantask bench:matrix FILTER='^BenchmarkParseE2E$/simple' BENCHTIME=1x: runs;scripts/perf/ab.sh -T x -eandPERF_GOMEMLIMIT=64Mi scripts/perf/{ab,profile}.shexit 2 with a clear message.Before the merge (head
1b970a29):task check: passtask test:cbinding: passgo test -count=1 ./pkg/compatibility/...: passgo run honnef.co/go/tools/cmd/staticcheck@latest ./scripts/perf/gate/ ./pkg/lsp/, and inbenchmarks/compare:./cmd/publish/: clean.go vet ./...andgo test -race ./cmd/publish/inbenchmarks/compare: passgo run github.com/rhysd/actionlint/cmd/actionlint@v1.7.7 -shellcheck= .github/workflows/perf-gate.yml .github/workflows/bench-compare.yml: cleanscripts/perf/ab.sh -r 2 -t 20ms -i -b './pkg/gosqlx=^BenchmarkParseE2E$/^join$' origin/main→env.txthascpu model: Apple M4 (arm64)andgcoff env: GOGC=off GOMEMLIMIT=64MiB, and the report opens with the Runner CPU line.scripts/perf/profile.sh -t 50ms -q join→ summary names the CPU and the GOGC=off env; cachegrind is reported unavailable (no valgrind on macOS).bench_matrix_test.gowas added to both trees.B/op of
diagnostics/1KBdrops 1.4% because the output buffer no longer grows. Time is not significant on this shared machine.CI (amd64): perf-gate shadow verdict on this PR
Current head
a9a2d60d(after the second review round and the merge of origin/maind29c6446): run 41339917373, EPYC 7763: PASS (shadow). Gated suite is now 32 benchmarks (includes #923's additions). Geomean -0.04% (p=0.631), A/A control 0 of 32 moved more than 3%, allocs/op identical for all 32, B/op unchanged, instructions/op within ±0.12% for every case. One benchmark faster (Normalize/complex-3.03%, A/A -2.39%: noise, this PR changes no library code). All checks green.Previous head:
Head
44249405(after the origin/main merge and review fixes): run 41337507444, merge commitda504728on base11f55dd7(#917), EPYC 9V74: PASS (shadow). No gated metric moved past its threshold. Geomean +0.13% (p=0.853), A/A control 2 of 26 benchmarks moved more than 3% (under the 10% noise limit;ParseRW/sqlserver/medium+4.66% vs A/A +4.83%,ParseE2E/complexA/A +3.36%), allocs/op identical for all 26, B/op unchanged, instructions/op geomean +0.03%. All other checks are green. The first Race Detector attempt timed out inpkg/transpiler(panic: test timed out after 2m0swhileTestTranspileDeepChainLinearwas just starting, on a slow runner wherepkg/sql/tokenizertook 112 s). The test takes 2.3 s locally under-raceand this PR changes no library code. One re-run of the failed job passed.Earlier runs: Run 41333597464 at merge commit 1b970a2, base
69a5a728, on an EPYC 9V74: PASS. No gated metric moved past its threshold. Geomean -0.19% (p=0.25), A/A control 0 of 26 benchmarks moved more than 3%, allocs/op unchanged everywhere, and the instructions/op table is clean. That is the expected result, because the PR changes no library code. The first commit's PR run, 41323785943 on an EPYC 9V45, was also PASS (geomean +0.10%, A/A 0 of 26). Both reports open with the new Runner CPU line and recordGOMEMLIMIT 64MiBin the settings.A/A dispatch runs (proof)
GOMEMLIMIT choice: gate mode A/A (base = head =
0c4f20d0, 10 rounds, 200 ms). The runners are different CPUs, so I compare the in-job ratio of each benchmark's GOGC=off time to its default-GC time. The first benchmark of each binary should look like the others if page faults no longer pollute it.ParseE2E/simple_selectgcoff/gcon (rest of ParseE2E)ParseRW/postgresql/mediumgcoff/gcon (rest of ParseRW)Full matrix: GOGC=off without a limit cannot run at all. Matrix-mode A/A run 41330996082 used
gomemlimit=off, 2 rounds of 50 ms. The runner was killed with "The runner has received a shutdown signal" (exit 143) 2.7 min into round 1, twice (attempt 1 and the rerun). The behaviour is consistent with memory exhaustion: the heap grows without bound across the large-input benchmarks. The same matrix with 64MiB completed twice: runs 41323701845 and 41330998379, 907 benchmarks, 693 s for 2 rounds of 50 ms, on EPYC 7763. The default 5 rounds of 100 ms should take about 45-60 min, inside the 240 min timeout.Matrix A/A finding: the matrix report says "allocs/op increased in 4 benchmarks" in an A/A. allocs/op is not exactly deterministic for large-input, low-N benchmarks:
ParseRW/*/large(e.g. 11154 vs 11156),Handler_LargeDocument, the*SustainedLoad*andTokenizerThroughputScalingbenchmarks. 52 of 907 benchmarks varied. Matrix mode only reports, so nothing fails. The gated suite's allocs/op were identical in every A/A and PR run above. I could not get a no-limit matrix baseline (see above), so I cannot tell how much of this comes from GC under the limit draining pools. Listed as an open question.Profile mode (run 41330929067 at
53149c18, EPYC 9V74). The first profile dispatch (41323699342) exposed a bug: alocalexpansion bug inprofile.shstopped every cachegrind run. The fix is in the second commit, with an end-to-end test that drivesprofile.shthrough a stub valgrind.Simulated caches, per operation (32 KiB 8-way L1i/L1d, 32 MiB LL):
cg-top-complex.txt:parser.gofunctions take 16.4% of all I1 misses (parsePrimaryExpression2.3%,parseFunctionCall2.0%).memclrNoHeapPointersat 2.8% (join) and 1.3% (complex), andheapSetTypeSmallHeaderis out of the top 40. C0's GOGC=off profile had 5.6% + 5.9% (join) and 5.0% + 3.2% (complex).Toolchain: every run above logs
go version go1.26.9 linux/amd64.Breaking Changes
None to the library. For the perf tooling:
bench:aband the competitor harness are not comparable with runs made before this change. Base and head of one run always share the setting.cmd/publish's internalenvForGOGC/shellCommandgained a memory-limit argument.configs[]gains an optionalgomemlimitfield (additive).🤖 Generated with Claude Code
https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2