refactor(benchmark): move measurement and reporting from shell to Go - #435
Merged
Conversation
The benchmark harness had drifted into doing in shell the two things shell is worst at. It parsed JSON with a regex to find snapshot refs, forked bc to divide two numbers, hand-assembled Markdown tables with printf, and carried three bash-version landmines — mapfile and negative array indices are bash 4+ while macOS ships 3.2, and printf '---:|' is read as flags. None of it was tested. Orchestration stays in shell, which is what it is for: the scripts still build datasets and launch cloudstic, restic, borg and duplicacy. Everything after that is internal/benchreport: - `run` measures a child process. Peak RSS comes from the wait4 rusage the kernel already returns, replacing /usr/bin/time — whose flag, label and unit all differ between BSD and GNU, and which is absent on a minimal Linux image. The unit difference survives as a two-line switch behind a build tag. - `row` takes numbers a harness measured itself. run.sh needs this: it compares four tools, and one that is not installed must leave a gap rather than abort the comparison, so it keeps its own timing and error handling. - `render` produces the table and charts. One CSV schema serves both harnesses, since they differ only in which column varies — the memory sweep holds the tool constant and varies size, the competitive run does the reverse. Which chart to draw is read off the data rather than passed in. run.sh now emits that CSV alongside the table it already printed, so the competitive run gets the same summary treatment as the memory sweep: bar charts with tools on the x-axis. Its own output is unchanged. The rendering logic is now unit-tested — column order, missing cells rendering as a dash rather than a zero, malformed numbers erroring rather than silently becoming zero, the table preceding the charts, and mermaid quoting rules. render-memory.sh is deleted.
This was referenced Aug 4, 2026
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
CI was running `run.sh local local cloudstic` — a 615-line cross-tool harness filtered down to one tool, on a runner where restic, borg and duplicacy are not installed. The comparison never happened there. The dead paths were the smaller problem. The real one is that the two jobs have conflicting requirements: a comparison needs a dataset that is fair across four tools and can only measure what all of them expose, while tracking this product over time needs a dataset that stresses this design and is free to report numbers no other tool has. Sharing one script meant a change made for cross-tool fairness would silently move the numbers CI trends on. - run.sh becomes compare.sh, which is only ever run by hand. - cloudstic.sh is new and is what the Benchmark workflow runs. It measures what a comparison cannot: incremental cost at one changed file and at a thousand, deduplication of already-stored data, and the stored-to-logical ratio. - lib.sh holds the measurement mechanics both use. The datasets deliberately stay separate — sharing them would reintroduce exactly the coupling this split removes. The report grows a Repo added column for a single-tool run, which is where deduplication shows up: copying 340 MB of already-stored data adds 40 KB. That column is withheld across tools, where it would invite comparing byte counts between different repository formats. Trigger is unchanged: manual dispatch. Worth noting the workflow has fired once since April, so the split is only worth what someone runs.
…memory The Benchmark workflow was dispatch-only, and had fired once since April. It now also runs on a pull request carrying the `benchmark` label, matching what Memory scaling already does. Deliberately the same label rather than a second one: labelling a PR is the act of asking for performance data, and wanting the memory curve but not the throughput numbers is not a real case. `synchronize` is included so pushing to an already-labelled PR re-measures without another click, and the job is skipped for any other label so an unrelated one does not start it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Answers the "is bash idiomatic here?" question by drawing the line where it actually falls: shell orchestrates, Go analyses.
Orchestration stays in shell, because that is what it is for — the scripts still build datasets and launch cloudstic, restic, borg and duplicacy, and you cannot benchmark another binary from
testing.B. Everything after that moves tointernal/benchreport.What was wrong with the shell version
Not style — concrete defects, all from the last two sessions:
grep -o 'snapshot/[a-f0-9]\{64\}'bc(echo "scale=1; $kb/1024" | bc)mapfileand negative array indices are bash 4+ while macOS ships 3.2;printf '---:|'is parsed as flags/usr/bin/timeparsing branches, because the flag, label and unit all differ between BSD and GNUWhat replaces it
runrowrenderrundeletes the/usr/bin/timedependency outright. The kernel hands max RSS to the parent throughwait4; taking it fromcmd.ProcessState.SysUsage()removes the fork, both parsing branches and the dependency. The unit difference survives as a two-line switch behind a//go:build unixtag, with a stub elsewhere — verified withGOOS=windows go build.rowexists becauserun.shgenuinely needs it: it compares four tools, and one that is not installed has to leave a gap rather than abort the comparison. It keeps its own timing and error handling and hands the numbers over, which is what stops the two harnesses' CSV schemas from drifting apart.One schema, two shapes
The harnesses differ only in which column varies — the memory sweep holds the tool constant and varies size; the competitive run does the reverse.
Kind()reads that off the data rather than being told, and picks the chart accordingly: lines over file count for scaling, bars over tool names for comparison.run.shgets the summary treatmentIt now emits that CSV alongside the table it already printed. Its own output is unchanged — the change is purely additive. Verified with a real
local local cloudsticrun:With several tools it produces one bar chart per operation. A single-tool run emits no charts at all and suppresses the heading, since one bar compares nothing.
Tests
The rendering logic was previously awk and bc inside a shell script, where it could not be tested. Now covered: column order following the run, missing cells rendering as a dash rather than a zero (a zero in a performance table reads as a real measurement), malformed numbers erroring rather than silently becoming zero, the table always preceding the charts, and mermaid's quoting rules — tool names quoted, numeric axes not, since quoting a numeric axis makes mermaid space the points evenly regardless of value.
Verification
./scripts/benchmark/run.sh local local cloudstic— full 1 GB dataset run, table unchanged, CSV emittedSIZES="500 2000" ./scripts/benchmark/memory.sh— Go measurement agrees with the old/usr/bin/timepath to within noise (101.9 vs 101.6 MB)/bin/bash -non both scripts under bash 3.2.57go test -race ./...passes;golangci-lint run ./...— 0 issuesGOOS=windows go build ./internal/benchreportcompilesnpx markdownlint-cli2 '**/*.md'— 0 issuesNot included
The other benchmarking gaps we discussed — incremental cost vs change ratio, API request counts, snapshot-count scaling — are untouched here. This PR is the plumbing they would all build on.