Skip to content

fix: render platforms without paired baselines - #9

Open
cpunion wants to merge 1 commit into
xgo-dev:mainfrom
dastai:codex/partial-platform-baseline-20260822
Open

fix: render platforms without paired baselines#9
cpunion wants to merge 1 commit into
xgo-dev:mainfrom
dastai:codex/partial-platform-baseline-20260822

Conversation

@cpunion

@cpunion cpunion commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

A pull request may introduce a platform that its base cannot build yet. In that case, requiring every result artifact to contain a baseline either hides the new platform or makes publication fail.

This change keeps same-runner pairing strict within each platform while allowing a complete platform to have no baseline. Reports retain paired deltas for platforms with baselines and render metrics from newly supported platforms as new, with an explicit note.

Validation:

  • npm run check (39 tests)
  • npm run build
  • rendered the real three-platform artifacts from cpunion/llgo run 32520873182: Linux/macOS retain paired deltas and Windows renders all current program and core benchmark values as new

@github-actions

Copy link
Copy Markdown

setup-benchmark-go-action

6d4d1e394e5b | workflow run | long-term charts

Action bundle sizes

Bundle Size vs main
RecordBundle 330342 B +224 B / +0.1% (worse)
RenderBundle 330885 B +224 B / +0.1% (worse)
Parser and validation operations
Group Benchmark Median time vs main
Grouping GroupRules100 160426.746 ns/op -10624 ns/op / -6.2% (better)
Parsing ParseGoOutput40 195814.776 ns/op +2416 ns/op / +1.2% (worse)
Validation ValidateArtifact40 207071.205 ns/op +661.6 ns/op / +0.3% (worse)

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

@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.

Review summary

This change cleanly relaxes the same-runner baseline rule from a global "baseline required for every result artifact" to a per-platform, all-or-nothing rule: a platform may omit its baseline entirely, but presence must be consistent across all shards of the same platform. The implementation is solid:

  • src/artifact.js uses a platformHasBaseline map with a symmetric, order-independent consistency check (previous === hasBaseline), and the specific per-platform assert message matches the updated test.
  • Any baseline that is present is still fully validated (schema, shardId, platform, and sample pairing), so the relaxation drops only a coverage/completeness guarantee, not an integrity one.
  • baselines.length === 0 ? [] : mergeShards(baselines) correctly guards mergeShards against an empty set.
  • Untrusted fields rendered in the report note (baseline.source.sha, baseline.source.url) remain validated (40-hex SHA, HTTPS-only URL wrapped in <...> autolink) — no injection surface introduced.
  • Test coverage is good: rejects inconsistent shards within a platform, allows a whole platform without a baseline, and verifies the new marking plus the note text.

One documentation follow-up below. No blocking issues.


Findings

[P2] Stale README: baseline behavior no longer matches the per-platform logic

README.md (lines 424–430) still documents the old all-or-nothing model:

When every platform artifact includes baseline-benchmark-file, each metric is compared with that paired baseline measured by the same runner job... Otherwise, each metric is compared with the newest matching platform in main.

After this PR that wording is inaccurate. With mixed platforms (some with a same-runner baseline, some without), the paired platforms render vs base while the unpaired platforms are marked new (metricComparison returns an undefined baseline) — they do not fall back to the main series. The comparison decision is now per platform, not a single global switch. Consider updating this paragraph to describe the per-platform behavior and note that platforms lacking a paired baseline are marked new, matching the new report note in src/report.js.

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