ReviewOS
Reload

also looking at this

craft-native/craft

fix(ci): make the benchmark gate measure something, and stop it failing at random

#75
Merged glennmichael123 wants to merge fix/benchmark-gate into main
5 files +263 -145
.github/workflows/benchmarks.ymlmodified+113-144
@@ -54,134 +54,131 @@ jobs:
5454 - name: First-party Zig dependencies
5555 uses: ./.github/actions/first-party-zig-deps
5656
57 - name: Build benchmark executable
57 - name: Setup Bun
58 uses: oven-sh/setup-bun@v2
59 with:
60 bun-version: latest
61
62 # ---------------------------------------------------------------------
63 # Binary load time, A/B against main on this runner
64 #
65 # What is measured is `craft --help`: process spawn, dynamic linking and
66 # argument parsing. It never opens a window, so it cannot see a change in
67 # window or webview startup. This step used to call that "Startup Time"
68 # and fail pull requests on it, which is how a number that cannot move
69 # ended up gating the code that would move it. Real startup is
70 # `benchmarks/startup.bench.ts`, and it needs a display.
71 #
72 # The comparison is A/B on one runner rather than against a stored
73 # number, because the stored number compared machines instead of
74 # binaries. Measured on one unchanging binary: p50 22.9ms, p95 38.8ms,
75 # max 65.6ms, and the old mean-of-5-against-a-cached-number method swung
76 # 22ms to 32ms — a 45% spread through a gate that fired at 20%.
77 # Interleaving both binaries in one sitting brought the same experiment
78 # to 3.5% worst case.
79 #
80 # So the cache holds main's *binary*, not main's timing.
81 # ---------------------------------------------------------------------
82 - name: Build binary
5883 working-directory: packages/zig
5984 run: |
6085 eval "$(pantry env | sed -n '/^export /,$p')" && zig build -Doptimize=ReleaseFast
86 mkdir -p "$RUNNER_TEMP/bench"
87 cp zig-out/bin/craft "$RUNNER_TEMP/bench/craft-head"
6188
62 - name: First-party Zig dependencies
63 uses: ./.github/actions/first-party-zig-deps
64
65 - name: Run benchmarks
66 id: bench
67 working-directory: packages/zig
68 run: |
69 mkdir -p benchmark-results
70
71 # Run system tray benchmark
72 if [ -f "zig-out/bin/craft" ]; then
73 echo "Running startup benchmark..."
74
75 # Measure startup time (average of 5 runs)
76 TOTAL=0
77 for i in {1..5}; do
78 START=$(date +%s%N)
79 timeout 2 ./zig-out/bin/craft --help >/dev/null
80 END=$(date +%s%N)
81 ELAPSED=$(( (END - START) / 1000000 ))
82 TOTAL=$((TOTAL + ELAPSED))
83 done
84 AVG_STARTUP=$((TOTAL / 5))
85 echo "startup_ms=$AVG_STARTUP" >> $GITHUB_OUTPUT
86
87 # Measure memory footprint
88 MEMORY=$(cat /proc/self/status | grep VmRSS | awk '{print $2}')
89 echo "memory_kb=${MEMORY:-0}" >> $GITHUB_OUTPUT
90 fi
91
92 # Run Zig benchmark tests
93 echo "Running benchmark tests..."
94 eval "$(pantry env | sed -n '/^export /,$p')" && zig build test 2>&1 | tee benchmark-results/test-output.txt
95
96 # Extract benchmark results if available
97 if grep -q "benchmark" benchmark-results/test-output.txt 2>/dev/null; then
98 grep "benchmark" benchmark-results/test-output.txt > benchmark-results/benchmarks.txt || true
99 fi
100
101 - name: Download baseline benchmarks
89 - name: Restore main's binary
10290 if: github.event_name == 'pull_request'
103 uses: actions/cache@v5
91 id: baseline
92 uses: actions/cache/restore@v4
10493 with:
105 path: baseline-benchmarks
106 key: benchmarks-baseline-${{ github.base_ref }}
94 path: baseline-binary
95 key: benchmarks-baseline-binary-${{ github.base_ref }}
10796 restore-keys: |
108 benchmarks-baseline-main
97 benchmarks-baseline-binary-main
10998
110 - name: Compare benchmarks
99 - name: Compare against main
111100 if: github.event_name == 'pull_request'
112101 id: compare
102 working-directory: benchmarks
113103 run: |
114 CURRENT_STARTUP=${{ steps.bench.outputs.startup_ms }}
115
116 if [ -f "baseline-benchmarks/startup_ms.txt" ]; then
117 BASELINE_STARTUP=$(cat baseline-benchmarks/startup_ms.txt)
118 DIFF=$((CURRENT_STARTUP - BASELINE_STARTUP))
119 PERCENT=$(echo "scale=2; ($DIFF * 100) / $BASELINE_STARTUP" | bc 2>/dev/null || echo "0")
120
121 echo "startup_diff=$DIFF" >> $GITHUB_OUTPUT
122 echo "startup_percent=$PERCENT" >> $GITHUB_OUTPUT
123 echo "baseline_startup=$BASELINE_STARTUP" >> $GITHUB_OUTPUT
124
125 # Fail if startup time regressed by more than 20%
126 THRESHOLD=$(echo "$BASELINE_STARTUP * 1.2" | bc | cut -d. -f1)
127 if [ "$CURRENT_STARTUP" -gt "$THRESHOLD" ]; then
128 echo "regression=true" >> $GITHUB_OUTPUT
129 echo "::warning::Startup time regressed by ${PERCENT}%"
130 else
131 echo "regression=false" >> $GITHUB_OUTPUT
132 fi
133 else
134 echo "No baseline available for comparison"
135 echo "regression=false" >> $GITHUB_OUTPUT
104 if [ ! -f "../baseline-binary/craft" ]; then
105 echo "No baseline binary cached yet — reporting without a comparison."
106 echo "compared=false" >> $GITHUB_OUTPUT
107 exit 0
136108 fi
137
138 - name: Generate benchmark report
109 echo "compared=true" >> $GITHUB_OUTPUT
110 chmod +x ../baseline-binary/craft
111
112 # Redirect, then cat — deliberately not `| tee`. `$?` after a
113 # pipeline is the last command's status, so `... | tee file` would
114 # report tee's success and the gate below could never fire. Whether
115 # `pipefail` happens to be set is not something a gate should depend
116 # on.
117 set +e
118 bun run binary-load-ab.ts \
119 --base ../baseline-binary/craft \
120 --head "$RUNNER_TEMP/bench/craft-head" \
121 --rounds 25 > ab-output.txt 2>&1
122 RC=$?
123 set -e
124 cat ab-output.txt
125
126 echo "regressed=$RC" >> $GITHUB_OUTPUT
127 DELTA=$(python3 -c "import json;print(json.load(open('binary-load-ab.json'))['delta_percent'])")
128 echo "delta=$DELTA" >> $GITHUB_OUTPUT
129
130 - name: Report
139131 run: |
140 echo "## Benchmark Results" >> $GITHUB_STEP_SUMMARY
132 echo "## Binary load time" >> $GITHUB_STEP_SUMMARY
141133 echo "" >> $GITHUB_STEP_SUMMARY
142 echo "### Startup Performance" >> $GITHUB_STEP_SUMMARY
143 echo "| Metric | Value |" >> $GITHUB_STEP_SUMMARY
144 echo "|--------|-------|" >> $GITHUB_STEP_SUMMARY
145 echo "| Startup Time | $ms |" >> $GITHUB_STEP_SUMMARY
146 echo "| Memory Usage | $KB |" >> $GITHUB_STEP_SUMMARY
147
148 if [ "$" == "pull_request" ] && [ -n "$" ]; then
134 if [ "$" = "true" ]; then
135 echo '```' >> $GITHUB_STEP_SUMMARY
136 cat benchmarks/ab-output.txt >> $GITHUB_STEP_SUMMARY
137 echo '```' >> $GITHUB_STEP_SUMMARY
149138 echo "" >> $GITHUB_STEP_SUMMARY
150 echo "### Comparison with main" >> $GITHUB_STEP_SUMMARY
151 echo "| Metric | Baseline | Current | Diff |" >> $GITHUB_STEP_SUMMARY
152 echo "|--------|----------|---------|------|" >> $GITHUB_STEP_SUMMARY
153 echo "| Startup | $ms | $ms | $ms ($%) |" >> $GITHUB_STEP_SUMMARY
154139 fi
140 echo "Measures \`craft --help\` — process spawn, dynamic linking and argument" >> $GITHUB_STEP_SUMMARY
141 echo "parsing. It does **not** measure window or webview startup." >> $GITHUB_STEP_SUMMARY
142
143 - name: Save main's binary as the baseline
144 if: github.ref == 'refs/heads/main'
145 run: |
146 mkdir -p baseline-binary
147 cp "$RUNNER_TEMP/bench/craft-head" baseline-binary/craft
148
149 - name: Cache main's binary
150 if: github.ref == 'refs/heads/main'
151 uses: actions/cache/save@v4
152 with:
153 path: baseline-binary
154 key: benchmarks-baseline-binary-main-${{ github.sha }}
155155
156156 - name: Comment on PR
157 if: github.event_name == 'pull_request'
157 if: github.event_name == 'pull_request' && steps.compare.outputs.compared == 'true'
158158 uses: actions/github-script@v7
159159 with:
160160 script: |
161 const startup = '$';
162 const memory = '$';
163 const diff = '$' || '0';
164 const percent = '${{ steps.compare.outputs.startup_percent }}' || '0';
165 const regression = '$' === 'true';
166
167 let emoji = regression ? '🔴' : '✅';
168 let status = regression ? 'Performance regression detected!' : 'Performance looks good';
169
170 const body = `## ${emoji} Benchmark Results
171
172 ${status}
161 const fs = require('fs');
162 const out = fs.readFileSync('benchmarks/ab-output.txt', 'utf8');
163 const regressed = '$' === '1';
164 const body = `## ${regressed ? '\u{1F534}' : '\u2705'} Binary load time
173165
174 | Metric | Value |
175 |--------|-------|
176 | Startup Time | ${startup}ms |
177 | Memory Usage | ${memory}KB |
178 | Change | ${diff}ms (${percent}%) |
166 \`\`\`
167 ${out.trim()}
168 \`\`\`
179169
180170 <details>
181 <summary>Benchmark Details</summary>
171 <summary>What this measures</summary>
182172
183 - Startup time measured as average of 5 runs
184 - Regression threshold: 20% slower than baseline
173 \`craft --help\`: process spawn, dynamic linking and argument parsing.
174 It never opens a window, so it **cannot** see a change in window or
175 webview startup — real startup is \`benchmarks/startup.bench.ts\`, which
176 needs a display.
177
178 Both binaries are measured interleaved on this runner and compared by
179 p50, rather than against a number recorded on another machine. On
180 byte-identical binaries that method reads within ~3.5%; the old one
181 swung 45%.
185182 </details>`;
186183
187184 const { data: comments } = await github.rest.issues.listComments({
@@ -189,62 +186,34 @@ jobs:
189186 repo: context.repo.repo,
190187 issue_number: context.issue.number,
191188 });
192
193 const existingComment = comments.find(c =>
194 c.user.login === 'github-actions[bot]' &&
195 c.body.includes('Benchmark Results')
189 const existing = comments.find(c =>
190 c.user.login === 'github-actions[bot]' && c.body.includes('Binary load time')
196191 );
197
198 if (existingComment) {
192 if (existing) {
199193 await github.rest.issues.updateComment({
200 owner: context.repo.owner,
201 repo: context.repo.repo,
202 comment_id: existingComment.id,
203 body: body
194 owner: context.repo.owner, repo: context.repo.repo,
195 comment_id: existing.id, body,
204196 });
205197 } else {
206198 await github.rest.issues.createComment({
207 owner: context.repo.owner,
208 repo: context.repo.repo,
209 issue_number: context.issue.number,
210 body: body
199 owner: context.repo.owner, repo: context.repo.repo,
200 issue_number: context.issue.number, body,
211201 });
212202 }
213203
214 - name: Save benchmark baseline
215 if: github.ref == 'refs/heads/main'
216 run: |
217 mkdir -p baseline-benchmarks
218 echo "$" > baseline-benchmarks/startup_ms.txt
219 echo "$" > baseline-benchmarks/memory_kb.txt
220
221 - name: Cache benchmark baseline
222 if: github.ref == 'refs/heads/main'
223 uses: actions/cache/save@v4
224 with:
225 path: baseline-benchmarks
226 key: benchmarks-baseline-main-${{ github.sha }}
227
228 - name: Save benchmark history
229 if: github.ref == 'refs/heads/main'
230 run: |
231 mkdir -p .github/metrics
232 DATE=$(date -u +%Y-%m-%d)
233 COMMIT=$(git rev-parse --short HEAD)
234
235 echo "{\"date\": \"$DATE\", \"commit\": \"$COMMIT\", \"startup_ms\": $, \"memory_kb\": $}" >> .github/metrics/benchmarks.jsonl
236
237204 - name: Upload benchmark results
205 if: always()
238206 uses: actions/upload-artifact@v4
239207 with:
240208 name: benchmark-results
241209 path: |
242 packages/zig/benchmark-results/
243 .github/metrics/
210 benchmarks/ab-output.txt
211 benchmarks/binary-load-ab.json
212 if-no-files-found: ignore
244213 retention-days: 90
245214
246215 - name: Fail on regression
247 if: steps.compare.outputs.regression == 'true'
216 if: steps.compare.outputs.regressed == '1'
248217 run: |
249 echo "::error::Performance regression detected. Startup time increased by $%"
218 echo "::error::Binary load time regressed by $% against main, measured A/B on this runner."
250219 exit 1
benchmarks/.gitignoreadded+2-0
@@ -0,0 +1,2 @@
1binary-load-ab.json
2ab-output.txt
benchmarks/README.mdmodified+18-0
@@ -80,6 +80,7 @@ Craft's minimal envelope is **1.4x** faster than Electrobun, **1.5x** faster tha
8080| **IPC Overhead** | `ipc.bench.ts` | JSON serialization with each framework's message format | mitata micro-benchmark |
8181| **Memory** | `memory.bench.ts` | RSS of entire process tree after 3s stabilization | `ps -o rss=` across all child PIDs |
8282| **Startup** | `startup.bench.ts` | Cold-start time (spawn -> ready -> exit) | `performance.now()` across 10 iterations |
83| **Binary load** | `binary-load-ab.ts` | Process spawn, dynamic linking and argument parsing — **not** window startup | p50 of interleaved A/B runs of two binaries on one machine |
8384
8485## Run Individual Benchmarks
8586
@@ -88,8 +89,25 @@ bun run bench:size # Bundle/binary size comparison
8889bun run bench:ipc # IPC protocol micro-benchmarks
8990bun run bench:memory # Process memory (RSS)
9091bun run bench:startup # Startup time
92bun run bench:load --base <binary> --head <binary> # A/B binary load time
9193```
9294
95## What CI gates on
96
97`binary-load-ab.ts`, and only that one. It needs no display, so it is the only
98benchmark here that can run on a CI runner honestly.
99
100It compares two binaries **interleaved on the same runner**, rather than a
101measurement against a number recorded elsewhere. That matters more than it
102sounds: on one unchanging binary this measurement reads p50 22.9ms, p95
10338.8ms, max 65.6ms, and comparing a mean-of-5 against a stored baseline swung
10422ms to 32ms — a 45% spread, through a gate that fired at 20%. Interleaving
105brought the same experiment to 3.5% worst case.
106
107`startup.bench.ts` measures the thing people actually mean by startup — window,
108webview, first paint — and needs a display, so it is run by hand rather than
109gated.
110
93111## Hello World Apps
94112
95113Each framework runs the same minimal Hello World content:
benchmarks/binary-load-ab.tsadded+128-0
@@ -0,0 +1,128 @@
1#!/usr/bin/env bun
2
3/**
4 * A/B binary load time, measured on one machine in one sitting.
5 *
6 * ## What this measures, and what it does not
7 *
8 * `craft --help` parses its arguments, prints, and exits. It never opens a
9 * window, never touches AppKit or GTK, and never starts a WebContent process.
10 * So this measures **binary load time** — process spawn, dynamic linking,
11 * relocation, and argument parsing — and nothing about how long an app takes
12 * to appear on screen.
13 *
14 * That distinction is the reason this file exists. The benchmark workflow
15 * called the same measurement "Startup Time" and failed pull requests on it,
16 * which meant a number that cannot move when window creation gets slower was
17 * gating changes to window creation. Naming it for what it is stops the next
18 * person trusting it for something it cannot see. Real startup — window,
19 * webview, first paint — is `startup.bench.ts`, and it needs a display.
20 *
21 * ## Why A/B rather than a stored number
22 *
23 * Load time on a shared CI runner is noisy and heavy-tailed. Measured on one
24 * unchanging binary on an idle laptop: p50 22.9ms, p95 38.8ms, max 65.6ms.
25 * Comparing today's measurement against a number recorded on another machine
26 * on another day compares the machines, not the binaries — on identical
27 * binaries that method spread 22ms to 32ms, a 45% swing, against a gate that
28 * fired at 20%.
29 *
30 * Running both binaries interleaved on the same machine cancels almost all of
31 * it, because whatever slows one round slows both halves of it. Same
32 * experiment, same laptop, byte-identical binaries: worst case 3.5%.
33 *
34 * Interleaving is what does the work. Measuring all of A and then all of B
35 * would leave a drift between the halves attributed to the change.
36 */
37
38interface Options {
39 base: string
40 head: string
41 rounds: number
42 threshold: number
43}
44
45function parseArgs(argv: string[]): Options {
46 const get = (flag: string): string | undefined => {
47 const i = argv.indexOf(flag)
48 return i === -1 ? undefined : argv[i + 1]
49 }
50 const base = get('--base')
51 const head = get('--head')
52 if (!base || !head) {
53 console.error('usage: bun run binary-load-ab.ts --base <binary> --head <binary> [--rounds N] [--threshold 1.2]')
54 process.exit(2)
55 }
56 return {
57 base,
58 head,
59 rounds: Number(get('--rounds') ?? 25),
60 threshold: Number(get('--threshold') ?? 1.2),
61 }
62}
63
64function runOnce(binary: string): number {
65 const start = Bun.nanoseconds()
66 Bun.spawnSync({ cmd: [binary, '--help'], stdout: 'ignore', stderr: 'ignore' })
67 return (Bun.nanoseconds() - start) / 1e6
68}
69
70function quantile(sorted: number[], q: number): number {
71 return sorted[Math.min(sorted.length - 1, Math.floor(sorted.length * q))]
72}
73
74interface Stats { p50: number, p95: number, min: number, max: number }
75
76function stats(times: number[]): Stats {
77 const s = [...times].sort((a, b) => a - b)
78 return { p50: quantile(s, 0.5), p95: quantile(s, 0.95), min: s[0], max: s[s.length - 1] }
79}
80
81const opts = parseArgs(Bun.argv)
82
83// The first launch of each binary pays for a cold page cache and is not
84// representative of anything. Warm both, then start measuring.
85runOnce(opts.base)
86runOnce(opts.head)
87
88const baseTimes: number[] = []
89const headTimes: number[] = []
90for (let i = 0; i < opts.rounds; i++) {
91 // Alternate which goes first so neither consistently occupies the same
92 // position in a round — an ordering effect would otherwise land entirely on
93 // one side.
94 if (i % 2 === 0) {
95 baseTimes.push(runOnce(opts.base))
96 headTimes.push(runOnce(opts.head))
97 }
98 else {
99 headTimes.push(runOnce(opts.head))
100 baseTimes.push(runOnce(opts.base))
101 }
102}
103
104const base = stats(baseTimes)
105const head = stats(headTimes)
106const ratio = head.p50 / base.p50
107const percent = (ratio - 1) * 100
108const regressed = ratio > opts.threshold
109
110const fmt = (n: number) => n.toFixed(1)
111console.log(`rounds: ${opts.rounds} interleaved`)
112console.log(`base: p50 ${fmt(base.p50)}ms p95 ${fmt(base.p95)}ms (${fmt(base.min)}–${fmt(base.max)}ms)`)
113console.log(`head: p50 ${fmt(head.p50)}ms p95 ${fmt(head.p95)}ms (${fmt(head.min)}–${fmt(head.max)}ms)`)
114console.log(`delta: ${percent >= 0 ? '+' : ''}${fmt(percent)}% (fails above +${fmt((opts.threshold - 1) * 100)}%)`)
115
116const summary = {
117 base_p50_ms: Number(base.p50.toFixed(2)),
118 head_p50_ms: Number(head.p50.toFixed(2)),
119 delta_percent: Number(percent.toFixed(2)),
120 regressed,
121}
122await Bun.write('binary-load-ab.json', `${JSON.stringify(summary, null, 2)}\n`)
123
124if (regressed) {
125 console.error(`\nBinary load time regressed by ${fmt(percent)}%.`)
126 process.exit(1)
127}
128console.log('\nNo binary load time regression.')
benchmarks/package.jsonmodified+2-1
@@ -9,7 +9,8 @@
99 "bench:startup": "bun run startup.bench.ts",
1010 "bench:size": "bun run size.bench.ts",
1111 "bench:ipc": "bun run ipc.bench.ts",
12 "bench:memory": "bun run memory.bench.ts"
12 "bench:memory": "bun run memory.bench.ts",
13 "bench:load": "bun run binary-load-ab.ts"
1314 },
1415 "dependencies": {
1516 "mitata": "^1.0.10"