Skip to content

feat: report paired benchmark differences - #7

Merged
cpunion merged 3 commits into
xgo-dev:mainfrom
dastai:codex/paired-same-runner-delta
Aug 18, 2026
Merged

feat: report paired benchmark differences#7
cpunion merged 3 commits into
xgo-dev:mainfrom
dastai:codex/paired-same-runner-delta

Conversation

@cpunion

@cpunion cpunion commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

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

  • add opt-in sample-pairing: index for benchmark files whose repetitions correspond by occurrence
  • validate matching benchmarks, sample counts, and metrics at every paired sample index
  • show every comparable metric as signed unit difference / percentage; paired reports use the median pairwise differences and percentage changes, for example -1.321 ns/op / -23.0% (better)
  • keep the existing summary comparison when pairing is not declared
  • preserve enough precision for very small nonzero differences and percentages
  • index baseline benchmarks once per platform, keeping report rendering O(B·U)

Validation

  • npm run check (33 tests)
  • npm run build
  • npm run benchmark
  • rendered the LLGo #2027 Linux and macOS artifacts; an unchanged noisy benchmark changes from a misleading median-ratio +6.0% to a paired -0.11 ns/op / -0.9%, while the consistent interface-call result remains -1.321 ns/op / -23.0%

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.js validateSamplePairing verifies 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.js formatPreciseNumber: magnitudes below ~1e-7 render as 0 after 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's samplePairing, while the actual paired path also requires the baseline to be index-paired with matching counts. validateSamplePairing keeps these consistent today, so the note is accurate; just coupled indirectly.

Comment thread src/comparison.js Outdated
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown

setup-benchmark-go-action

8d26b1027220 | workflow run | long-term charts

Action bundle sizes

Bundle Size vs main
RecordBundle 330118 B +1.1% (worse)
RenderBundle 330661 B +1.1% (worse)
Parser and validation operations
Group Benchmark Median time vs main
Grouping GroupRules100 148480.834 ns/op -35.3% (better)
Parsing ParseGoOutput40 182503.771 ns/op -30.0% (better)
Validation ValidateArtifact40 198642.641 ns/op -33.5% (better)

Compared only with the latest matching platform in the main series.

@cpunion

cpunion commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

All review observations are covered by the current head:

  • c047c6a removes the repeated linear benchmark lookup, validates the unit set at each paired sample index, preserves tiny nonzero deltas, and only emits the pairwise-median note when both sides are index-paired.
  • The inline performance thread is resolved.
  • 8d26b10 keeps the wording and generated bundles consistent with the signed absolute/percentage format.

The latest CI and self-benchmark jobs pass.

@cpunion
cpunion force-pushed the codex/paired-same-runner-delta branch from 8d26b10 to a18c693 Compare August 18, 2026 05:56
@cpunion
cpunion merged commit 69df5d4 into xgo-dev:main Aug 18, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant