Skip to content

Commit e9b398b

Browse files
zkochanclaude
andauthored
perf: measure the samples in parallel jobs (#60)
* perf: measure the samples in parallel jobs and publish from one that measures nothing The workflow ran `pnpm run benchmark` three times in a row in a single job: 8m05s, 7m57s, 8m05s, for 24m20s of a run whose measurements take eight minutes. The three are independent — each makes its own temp directory, provisions its own package managers, and starts its own registry on a port the system says is free, and the only thing a later one reads from an earlier is the sample count that `LIMIT_RUNS` caps, which three samples never reach. So they now run as a matrix. Three samples rather than one because the page reports the minimum per package manager *per version*. Cutting to a single sample wouldn't only add noise, it would bias: a version released this week would be a min-of-one while a long-lived one keeps its min-of-many, which reads as a regression in whatever was released most recently. Splitting the measuring means something has to join it back together, so `index.js` grows a `--report-only` mode. `benchmarkFixtures` and `nodeVersionsSection` now take a callback for one manager's results — measuring measures, reporting reads back what a measuring run recorded — and everything downstream of that only draws, so there is one copy of it. A reporting run installs no package manager and starts no registry, which is what keeps it at seconds rather than minutes, and it means the job that commits results can't fail for a benchmarking reason. It also can't invent a number a measuring run failed to record: it fails instead. What it can't work out for itself is which version of each tool the results were measured with, since nothing is installed to ask — hence the `versions.json` a measuring run writes and its job uploads. The Node.js version goes in it too: the charts say "Tests were run using Node.js X", and that is the run that measured them, not the one drawing the page. `mergeResults.js` folds the runs together. Each starts from the same commit and appends to the results it checked out, so what a run contributes is whatever it added past the length of the baseline — its whole file still carries every sample recorded before this week and would otherwise be counted once per job. Verified by rebuilding the published page from the committed results alone: identical tables and numbers, differing only in the timestamp. And by merging three simulated runs against the real results tree: three samples added, one per run, a version no baseline had getting exactly three, and only the file that actually changed showing up in `git status`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: fail a reporting run on a manifest it can't trust, and keep the measuring jobs read-only Two review findings on the parallel-jobs change. The workflow used to be one job, and that job declared its own permissions. Splitting it left the measuring jobs inheriting whatever the repository defaults to, so the default is now read-only and the one job that publishes raises it for itself. The reporting run caught a manifest it couldn't read or parse, but not one that parsed into the wrong shape. A missing package manager would surface downstream — the results aren't where its version says they are — but a missing pnpr version surfaced nowhere: the page went out saying the registry everything was measured against was `vundefined`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: demand a version for the Node.js section's tools too The manifest guard checked every package manager column but only that the `nodeManagers` object existed, so an empty or partial one got as far as `readRecordedResults`. That did name the tool it couldn't place, but by then the fixture section had already been built, and nothing said the manifest was what was wrong. Checked over the keys of `nodeManagersMap`, which is what `writeVersionsManifest` writes the section from, so what a reporting run demands is exactly what a measuring run records. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f089d57 commit e9b398b

9 files changed

Lines changed: 426 additions & 70 deletions

File tree

.github/workflows/benchmark.yml

Lines changed: 65 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,33 @@ on:
55
- cron: '30 2 * * 0'
66
workflow_dispatch:
77

8+
# Read-only by default, so the jobs that only measure cannot write to the
9+
# repository whatever the repository's own default happens to be. The one job
10+
# that publishes raises this for itself.
11+
permissions:
12+
contents: read
13+
814
jobs:
9-
build:
15+
# The samples this run contributes, measured in parallel rather than one
16+
# after another. Three measurements of the same thing are what the published
17+
# number is a `min()` of, and they are independent of each other: each makes
18+
# its own temp directory, provisions its own package managers, and starts its
19+
# own registry on a port the system says is free. Run sequentially in one job
20+
# they took three times as long as a single measurement for no benefit.
21+
#
22+
# Three samples rather than one because the page reports the minimum per
23+
# package manager *per version*. Cutting to one sample wouldn't only add
24+
# noise, it would bias: a version released this week would be a min-of-one
25+
# while a long-lived one keeps its min-of-many, which reads as a regression
26+
# in whatever was released most recently.
27+
benchmark:
28+
strategy:
29+
# One machine having a bad day shouldn't throw away the samples the
30+
# others already measured. The reporting job publishes whatever arrived.
31+
fail-fast: false
32+
matrix:
33+
sample: [1, 2, 3]
1034
runs-on: ubuntu-latest
11-
# The job commits the results it measures back to the branch, which the
12-
# default read-only token can't do.
13-
permissions:
14-
contents: write
1535
steps:
1636
- uses: actions/checkout@v4
1737
with:
@@ -37,12 +57,46 @@ jobs:
3757
- run: pnpm run benchmark
3858
env:
3959
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
40-
- run: pnpm run benchmark
41-
env:
42-
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
43-
- run: pnpm run benchmark
44-
env:
45-
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
60+
# Only uploaded when the measurement finished. A run that died partway
61+
# recorded some scenarios and not others, and those are exactly the
62+
# numbers not to publish.
63+
- uses: actions/upload-artifact@v4
64+
with:
65+
name: samples-${{ matrix.sample }}
66+
path: |
67+
results
68+
versions.json
69+
retention-days: 3
70+
71+
# Draws the page from the samples the jobs above recorded. It measures
72+
# nothing itself — no package manager is installed here and no registry is
73+
# started — so it can't quietly substitute a number for one a measuring run
74+
# failed to record, and it costs seconds rather than minutes.
75+
report:
76+
needs: benchmark
77+
# `needs` alone would skip this job if any one sample failed, throwing away
78+
# the two that succeeded.
79+
if: ${{ !cancelled() }}
80+
runs-on: ubuntu-latest
81+
# The job commits the results it publishes back to the branch, which the
82+
# default read-only token can't do.
83+
permissions:
84+
contents: write
85+
steps:
86+
- uses: actions/checkout@v4
87+
with:
88+
persist-credentials: false
89+
fetch-depth: 0
90+
- uses: pnpm/setup@v2
91+
# Fails when no sample arrived at all, which is the right outcome: with
92+
# nothing measured this week, the alternative is republishing last week's
93+
# numbers under today's date.
94+
- uses: actions/download-artifact@v4
95+
with:
96+
pattern: samples-*
97+
path: samples
98+
- run: pnpm run merge-results samples
99+
- run: pnpm run report
46100
- name: Commit & Push changes
47101
# Pinned: this step is handed a token that can write to the repo, so
48102
# what runs in it shouldn't be able to change without a commit here.

.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,5 @@ node_modules
55
.tmp
66

77
managers
8+
9+
versions.json

generateStackedSvg.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@
44

55
const getMax = (results) => Math.max(...results.map((r) => r.v11))
66

7-
export default (results, formattedNow) => {
7+
// `nodeVersion`: see the note in `generateSvg.js`.
8+
export default (results, formattedNow, nodeVersion = process.version) => {
89
const v12Color = '#fbae00'
910
const extraColor = '#cccccc'
1011

@@ -102,7 +103,7 @@ export default (results, formattedNow) => {
102103
})
103104
})
104105

