feat: report paired benchmark differences - #7
Conversation
There was a problem hiding this comment.
FennoAI Review
Reviewed the sample-pairing: index feature (pairwise-median absolute + percentage change reporting for same-runner paired samples). The implementation is careful and well-tested: every rendering precondition is guarded before switching to the paired path, division-by-zero and undefined-percentage cases are handled, validateSamplePairing enforces matching benchmark/unit/sample counts on both write and load, and the dist/ bundles were regenerated. Security review found no injection or trust-boundary issues (units/keys are sanitized; baseline source is re-pinned to the PR target). Documentation matches the implemented behavior. All 19 affected tests pass locally.
One inline performance note below, plus a few minor observations. Nothing blocking.
Minor observations (no inline anchor needed):
src/artifact.jsvalidateSamplePairingverifies per-unit sample counts match but not that a unit occupies the same sample indices on both sides. In practice every Go benchmark sample carries all units, so this is unlikely to trigger — but the README claims the recorder "rejects ... rather than silently breaking the pairing," and a unit present at different positions with equal total count would pair mismatched repetitions. Consider asserting each sample carries the full unit set.src/presentation.jsformatPreciseNumber: magnitudes below ~1e-7 render as0after trailing-zero stripping, so a tiny nonzero paired difference can display as+0 <unit>(sign from the original nonzero value). Edge display artifact only.- The report note in
src/report.js("...pairwise medians") is keyed only on the current platform'ssamplePairing, while the actual paired path also requires the baseline to be index-paired with matching counts.validateSamplePairingkeeps these consistent today, so the note is accurate; just coupled indirectly.
setup-benchmark-go-action
Action bundle sizes
Parser and validation operations
Compared only with the latest matching platform in the main series. |
|
All review observations are covered by the current head:
The latest CI and self-benchmark jobs pass. |
8d26b10 to
a18c693
Compare
Problem
A same-runner report currently compares the head median with the base median. That discards intentional sample pairing: runner drift can make the ratio of independent medians disagree with the median pairwise change. Reports also show only the percentage, not the signed difference in the metric's unit.
Changes
sample-pairing: indexfor benchmark files whose repetitions correspond by occurrencesigned unit difference / percentage; paired reports use the median pairwise differences and percentage changes, for example-1.321 ns/op / -23.0% (better)Validation
npm run check(33 tests)npm run buildnpm run benchmark+6.0%to a paired-0.11 ns/op / -0.9%, while the consistent interface-call result remains-1.321 ns/op / -23.0%