feat: add benchmark test harness for comparing ts-loader build performance - #1706
Conversation
…mance Generates a synthetic project and times cold builds and incremental rebuilds (low- vs. high-fan-out file touches, under both transpileOnly modes) to compare one ts-loader checkout's speed against another. Wired into CI as a report-only PR check (Ubuntu + Windows) so perf-sensitive branches get an ongoing, automatic comparison against main rather than relying on ad hoc local measurement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Benchmark (Windows)
PR branch = |
Benchmark (Ubuntu)
PR branch = |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a benchmark test harness to compare ts-loader performance (cold builds + incremental rebuilds) between two checkouts and publishes a report-only comparison in CI for PRs.
Changes:
- Added a synthetic fixture generator and benchmark runner CLI to time cold builds and watch rebuilds under both
transpileOnlymodes. - Added a GitHub Actions workflow to run benchmarks on Ubuntu + Windows, upload artifacts, and post PR comments.
- Updated repo ergonomics to support the new harness (eslint extensions, yarn script, docs, gitignore, workflow action bumps).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/benchmark-tests/tsconfig.json | TS config for the benchmark test pack scripts/tooling. |
| test/benchmark-tests/scenarios.mts | Webpack config builder + cold/watch timing primitives + touch helper. |
| test/benchmark-tests/run-benchmark.mts | CLI orchestrator: generates fixtures, runs scenarios, summarizes + writes reports. |
| test/benchmark-tests/generate-fixture.mts | Deterministic synthetic TS project generator for benchmark scenarios. |
| test/benchmark-tests/README.md | Benchmark pack documentation and usage guidance. |
| package.json | Adds yarn benchmark script. |
| eslint.config.mjs | Expands linting file globs to include .mts/.cts. |
| AGENTS.md | Documents the new benchmark command and test pack purpose. |
| .gitignore | Ignores .benchmark/ output directory. |
| .github/workflows/windows-test-probe.yml | Bumps checkout action major version. |
| .github/workflows/release.yml | Bumps checkout/setup-node action major versions. |
| .github/workflows/push.yml | Bumps checkout/setup-node action major versions. |
| .github/workflows/benchmark.yml | New CI workflow to run, summarize, artifact, and PR-comment benchmark results. |
| .github/scripts/post-benchmark-comment.js | New helper to upsert a benchmark comment on PRs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function parseArgs(argv: string[]): Args { | ||
| const get = (flag: string, fallback: string): string => { | ||
| const i = argv.indexOf(flag); | ||
| return i === -1 ? fallback : argv[i + 1]; | ||
| }; | ||
| const rootA = path.resolve(get('--root-a', process.cwd())); | ||
| const rootB = path.resolve(get('--root-b', rootA)); | ||
| return { | ||
| rootA, | ||
| rootB, | ||
| labelA: get('--label-a', 'A'), | ||
| labelB: get('--label-b', 'B'), | ||
| fileCount: Number(get('--files', '300')), | ||
| warmup: Number(get('--warmup', String(WARMUP_ITERATIONS))), | ||
| iterations: Number(get('--iterations', String(MEASURED_ITERATIONS))), | ||
| benchmarkDir: path.resolve(get('--benchmark-dir', path.join(process.cwd(), '.benchmark'))), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fixed in commit 7e74c5fb. The get() helper now checks i === -1 || i + 1 >= argv.length || argv[i + 1].startsWith('--') before accessing argv[i + 1], returning the fallback in all those cases.
| function median(values: number[]): number { | ||
| const sorted = [...values].sort((x, y) => x - y); | ||
| const mid = Math.floor(sorted.length / 2); | ||
| return sorted.length % 2 ? sorted[mid] : (sorted[mid - 1] + sorted[mid]) / 2; | ||
| } | ||
|
|
||
| function stddev(values: number[]): number { | ||
| const mean = values.reduce((sum, v) => sum + v, 0) / values.length; | ||
| const variance = values.reduce((sum, v) => sum + (v - mean) ** 2, 0) / values.length; | ||
| return Math.sqrt(variance); | ||
| } |
There was a problem hiding this comment.
Fixed in commit 62cffe6c. Added validation in parseArgs that throws clear errors if --warmup is negative, --iterations is not positive, or warmup >= iterations. Also added explicit empty-array guards in median() and stddev() that throw rather than silently producing NaN.
| const sessionA = await withTimeout( | ||
| createWatchSession(configFor(args.rootA, fixtureMetaA, 'a')), | ||
| INITIAL_BUILD_TIMEOUT_MS, | ||
| `${id} initial build (a)` | ||
| ); | ||
| const sessionB = await withTimeout( | ||
| createWatchSession(configFor(args.rootB, fixtureMetaB, 'b')), | ||
| INITIAL_BUILD_TIMEOUT_MS, | ||
| `${id} initial build (b)` | ||
| ); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Fixed in commit b017825. Both sessionA and sessionB are now declared as let before the try/finally, and their creation is moved inside the try block. The finally uses optional chaining (?.close()) so whichever sessions were successfully created are always closed, even on partial failure.
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
|
|
||
| jobs: |
| - name: copy files | ||
| shell: pwsh | ||
| run: | | ||
| New-Item C:\source\ts-loader -ItemType Directory |
| const { data: comments } = await github.rest.issues.listComments({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| }); | ||
| const existing = comments.find((c) => c.body.includes(marker)); | ||
| if (existing) { | ||
| await github.rest.issues.updateComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| comment_id: existing.id, | ||
| body, | ||
| }); | ||
| } else { | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| body, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fixed by wrapping the listComments/updateComment/createComment calls in a try/catch. A 403 status (permission denied — typical for fork PRs) is caught and returns early, keeping the workflow report-only as intended. Any other error is re-thrown.
|
Thanks @acutmore! I want to get a sense of the difference in perf that 7.1 will make, but this is just generally useful I think |
…edian/stddev Co-authored-by: johnnyreilly <1010525+johnnyreilly@users.noreply.github.com>
Co-authored-by: johnnyreilly <1010525+johnnyreilly@users.noreply.github.com>
Co-authored-by: johnnyreilly <1010525+johnnyreilly@users.noreply.github.com>
|
@copilot here are some potential problems/fixes to implement: Root causes and fixes |
…processes
- Add run-side.mts: subprocess entry-point that runs all iterations for one
side and emits { durations: number[] } JSON to stdout. Each side now gets
its own OS process with a single TypeScript + webpack instance on the heap,
eliminating the GC pressure cross-contamination that caused systematic bias
in the in-process interleaved measurements.
- Update run-benchmark.mts: replace runColdScenario/runIncrementalScenario
with runSideProcess() (spawnSync) and runScenario() which alternates which
side runs first across scenarios. Bump MEASURED_ITERATIONS 6 → 10.
- Update scenarios.mts: change aggregateTimeout 100 → 0 so incremental
rebuild timings measure actual build cost, not debounce latency.
Co-authored-by: johnnyreilly <1010525+johnnyreilly@users.noreply.github.com>
Done. Changes applied in the latest commit:
|
|
@copilot cold build looks good - the variance is small. Incremental rebuild, both types, are quite different - please investigate and implement fix such that variance for each type of check are low. There are no changes on src/*.TS right now and so variance should be minimal. See recent results below
|
The root cause was that the subprocess approach ran all of side A's iterations sequentially before all of side B's — any host-level jitter (CPU throttling, disk cache warming, GC) between those two time windows appears as a false A-vs-B delta even when both checkouts are identical. Fix: incremental rebuild scenarios now run both sides' watch sessions in the same subprocess, interleaved A/B/A/B each iteration ( Cold builds keep the isolated subprocess approach ( |
|
@copilot the variance seems as wide as ever. Also the cold build variance which was small before the last commit is now large. See details below. Please revert changes to cold build and work on the incremental rebuild scenario. If it makes sense, separate cold build and incremental rebuild - they may be fundamentally different? Benchmark (Windows)
PR branch = |
Two changes in the latest commit: Cold build ( Incremental rebuild (
This means any host-level jitter (CPU scheduler noise, thermal throttling) affects both sides equally and cancels in the delta, rather than one side consistently absorbing it before the other runs. |
8fbb600 to
9a0c603
Compare
… all non-cold-typeCheck scenarios Cold typeCheck builds are the only scenario reliably large enough (800-1200ms) to absorb fixed per-iteration overhead. Cold transpileOnly builds and both incremental touch types are much smaller in absolute duration, so the same overhead (subprocess/V8 startup, fs-watch latency) is a much larger fraction of the measurement - confirmed by CI data showing cold transpileOnly swinging +11-16% on both Windows and Ubuntu. Also exclude the Windows working directories from Defender's real-time scanning, since this benchmark does a lot of file I/O (a 300-file fixture, output per build, a touched file per incremental iteration) that scanning adds unpredictable latency to.
…ounts 5 real CI runs (Windows + Ubuntu) showed the previous sequential, selectively-boosted approach still landed outside +-3% roughly half the time, including cold typeCheck (previously assumed reliable) and, strikingly, all three transpileOnly scenarios regardless of magnitude. Run both sides of every scenario concurrently instead of sequentially, so both experience the same wall-clock host conditions and that noise cancels out of the delta rather than biasing whichever side happens to run during a noisier window. Also boost every scenario's iteration count uniformly (previously only non-cold-typeCheck scenarios were boosted) - there's ample time budget within the job timeout even at 6x.
10 real CI runs with concurrent execution + uniform 6x boost showed cold builds now solid on both platforms (concurrency fixed them), but incremental rebuilds stayed noisy specifically on Windows (leaf/hub touch ~40-60% outside +-3% across 5 runs each) despite the same 6x boost. This points to Windows fs-watch imprecision rather than the fixed-overhead issue cold builds had. Each incremental rebuild is only tens of ms, so a much larger multiplier (20x vs 6x) is still cheap and should narrow the sampling error further.
…ypeCheck alone leaf_tc and hub_to stayed noisy across the last round's 10 CI runs despite the uniform 20x incremental boost, while hub_tc (already the most expensive incremental scenario at 400-600ms/iteration, doing a full ~180-file dependant recheck) was already reliable. Doubling hub_tc's iteration count too would cost meaningful CI time for no benefit, so only bump the three cheap incremental scenarios (leaf touch either mode, hub touch transpileOnly) to 40x.
…-touch typeCheck alone" This reverts commit 8bc965e.
…es noise
The three separate cold/leaf/hub loops (interleaved by mode) plus a
stabilizing sort() were added to stop a transient host-noise burst from
biasing an entire contiguous block of scenarios. That's no longer the
mechanism protecting against noise - each scenario's two sides now run
concurrently, so noise during any scenario is shared between both sides and
cancels out of that scenario's own delta regardless of execution order
relative to other scenarios. A single loop already produces results in the
desired display order, so the sort is no longer needed either.
Also: run-side.mts had a dynamic `await import('node:fs')` and a stale
"via spawnSync" doc comment left over from an earlier synchronous version -
both cleaned up.
Was describing the original sequential, same-process approach: wrong default iteration count (6, now 10), no mention of run-side.mts, subprocess isolation, concurrent execution, the per-scenario iteration multiplier, or the Windows Defender exclusion - all added during the reliability work.
- run-benchmark.mts: parseArgs's flag parser had no guard for a missing or
flag-shaped value, unlike run-side.mts's; now throws a clear error instead
of a raw TypeError or silently swallowing the next flag as a bogus value.
- run-benchmark.mts: --files had no integer validation unlike --warmup/
--iterations, so a bad value silently produced a near-empty fixture and
meaningless fast numbers instead of erroring.
- run-benchmark.mts: isRegressionFlagged divided by the B-side median with
no zero guard, risking a NaN/Infinity flag on a degenerate measurement.
- benchmark.yml: added a concurrency/cancel-in-progress group (push.yml
already has one) - without it, two runs racing on the same PR could both
find no existing marker comment and each create their own duplicate.
- post-benchmark-comment.js: listComments had no pagination, so the marker
comment could be missed on a PR with more than the default page of
comments, causing a fresh duplicate on every subsequent run; switched to
github.paginate.
- windows-test-probe.yml: was missing the Windows Defender exclusion that
benchmark.yml has for the same heavy file I/O pattern - genuine drift from
the same copy-pasted Windows workaround living in four places.
- generate-fixture.mts/run-benchmark.mts: FixtureMeta's four flat parallel
touch fields forced a template-literal-constructed key + type cast at each
call site; replaced with a nested `touch: { leaf, hub }` shape so callers
index by a real property with no cast needed.
Generates a synthetic project and times cold builds and incremental rebuilds (low- vs. high-fan-out file touches, under both transpileOnly modes) to compare one ts-loader checkout's speed against another. Wired into CI as a report-only PR check (Ubuntu + Windows) so perf-sensitive branches get an ongoing, automatic comparison against main rather than relying on ad hoc local measurement.