105-
svgStr += ` <text x="${graph.x + graph.w}" y="${vb.h - 2}" class="font s4 text" text-anchor="end">Tests were run using Node.js ${process.version} at: ${formattedNow}</text>\n`
106+
svgStr += ` <text x="${graph.x + graph.w}" y="${vb.h - 2}" class="font s4 text" text-anchor="end">Tests were run using Node.js ${nodeVersion} at: ${formattedNow}</text>\n`
106107

107108
svgStr += '</svg>\n'
108109
return svgStr

generateSvg.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@ const getHighestNumber = (resultArrays) => {
1717
return max
1818
}
1919

20-
export default (resultArrays, pms, tests, formattedNow) => {
20+
// `nodeVersion` is the Node.js the results were measured on. A reporting run
21+
// draws a page it did not measure, so it can't use its own `process.version`.
22+
export default (resultArrays, pms, tests, formattedNow, nodeVersion = process.version) => {
2123
let svgStr = ''
2224
// Expand stacked PM slots into two legend entries (primary + extra).
2325
const legendEntries = pms.flatMap((pm) => pm.stacked
@@ -217,7 +219,7 @@ export default (resultArrays, pms, tests, formattedNow) => {
217219

218220
// add node version
219221
;(() => {
220-
const text = `Tests were run using Node.js ${process.version} at: ${formattedNow}`
222+
const text = `Tests were run using Node.js ${nodeVersion} at: ${formattedNow}`
221223
const anchor = 'end'
222224
const x = graph.x + graph.w
223225
const y = vb.h - 2

0 commit comments

Comments
 (0)