Make the benchmarks able to resolve real effects - #15
Merged
Conversation
The focused cache restore benchmark could not distinguish a real improvement from runner noise, so its headline numbers were not evidence of anything. Two consecutive runs against an unchanged actions/setup-java@v4.8.0, where the true difference is exactly zero, reported medians of 2s and 3s. That spurious 1.2s separation has a bootstrap 95% CI of [-1.97, -0.43], which excludes zero. Across the same two runs the reported candidate delta flipped from +0.6s to -0.8s. Three defects caused this: - Durations were read from the Actions API, whose step timestamps have one-second resolution. Setup takes two to six seconds, so every sample carried +/-500ms of quantization error, the same magnitude as the effects being measured. Every recorded sample was an integer. - Each arm ran as its own matrix of independent jobs, so the comparison was confounded with between-runner variance, which on hosted runners is larger than the effect. Adding samples to each arm does not remove it. - The report paired "sample N" of one arm with "sample N" of the other. Those jobs shared no runner and no point in time, so the pairing removed no variance while the label implied precision the data did not have. Results were published as bare point estimates with no interval. This change rebuilds the workflow around measurements that can support a verdict: - scripts/measure.mjs times the setup step from inside the job at millisecond resolution instead of reading the API clock. - Both arms now run in the same job in ABBA order, so each runner produces one genuinely paired difference with its own speed cancelled out, and the mirrored order cancels drift across slots. Each slot deletes ~/.m2 first so every restore extracts into an empty tree. This also uses fewer jobs than the design it replaces. - scripts/stats.mjs adds seeded bootstrap intervals, a paired permutation test, and a Hodges-Lehmann shift estimate, and converts them into an explicit verdict. Comparisons whose interval includes zero are reported as inconclusive rather than as a number. - The report publishes a noise floor derived from the within-runner repeat spread, and an A/A control that applies the same estimator to the baseline against itself. Because each arm is already measured twice per runner, the control costs no extra jobs. A run whose control resolves a difference is flagged as untrustworthy. - The workflow takes setup-java-repository, baseline-ref, and candidate-ref, so a PR branch can be measured directly. Validated against synthetic data with a known effect injected under realistic between-runner spread: a true 0ms effect reports inconclusive, and a true 400ms effect is recovered as -361ms with a 95% CI of [-420, -299] and p = 0.002. The previous harness could not resolve anything below one second and returned false positives at zero. The real A/A dataset is pinned in scripts/stats.test.mjs so unpaired sampling is not reintroduced. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
Contributor
There was a problem hiding this comment.
Pull request overview
This PR overhauls the Focused cache restore benchmark so it can reliably detect real performance effects (vs runner noise) by switching to in-job millisecond timing, same-runner ABBA pairing, and statistical reporting with intervals and explicit verdicts.
Changes:
- Replace Actions API step-timestamp timing with an in-job stopwatch (
scripts/measure.mjs) and per-runner ABBA sampling in the workflow. - Introduce shared statistical helpers (
scripts/stats.mjs) with bootstrap intervals, permutation testing, and verdict classification, plus comprehensive tests. - Rewrite the focused report generator to consume uploaded CSV timings, compute paired analysis + guard rails (noise floor, A/A control), and update docs/tests accordingly.
Show a summary per file
| File | Description |
|---|---|
| scripts/stats.mjs | Adds statistical primitives (bootstrap/paired intervals, permutation test, HL shift, verdict formatting). |
| scripts/stats.test.mjs | Adds tests for determinism, intervals, classification, and a pinned regression dataset for unpaired false positives. |
| scripts/report-focused.mjs | Reworks focused report to read uploaded timing CSVs, build paired samples, analyze, and render a verdicted report. |
| scripts/report-focused.test.mjs | Adds tests for parsing/pairing, noise floor, verdict behavior, and report rendering. |
| scripts/measure.mjs | Adds millisecond stopwatch + CSV recorder for benchmark slots inside the job. |
| scripts/focused-cache-restore.sh | Adds helpers for cache identity, fixture seeding, reset, and verification across ABBA slots. |
| .github/workflows/focused-cache-restore.yml | Runs both arms in one job in ABBA order, records timings, uploads artifacts, and generates the new report. |
| scripts/report.test.mjs | Removes focused-job-name parsing tests that no longer apply. |
| README.md | Updates benchmark methodology documentation and explicitly calls out limitations of other workflows still using API timing/unpaired arms. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+186
to
+190
| export function formatInterval(interval, {digits = 1, unit = 's'} = {}) { | ||
| if (!interval) return 'n/a'; | ||
| const {estimate, low, high} = interval; | ||
| return `${estimate.toFixed(digits)}${unit} (95% CI ${low.toFixed(digits)} to ${high.toFixed(digits)})`; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed. formatInterval now reads the level off the interval itself (confidence, defaulting to 0.95 for intervals built before that field was carried), so widening an interval can no longer publish it under a 95% label. Covered by a test in scripts/stats.test.mjs.
Comment on lines
241
to
250
| export async function main(env = process.env) { | ||
| const [owner, repo] = env.GITHUB_REPOSITORY.split('/'); | ||
| const token = env.GH_TOKEN; | ||
| const runId = env.GITHUB_RUN_ID; | ||
| const attempt = env.GITHUB_RUN_ATTEMPT; | ||
| const samples = Number(env.SAMPLES); | ||
| const javaVersion = env.JAVA_VERSION; | ||
| if (!owner || !repo || !token || !runId || !attempt || !samples) { | ||
| const baselineRef = env.BASELINE_REF; | ||
| const candidateRef = env.CANDIDATE_REF; | ||
| const setupJavaRepository = env.SETUP_JAVA_REPOSITORY; | ||
| if (!owner || !repo || !token || !runId || !baselineRef || !candidateRef) { | ||
| throw new Error('Missing required GitHub Actions environment variables'); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Agreed, and this applied to the other three reports too. Added a shared requireEnv helper in paired.mjs and pointed all four reports at it, each declaring every variable it renders — including SETUP_JAVA_REPOSITORY and JAVA_VERSION here. It names the missing variables rather than reporting a generic failure, and the owner/repo split now has its own message. I verified all four workflows actually set what they now require.
The first live run missed the seeded wrapper cache on every runner, so all six measure jobs failed with the wrapper fixture absent. setup-java keys its separate wrapper cache on **/.mvn/wrapper/maven-wrapper.properties, and cache-dependency-path overrides the pattern for the main dependency cache only, not for the additional one. setup-java itself carries such a file under __tests__/cache/maven, so the derived key depends on how many copies of the action are checked out. The seed jobs checked out one ref and the measure job checked out two, which produced different wrapper keys and a permanent miss. Every job now checks out both refs, including the arm it does not use, so all jobs hash an identical tree. The fixture check also reports a missing file directly instead of letting wc fail with a bare no-such-file error. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
An A/A run of the new workflow, with baseline-ref and candidate-ref both set to main, reported a 0.859s improvement on identical code. The A/A control was flat at -0.060s, so slot ordering was not the cause. The cause was that each arm seeded its own cache entry. A cache entry's download throughput depends on where the service placed the stored blob, and that placement is fixed for the life of the entry. In one job on one runner the baseline blob was served at 59.4 and 60.3 MB/s while the candidate blob was served at 131.8 and 105.1 MB/s, and the same ordering held on every runner. Because that bias is constant across runners rather than random, pairing cannot remove it, and additional samples only tighten the interval around the wrong answer. The arm was confounded with the blob. A single entry is now seeded, from the candidate so the wrapper cache is populated, and both arms restore it. The blob is therefore held constant across the comparison by construction rather than assumed not to matter. The README documents the confound and states that an A/A run is the check to perform after changing the harness. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
With one shared cache entry, running the whole matrix at once puts every runner on the same blob at the same moment. At 20 concurrent runners the mean restore rose from about 3s to about 8s and the noise floor from 0.34s to 1.89s, which is enough to hide the effect under test. That is contention no real workflow would experience, and it costs sensitivity for nothing. The matrix now runs in waves of four, so each measurement reflects an ordinary restore. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
These two workflows still measured the way the focused benchmark did before it was rebuilt: durations read from the Actions API's one-second clock, arms spread across independent jobs, no intervals, and one seeded cache entry per arm. Their sub-second results were therefore not evidence of anything, and the JDK cache workflow carried the same arm-versus-blob confound that an A/A run exposed in the focused benchmark. scripts/paired.mjs now holds the pairing logic that all three share: reading per-runner timing CSVs, grouping slots by arm, discarding runners that did not complete every slot, deriving the noise floor from within-arm repeats, and producing an interval, a permutation p-value and a verdict. The focused report is rebuilt on it unchanged in behaviour. JDK cache becomes a two-arm ABBA comparison. One seed job populates a single Maven entry and a single JDK entry, and both arms restore the same Maven entry, so only the JDK entry differs between them. cache-jdk false and true are then measured in one job per runner, with the tool cache and the local repository cleared before every slot. The action ref is now an input so a branch can be measured. The version sweep generalises ABBA to seven arms: every runner sets up v1 through main and then main back through v1, which places each version's two slots symmetrically about the middle of the job so drift cancels. Every version that supports cache-dependency-path restores one shared entry. v3 predates that input and keys on pom.xml, so it necessarily has its own entry and the report says to treat its difference with more caution. Versions are compared against main with a paired interval per runner, and main against itself supplies the A/A control. cache-read-only is not used in the sweep. It exists only on main, so applying it to some arms and not others would make post-job behaviour asymmetric; every slot restores on an exact primary-key hit, which actions/cache already skips saving. Both matrices run in waves of four, because every runner pulls the same seeded blob and running them all at once measures contention on the cache service that no real workflow would see. Tests cover the two properties the design depends on: that differencing within a runner removes between-runner speed differences, and that the mirrored order cancels drift that is linear across the job. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
The version sweep and the JDK cache benchmark both checked PetClinic out over the workspace root. The JDK cache workflow then had no copy of the benchmark scripts to invoke, and the version sweep, which checked them out to .benchmark, put them inside PetClinic's basedir, where its nohttp check failed the seed build on http:// URLs in a toolchains template. Check the benchmark repository out at the root and PetClinic below it instead. The scripts are then addressable as scripts/, PetClinic's build only sees its own tree, and the cache-identity markers move with it. Also stop jdk-cache.sh from truncating the wrapper properties; the seed job needs the real contents to build, and an idempotent append keeps the hashed file identical between seeding and measuring. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
A measurement job is a couple of minutes of work, but a slot can stall indefinitely on a cache download or a JDK fetch. One such runner in the version sweep sat on a step that takes two seconds elsewhere, and with the default six-hour job timeout it would have blocked the report for the rest of the day. Bound the measurement jobs at 25 minutes. A stalled runner now fails quickly and drops out of the report, which already discards runners that did not complete every slot, and the remaining runners still produce a verdict. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
The JDK cache benchmark's A/A control failed: comparing the no-cache arm's own first and last slot reported an improvement of 0.4s, 95% CI -0.7 to -0.1, on slots that ran identical configuration. The headline verdict could not be trusted while that was true. The cause is that the first setup in a job pays costs the later ones do not: DNS resolution, TLS handshakes to the cache service and to the JDK host, and a cold page cache. That is a one-off spike at slot 1 rather than drift across the job, so the mirrored slot order cannot cancel it, and it biases the arm that happens to hold the first slot. Run one unmeasured warm-up slot in every measurement job so the measured slots all start from the same warmed state. Applied to all three workflows rather than only the one where it was caught, since they share the structure that exposes it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
The sweep reported v2.5.1 as a 1.9s improvement over main. That number is real but it means v2 does no caching and so never restores the Maven repository the other versions spend most of their time on. Reported as an improvement it invites exactly the wrong conclusion. Report v1 and v2 as not comparable and say why. They are still measured, because seeing what the caching versions spend their time on is the point of including them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
brunoborges
changed the title
An A/A run with both refs set to main reported a 0.821s regression, 95% CI [0.119, 1.672], on identical code. Seven of the ten runners agreed to within 0.12s. Two had a single slot that stalled on the cache service — 8.169s against a 3.286s sibling, and 10.030s against 4.449s — and both stalls happened to land on the candidate arm, which is enough to move the mean of ten paired differences by more than any effect these workflows measure. Two changes, and the same data now reports -0.020s, 95% CI [-0.069, 0.030], p = 0.498, inconclusive. Runners whose own arm disagrees with itself by more than a robust threshold are discarded before the estimate is formed. The threshold is the median within-arm spread plus six times its median absolute deviation, so it adapts to the run rather than being a fixed number of seconds, and at most half the runners can be dropped. The decision uses only within-arm spread, which has the same distribution whether or not the arms differ; filtering on the arm difference itself would bias the result, but this cannot. Every report lists what it discarded and why. A verdict now also requires the permutation test to agree with the interval. The percentile bootstrap is only approximate at these sample sizes while the sign-flip test is exact under the null, so where they disagree the interval is the one that is wrong. In the run above the interval excluded zero while the test reported p = 0.099, and the verdict believed the interval. The statistic stays the mean rather than becoming a median. A sign-flip test on the median has almost no power here: six runners agreeing on a 12.5s effect gives p = 0.13. Robustness comes from the filter, not from the estimator. The failing A/A dataset is pinned as a regression test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
A slot in the version sweep sat on a cache restore for 214 seconds while its neighbours in the same job took three. It is the second time a run has been held open this way, and the version is incidental: the sweep restores the same 154 MiB blob fifteen times per job across ten jobs, and the cache service occasionally stalls one of them. Cap each measured setup at three minutes. A stalled slot now fails its runner, which the report discards along with the other incomplete ones, rather than running out the job timeout. Narrow the sweep's waves to two, since it restores three times as often per job as the two-arm workflows. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
A slot in the version sweep sat for 214 seconds on a setup that took 0 seconds in the same job's mirrored slot, on the same runner and the same action version. The action had already finished: it logged "maven cache is not found" and then produced no further output until the job was cancelled. The cause is in @actions/cache. Its promiseWithTimeout clears the timeout inside a .then, which only runs when the raced promise fulfils. When the cache service drops the blob mid-download, downloadToBuffer rejects, clearTimeout is skipped, and the armed timer keeps the event loop alive for the remainder of the segment timeout after all work is done. That default is 10 minutes. Cap it at 2 minutes here. These restores move 154 MiB in about 3 seconds, so the cap is far beyond any legitimate download and cannot abort real work; it only bounds the idle. The stall filter already keeps such a runner out of the estimate, so this is about not paying for the wait. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
The stall filter was only applied to the two-arm workflows, so the sweep still formed its estimates from runners that had stalled on a restore. It showed: its A/A control, comparing main with itself, reported 0.6s against a noise floor of 0.157s. Generalise the filter across arms. A runner whose two slots for any one version disagree by more than the robust threshold is dropped, on the same reasoning as before: which version a stall lands on is arbitrary, and the decision uses only within-version spread, so it cannot bias the differences between versions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
The Maven configuration warm path was the last workflow still measuring the old way. It ran the arms ABAB, which does not cancel drift across the job: with a steady slowdown of d per slot the candidate is biased by +d, because its slots sit later on average than the baseline's. It had no warm-up slot, so the first setup's DNS, TLS and cold page cache costs landed entirely on the baseline. And it summarised each configuration on its own, where one runner yields one paired difference and no interval is possible. Run the arms ABBA after a discarded warm-up slot, matching the other workflows, and add a report job that pools across the matrix. Each of the 36 configurations is a block: the arms are compared within it, on one runner, and the differences are combined. That answers what this workflow actually asks — whether the candidate differs from the baseline across configurations — and costs no extra jobs. Breakdowns by operating system and cache profile get their own intervals, and per-configuration numbers are published as single observations with no verdict attached, because that is all they are. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
Two ways the reports could publish something misleading rather than fail. formatInterval hard-coded "95% CI" while the interval it was given carries its own confidence level, so a caller that widened or narrowed an interval would have had it labelled 95% regardless. Read the level off the interval. The reports also checked only some of the environment variables they go on to render. A missing SETUP_JAVA_REPOSITORY or JAVA_VERSION reached the summary as "undefined" instead of stopping the run. Check every variable a report renders, in one shared helper, and name the ones that are missing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
The version sweep used to place all seven versions in one table and compare each against main. That is not a fair ranking: v1 and v2 are uncached, v3 uses an older cache-key contract, and v5.6/main restore the Maven Wrapper in addition to dependencies. Keep the same-runner mirrored measurements, but report three cohorts: uncached (v1/v2), dependency cache (v3/v4/v5.2), and dependency plus wrapper cache (v5.6/main). Each cohort has its own reference and A/A control. Apply Holm's step-down correction within a cohort so multiple comparisons do not turn an uncertain p-value into a headline verdict. Retain the all-arm diagnostics for runner quality and workload shape, explicitly not as a ranking. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
Collaborator
Author
|
Updated the version sweep based on the review: it now reports three behaviorally comparable cohorts (uncached v1/v2; dependency cache v3/v4/v5.2; dependency + wrapper cache v5.6/main), with a cohort-specific reference, A/A control, interval, and Holm-adjusted p-values. The all-arm analysis remains only as a runner/workload diagnostic and is explicitly not a ranking. README and regression tests were updated; 35 tests pass. |
The previous commit split the sweep into three cohorts, each with its own reference. That was wrong in two ways. It deleted the comparison the sweep exists to make. v4 and v5.2 are what users are actually running, and after the split neither could be compared with main at all: they sat in a cohort referenced against v5.2, while main sat in another. It also gave a verdict to a comparison the harness cannot make. v3 keys on pom.xml, so it restores its own cache entry, and a blob's throughput is fixed for the life of that entry. The README already says pairing cannot remove that confound. In run 30979614347 v3 took 3.69s and 3.89s on two runners that ran v4 in 0.50s and 0.52s moments later in the same job, and the cohort report published that sevenfold gap as a `regression` at p=0.046. Rank against main again, but rank only v4, v5.2 and v5.6 - the versions that restore the same seeded entry, so that a difference between them is a difference in the implementation. Keep Holm's correction, which was the sound half of the previous commit, and apply it across that family. Measure v1, v2 and v3 as before and publish their durations, each with the reason it carries no verdict, because for them a difference in duration is a difference in the workload. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
main is the newest code, so calling v5.6.0 a `regression` for being slower than it states the finding backwards. Nothing regressed in v5.6.0; it shipped, and then main got faster. The table was reading the difference in the direction that made the older code look at fault for the improvement that came after it. The two-arm workflows already have this right: they difference candidate minus baseline, so the verdict describes the code under test. The sweep is the same question with more baselines, so difference main minus each version and let the verdict describe main. v5.6.0 now reads as a 1.303s `improvement` in main rather than a 1.303s `regression` in v5.6.0, which is the same measurement said the right way round. Pin the direction with a test, because the sign is easy to flip back and the resulting table is wrong in a way that still looks plausible. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
The Maven configuration workflow was called a warm path and never had a seed job, so its synthetic pom hashed to a key nothing had stored and every "warm" restore was a miss. Its three cache profiles were three variations on a failed lookup. The focused workflow, meanwhile, compared the same two refs on the same warm restore as the version sweep. Between them the benchmarks measured one quantity, repeatedly, and it is mostly not a quantity setup-java controls: the transfer is handed to @actions/cache, so a large fixture times the network. Identical code varied ninefold across runners while the effect under test was a third of that spread. Reorganise them by question. Cache value builds PetClinic for real, cached against uncached, to establish the scale everything else is a fraction of. Cache key stability asserts the key rather than timing it, because a key that moves when it should not costs a whole miss. Action overhead keeps the configuration matrix but seeds a megabyte-sized entry so restores genuinely hit and the action's own work is what remains; its four cache profiles are now nested levels that difference into a decomposition. Transfer overlap is the same measurement at 160 MiB, where concurrency has something to hide. Cache save measures what the first run pays, which nothing measured before. Every action-overhead slot records cache-hit, and the report refuses to be read normally when a profile did not do the work its name claims. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
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
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.
The focused cache restore benchmark could not distinguish a real improvement from runner noise, so its headline numbers were not evidence of anything.
The benchmark reported effects that do not exist
Runs 30972482195 and 30973171928 measured the same unchanged
actions/setup-java@v4.8.0twelve minutes apart. The true difference is exactly zero.2,1,3,1,1,2,3,2,2,23,4,2,4,4,3,4,3,3,1A 1.2s separation whose bootstrap 95% CI is
[-1.97, -0.43]— it excludes zero. Over the same two runs the reportedmainvsv4delta flipped from +0.6s to -0.8s. Every published-1.0swas inside the noise band.Root causes:
sample Nof one arm withsample Nof the other, but those jobs shared no runner and no point in time. It removed no variance while the label implied precision the data did not have. No intervals were reported anywhere.What changed
scripts/measure.mjstimes setup from inside the job at millisecond resolution.~/.m2first.scripts/stats.mjsadds seeded bootstrap intervals, a paired permutation test, and a Hodges-Lehmann shift, and converts them into an explicit verdict. A comparison whose interval includes zero reportsinconclusiverather than a number that looks like a result.setup-java-repository/baseline-ref/candidate-refinputs, so a PR branch can be measured directly.The A/A check found a confound in this PR
I ran the new workflow with
baseline-refandcandidate-refboth set tomain. The true effect is exactly zero, so anything else is a harness defect. It reported a 0.859s improvement (run 30974285173). The A/A control was flat, so slot ordering was not the cause.The cause was that each arm seeded its own cache entry. A cache entry's download throughput depends on where the service placed the stored blob, and that placement is fixed for the entry's life. In one job on one runner:
The same ordering held on every runner. Because that bias is constant across runners rather than random, pairing cannot remove it and more samples only tighten the interval around the wrong answer. The arm was confounded with the blob. One entry is now seeded and both arms restore it, holding the blob constant by construction.
Validation on real runners
mainvsmain(A/A)inconclusivev4.8.0vsmaininconclusivev4.8.0vsmain, waves of 4inconclusiveThe A/A run reports no effect, as it must. Capping concurrency cut the noise floor 11x (1.890s to 0.170s) and the interval width 10x.
Synthetic validation with a known injected effect under realistic between-runner spread:
inconclusiveinconclusiveimprovementWhat this says about
mainOn this scenario
mainis not measurably different fromv4.8.0: +0.073s with a 95% CI of [-0.107, +0.217]. The interval is tight enough to rule out the previously reported-1.0simprovement outright.26 tests pass. The real A/A dataset is pinned in
scripts/stats.test.mjsso unpaired sampling is not reintroduced.The same treatment, applied to the other three workflows
Benchmark setup-javaandJDK cachehad the same three defects, so they now share the machinery.scripts/paired.mjsholds the pairing, grouping, noise floor and verdict logic that all three reports build on.JDK cache became a two-arm ABBA comparison of
cache-jdk: falseagainstcache-jdk: trueon the same action ref, so the result isolates JDK caching rather than conflating it with implementation changes. One seed job populates a single Maven entry that both arms restore; only the JDK entry differs between them.Benchmark setup-java became a seven-arm mirrored sweep. Every runner sets up
v1..mainand thenmain..v1in one job, which places each version's two slots symmetrically about the middle of the job. Every version supportingcache-dependency-pathrestores one shared entry.cache-read-onlyis not used, because it exists only onmainand applying it asymmetrically would change post-job behaviour between arms.Three more A/A failures, each a real defect
Running each harness against itself is what found every remaining problem. In each case the true effect is exactly zero, so any other verdict is a defect rather than a finding.
A one-off first-slot cost. The first JDK cache run under the new harness failed its control: comparing the no-cache arm's own first and last slot, which run identical configuration, reported -0.4s, 95% CI [-0.7, -0.1]. The first setup in a job pays DNS resolution, TLS handshakes to the cache service and the JDK host, and a cold page cache. That is a spike at slot 1, not drift across the job, so mirrored ordering cannot cancel it — it simply biases whichever arm holds the first slot. Every job now runs one unmeasured warm-up slot.
A single stalled slot dominating the mean. A focused A/A with both refs on
mainreported "Slower", +0.821s, 95% CI [0.119, 1.672] (run 30976592589). Seven of ten runners agreed to within 0.12s; two had one slot that stalled on the cache service (8.169s against a 3.286s sibling, 10.030s against 4.449s) and both landed on the candidate arm by chance.Runners whose own arm disagrees with itself by more than a robust threshold are now discarded. That decision uses only within-arm spread, which has the same distribution whether or not the arms differ, so unlike filtering on the arm difference it cannot bias the result. On the same data: -0.020s, 95% CI [-0.069, 0.030], p = 0.498,
inconclusive. The dataset is pinned as a regression test.The estimator stays the mean rather than becoming a median — a sign-flip test on the median has almost no power here, giving p = 0.13 for six runners agreeing on a 12.5s effect. Robustness comes from the filter.
Interval and test disagreeing. A verdict now requires the permutation test to agree with the bootstrap interval. The percentile bootstrap is only approximate at ten paired observations while the sign-flip test is exact under the null, so where they disagree the interval is wrong. This earned its keep immediately: the revalidation run had an interval excluding zero but p = 0.081, and correctly reported
inconclusive.Stalls no longer hold a run open
Two runs were held open by a slot sitting on a cache restore for minutes while its neighbours took seconds. Each measured setup is capped at three minutes and each job at 25, so a stall now costs one runner — which the report already discards — instead of the run. The sweep restores fifteen times per job against the two-arm workflows' five, so it runs in narrower waves.
Results
A clean run of all four workflows from this branch. Every control comes back clean, which is what makes the headline numbers admissible.
JDK cache, 20 runners (run 30981227720):
cache-jdk: truevsfalseimprovementinconclusiveNoise floor 0.202s, no runner discarded. The JDK cache benefit is measured rather than asserted. An earlier run put it at -2.9s; that estimate was inflated by stalled runners the filter now removes, which is the difference between a number and a number you can rely on.
Focused cache restore,
v4.8.0vsmain, 20 runners (run 30981229350):mainvsv4.8.0within-noiseinconclusiveNoise floor 0.057s, 4 runners discarded. This is the harness declining to sell a result it cannot support: the interval excludes zero and p clears 0.05, but the estimate is smaller than the floor, so it reports
within-noiseinstead of a 49ms regression. Under the old harness this would have been a headline.Version sweep, 10 runners, warm restore path (run 30981226274):
mainis the newest code, so it is the thing under test and each released version is a baseline it is measured against — the same direction the two-arm workflows use. Every number below describesmain, and animprovementmeansmainis faster than that version. Only versions that do the same work from the same stored cache entry are ranked, with Holm's step-down correction across that family.mainvs itmaininconclusiveinconclusiveimprovementinconclusiveNoise floor 0.083s, no runner discarded.
mainis indistinguishable from v4 and v5.2.mainis measurably faster than v5.6.0, and this is the one sweep finding that survives scrutiny: it has now reproduced across three independent runs, and v5.6.0 is the slower arm on all ten runners individually in each of them, which no amount of blob luck explains. The point estimate moves between runs (-0.4s here, -1.3s in the previous run) because it scales with how slow the runners happen to be that day; the sign does not move.An earlier revision of this PR reported that same measurement as a
regressionin v5.6.0. That states it backwards — nothing regressed in v5.6.0; it shipped, and thenmaingot faster. The sweep now differencesmainminus each version so the verdict describes the code under test, and a test pins the direction.v1, v2 and v3 are measured on the same runners and their durations published, but they carry no verdict, because for them a difference in duration is a difference in the workload rather than in the implementation:
cache-dependency-pathand keys onpom.xml, so it restores its own entry — a blob's throughput is fixed for the life of the entry, and pairing cannot remove that confoundThe old report would have called v2 a 1.5s improvement over
main. An intermediate version of this PR split the sweep into three cohorts with three references; that was reverted, because it deleted the v4/v5-vs-maincomparison the sweep exists to make, and it handed v3 a verdict at p=0.046 — a live false positive of exactly the kind this PR exists to prevent. A stored blob's throughput is fixed for the life of the entry, and pairing cannot remove that.Maven configuration warm path,
v4.8.0vsmain, 36 configurations (run 30981230695):inconclusiveimprovementregressionregressioninconclusiveNoise floor 0.027s, 4 configurations discarded. This workflow runs each configuration once, so it pools paired differences across the matrix instead of quoting a per-cell verdict it cannot support. The pooled number is
inconclusivebecause two real effects of opposite sign cancel:mainis about 0.14s faster than v4.8.0 when no cache is configured and about 0.11s slower when one is. Both signs and magnitudes reproduce across runs. Neither is visible in the pooled figure, which is exactly why the report breaks the matrix down rather than publishing a single number.37 tests pass. The original A/A dataset is pinned in
scripts/stats.test.mjsso unpaired sampling cannot be reintroduced, and tests cover the properties the design rests on: that differencing within a runner removes between-runner speed differences, that the mirrored order cancels drift linear across the job, and that a stalled runner is excluded.Update: benchmarks rescoped by question
The three timing workflows were measuring one quantity repeatedly, and mostly not one setup-java controls.
maven-configuration-warm-pathwas not a warm path. It had no seed job. Its syntheticbenchmark/pom.xmlhashed to a key nothing had ever stored, so every "warm" restore was a miss — job logs readmaven cache is not foundandPath Validation Error ... hence no cache is being saved. Its three cache profiles were three variations on a failed lookup, and the −0.144 s / +0.11 s figures it published were cold-miss effects under a heading that said warm.focused-cache-restoreduplicated the version sweep, comparing v4.8.0 againstmainon the same warm restore.@actions/cache. Identical code varied 9.8× across runners (SD 1.208 s) while the effect under test was 0.440 s — 0.36× that spread.The workflows now
maincompare per release?Action overhead and Transfer overlap are the same measurement at two fixture sizes, deliberately. A large fixture measures the network; a small one measures the action. Together they decompose a restore into the part setup-java controls and the part it does not, and the 0.44 s overlap win from actions/setup-java#1174 should appear in one and not the other.
Action overhead's four cache profiles are now nested levels —
none,maven-miss,maven-hit,gradle-miss— that difference into a published decomposition, so the report says where the time goes rather than only whether two refs differ. Every slot recordscache-hit, and the report leads with a failure banner if amaven-hitslot missed, because that would silently turn one level into another.Cache key stability measures nothing: the key is a deterministic function of the tree, and a key that moves when it shouldn't costs a full miss (~1 min on PetClinic) where the timing workflows resolve tens of milliseconds. It stores nothing either — setup-java skips its post-job save when the cache path doesn't exist, and no probe creates
~/.m2.The methodology from earlier in this PR is unchanged and applies to all of them: millisecond in-job timing, same-runner mirrored ordering, one shared cache entry per comparison, a discarded warm-up slot, stalled-runner dropping, and a verdict that requires the permutation test and the interval to agree.
58 unit tests pass; shellcheck and YAML parse clean.