Skip to content

ci(bench): compare base and PR benchmarks on one runner - #59

Merged
sentomk merged 1 commit into
mainfrom
ci/pr-benchmark-comparison
Oct 16, 2026
Merged

sentomk merged 1 commit into
mainfrom
ci/pr-benchmark-comparison

Conversation

@sentomk

@sentomk sentomk commented Oct 16, 2026

Copy link
Copy Markdown
Owner

Summary

Third step of the benchmark chart rework. The PR benchmark job used to run the benchmarks once and only upload the JSON, so the first time anyone saw a chart was after the merge. It now builds and runs the benchmarks for the PR base and for the PR merged into its base back to back on the same runner, and posts the comparison as a sticky comment on the PR. The job stays informational and does not block merging.

Changes

  • ci-pr.yml, benchmark job: check out the base commit and the PR merge result side by side, build ptn_bench for both, run both with the existing scenario filter, run scripts/bench_compare.py for the chart and report, then post or update one PR comment. The job-level permissions are contents: read and pull-requests: write. The comment step is skipped for pull requests from forks, which only get the artifact.
  • New scripts/bench_pr_comment.py builds the comment: Patternia rows marked slower, faster or within noise, and the remaining implementations (std::visit, switch, ...) as control rows. The controls do not run Patternia code, so how far they move between base and PR is used as the noise estimate, and a Patternia difference is only called out above max(3%, 2 x noise).
  • The raw JSON, the comparison chart and the comment text are uploaded as the benchmark-compare-<PR number> artifact. The scale benchmark (ptn_bench_scale) is no longer run on pull requests; the post-merge publish still produces its chart.
  • The job takes roughly twice as long as before, which is acceptable because it is not a required check.

Testing

  • Parsed the workflow YAML with Ruby; the job names are unchanged and Benchmark (informational) is still not among the required checks.
  • Ran the compare step end to end on local benchmark JSON (the v0.9.2 and v0.9.3 builds, six scenarios): the chart, the report and the comment were produced, and the comment correctly marked both Patternia rows as within noise while a control row moved by 4.9%.
  • Exercised the create-or-update logic of the comment step against a stub of gh: it posts a new comment when none carries the marker and patches the existing one otherwise, and the jq filter selects only the marked comment.
  • Not run on GitHub Actions yet. This PR changes the workflow it is checked by, so its own run is the end-to-end test: the comment should appear on this PR and, since nothing here touches the benchmarks, report everything as within noise. I will check that the comment appears, the artifact is attached and the job conclusion is sensible.
  • Known limitations: base is always run before the PR, so a systematic ordering effect is possible (the control rows would show part of it); and the existing chart script truncates long benchmark names in the PNG.

Build and run the benchmarks for the PR base and the PR merge result
back to back on the same runner, and post the comparison as a sticky PR
comment. Rows that do not run Patternia code (std::visit, switch) serve
as controls for the measurement noise. The comparison chart and raw JSON
stay available as an artifact. The job remains informational and does
not block merging.
@codecov

codecov Bot commented Oct 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

Copy link
Copy Markdown
Contributor

Benchmark comparison (informational)

Base ee454c9 vs PR cae40bc, run one after the other on the same GitHub-hosted runner. This does not block merging.

Noise estimate: the 20 control rows below do not run Patternia code, so they should not change between base and PR; they moved by a median of 1.5%. Differences under 3.0% are treated as noise.

Patternia: 8 benchmarks, 0 slower, 1 faster, 7 within noise.

Benchmark Base (ns) PR (ns) Delta Verdict
PatterniaPipe CommandParser 1.716 1.733 +1.0% within noise
PatterniaPipe PacketMixed 1.598 1.490 -6.8% faster
PatterniaPipe PacketMixedHeavyBind 1.592 1.562 -1.9% within noise
PatterniaPipe ProtocolRouter 1.387 1.369 -1.3% within noise
PatterniaPipe VariantAltHot 1.122 1.128 +0.6% within noise
PatterniaPipe VariantAltHotGuarded 1.218 1.196 -1.8% within noise
PatterniaPipe VariantMixed 1.344 1.355 +0.9% within noise
PatterniaPipe VariantMixedGuarded 1.153 1.151 -0.1% within noise
Control rows (not Patternia code)
Benchmark Base (ns) PR (ns) Delta
IfElse CommandParser 1.629 1.608 -1.3%
IfElse ProtocolRouter 2.106 2.059 -2.2%
Sequential VariantAltHot 1.269 1.269 -0.0%
Sequential VariantAltHotGuarded 1.349 1.309 -3.0%
Sequential VariantMixed 0.924 0.926 +0.3%
Sequential VariantMixedGuarded 1.125 1.104 -1.9%
StdVisit CommandParser 1.135 1.095 -3.5%
StdVisit ProtocolRouter 1.391 1.371 -1.4%
StdVisit VariantAltHot 1.132 1.111 -1.9%
StdVisit VariantAltHotGuarded 1.192 1.170 -1.9%
StdVisit VariantMixed 1.360 1.323 -2.7%
StdVisit VariantMixedGuarded 1.795 1.771 -1.4%
SwitchIndex VariantAltHot 1.272 1.263 -0.7%
SwitchIndex VariantAltHotGuarded 1.322 1.321 -0.1%
SwitchIndex VariantMixed 0.821 0.828 +0.8%
SwitchIndex VariantMixedGuarded 0.924 0.917 -0.7%
Switch CommandParser 1.625 1.572 -3.3%
Switch PacketMixed 1.140 1.122 -1.6%
Switch PacketMixedHeavyBind 1.473 1.456 -1.1%
Switch ProtocolRouter 1.426 1.377 -3.4%

Raw results and the comparison chart are attached to this workflow run as an artifact. Shared runners are noisy, so use this as a trend, not a measurement.

@sentomk
sentomk merged commit b980395 into main Oct 16, 2026
24 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