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