Skip to content

ci(bench): remove run drift and raise the noise floor in the PR comment - #60

Merged
sentomk merged 1 commit into
mainfrom
ci/bench-comment-noise-offset
Oct 16, 2026
Merged

sentomk merged 1 commit into
mainfrom
ci/bench-comment-noise-offset

Conversation

@sentomk

@sentomk sentomk commented Oct 16, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up to the PR benchmark comment. The first real run of the comment (on #59, a pull request that changes no benchmark code) showed two things: the later run came out systematically faster than the earlier one, and one Patternia row was marked "faster" although nothing had changed. This change tries to account for the first and makes the verdict more conservative. It is a heuristic and, on the data available, it only partly helps; the limits are listed below.

Changes

  • scripts/bench_pr_comment.py: take the median signed change of the control rows (the implementations that do not run Patternia code) as the offset the later run picked up from run order and machine drift, and divide it out of the Patternia deltas.
  • The noise estimate is now the scatter of the control rows after that offset is removed, instead of the median absolute change.
  • The minimum noise threshold goes from 3% to 5%; the threshold used is max(5%, 2 x scatter).
  • The table gets an Adjusted column next to the raw Delta, and the verdict is based on Adjusted. With fewer than three control rows the offset cannot be estimated, so the comment falls back to raw deltas against the 5% floor and says so.

Testing

  • Ran the script on synthetic variants of the real artifact from ci(bench): compare base and PR benchmarks on one runner #59: a uniform +4% drift on every row of the PR run gives adjusted deltas of +0.0% and no verdicts; a -3% drift combined with a real +20% regression on one Patternia row shows +16.4% raw, +20.0% adjusted and a single "slower"; with fewer than three control rows it falls back as described above.
  • python3 -m py_compile passes.

Limits on real data (please read before relying on the verdicts)

I re-ran the script on the real artifact of #59, a run with no code change, which is the only real sample I have:

  • 18 of the 20 control rows moved negative, with a median of -1.5%, which is what motivated the offset.
  • The Patternia rows did not share that offset: their raw median is -0.7%. Dividing out the control offset therefore moved several of them away from zero (for example CommandParser +1.0% raw, +2.5% adjusted). The mean absolute delta over the eight Patternia rows is 1.79% raw and 1.84% adjusted, so the correction does not reduce their scatter on this sample.
  • PacketMixed is still marked "faster": raw -6.8%, adjusted -5.4%, against a threshold of 5.0%. The false positive is therefore only reduced to a borderline case, not removed.
  • The 5% floor is the part that is doing the work here. Both the offset correction and the floor were chosen from this single sample, so they may be overfitted; a real 3 to 5% change will not be flagged.
  • Within-run variation (the coefficient of variation, about 2% per benchmark) is much smaller than the differences between the two runs, so it cannot be used as the noise estimate.

More principled alternatives, not done here: run base, PR, base, PR and use the difference between the two runs of the same binary as that benchmark's own noise (about 10 more minutes of job time), or also require a multiple of each benchmark's coefficient of variation.

Divide the median change of the control rows out of the Patternia
deltas, so a systematic offset between the two runs is not read as a
change, and raise the minimum noise threshold from 3% to 5%. The
comment shows the adjusted delta next to the raw one and says how the
offset was estimated.
@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 b980395 vs PR 423aeb6, 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. As a group they moved by a median of -0.0% (run order and machine drift); that offset is divided out of the Patternia deltas (Adjusted). The controls then scatter by a median of 0.1%. Adjusted differences under 5.0% are treated as noise.

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

Benchmark Base (ns) PR (ns) Delta Adjusted Verdict
PatterniaPipe CommandParser 1.773 1.775 +0.1% +0.2% within noise
PatterniaPipe PacketMixed 1.623 1.621 -0.1% -0.0% within noise
PatterniaPipe PacketMixedHeavyBind 1.681 1.682 +0.0% +0.1% within noise
PatterniaPipe ProtocolRouter 1.613 1.613 -0.0% +0.0% within noise
PatterniaPipe VariantAltHot 1.558 1.556 -0.1% -0.1% within noise
PatterniaPipe VariantAltHotGuarded 1.712 1.713 +0.0% +0.0% within noise
PatterniaPipe VariantMixed 1.998 1.998 -0.0% +0.0% within noise
PatterniaPipe VariantMixedGuarded 1.995 1.996 +0.0% +0.1% within noise
Control rows (not Patternia code)
Benchmark Base (ns) PR (ns) Delta
IfElse CommandParser 2.178 2.201 +1.0%
IfElse ProtocolRouter 2.008 1.906 -5.1%
Sequential VariantAltHot 1.713 1.713 +0.0%
Sequential VariantAltHotGuarded 1.869 1.869 -0.0%
Sequential VariantMixed 1.167 1.167 -0.0%
Sequential VariantMixedGuarded 1.425 1.427 +0.1%
StdVisit CommandParser 1.395 1.389 -0.4%
StdVisit ProtocolRouter 1.522 1.518 -0.3%
StdVisit VariantAltHot 1.557 1.557 -0.0%
StdVisit VariantAltHotGuarded 1.713 1.713 +0.0%
StdVisit VariantMixed 1.993 1.990 -0.1%
StdVisit VariantMixedGuarded 2.152 2.152 +0.0%
SwitchIndex VariantAltHot 1.713 1.712 -0.0%
SwitchIndex VariantAltHotGuarded 1.869 1.868 -0.1%
SwitchIndex VariantMixed 0.987 1.004 +1.8%
SwitchIndex VariantMixedGuarded 1.438 1.429 -0.6%
Switch CommandParser 2.250 2.249 -0.0%
Switch PacketMixed 1.459 1.453 -0.4%
Switch PacketMixedHeavyBind 1.499 1.486 -0.9%
Switch ProtocolRouter 1.607 1.598 -0.6%

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 c5d9e3c into main Oct 16, 2026
24 checks passed
@sentomk
sentomk deleted the ci/bench-comment-noise-offset branch October 16, 2026 12:35
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