Skip to content

ci: Require an absolute delta before flagging a benchmark regression - #10701

Closed
kokokoXUY wants to merge 1 commit into
parse-community:alphafrom
kokokoXUY:ci/performance-noise-floor
Closed

kokokoXUY wants to merge 1 commit into
parse-community:alphafrom
kokokoXUY:ci/performance-noise-floor

Conversation

@kokokoXUY

@kokokoXUY kokokoXUY commented Sep 26, 2026 •

Copy link
Copy Markdown

Pull Request

Issue

Related to #10681 — item 1 of the tracking list (the other items are untouched).

Approach

.github/workflows/ci-performance.yml flagged a regression on relative change alone. On benchmarks measured in fractions of a millisecond, ordinary runner noise clears 25% trivially: on #10656 the only entry over threshold was Object.save (create) at 0.52 ms → 0.66 ms (+26.2%) — a 0.14 ms difference on a lock-file-only bump of a dev dependency that cannot affect runtime performance. The job passed unchanged on re-run.

The comparison now requires the absolute delta to clear a floor before the percentage decides the status:

// Ignore relative changes below this absolute delta (milliseconds).
const ABSOLUTE_NOISE_FLOOR_MS = 2;
...
const absoluteChange = prValue - baseValue;
const clearsNoiseFloor = Math.abs(absoluteChange) >= ABSOLUTE_NOISE_FLOOR_MS;

if (change > 50 && clearsNoiseFloor) { ... }
else if (change > 25 && clearsNoiseFloor) { ... }
else if (change < -25 && clearsNoiseFloor) { ... }

2 ms is the lower end of the range suggested in #10681. It is a single named constant so it can be tuned, or replaced by a run-to-run variance measure, without touching the branches.

Verification

The workflow still parses as YAML and the edited logic was exercised with the values from #10681:

noise 0.52 -> 0.66:      change=26.9%   before[regression=true]   after[regression=false]
real regression 10 -> 30: change=200.0%  before[regression=true]   after[regression=true]
real speedup 30 -> 10:   change=-66.7%  before[faster=true]       after[faster=true]

The first row is the reported false positive; the other two confirm genuine changes above the floor still classify exactly as before.

Limitation, stated plainly: I could not run the workflow itself locally because it needs the benchmark artifacts from a previous run, so this is a logic-level check of the edited branch rather than an end-to-end run. The YAML was parsed after the edit.

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK

No test file added: the comparison logic lives inline in the workflow's shell heredoc, which this repository does not unit-test. If you would rather have it covered, the conversion script would need to be extracted into a checked-in JavaScript module first — happy to do that instead, just say so.

Summary by CodeRabbit

  • Bug Fixes
    • Benchmark comparisons now avoid flagging changes under 2 ms as faster or slower, reducing status changes caused by small timing fluctuations.

@parse-github-assistant

Copy link
Copy Markdown

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant Bot changed the title ci: require an absolute delta before flagging a benchmark regression ci: Require an absolute delta before flagging a benchmark regression Sep 26, 2026
@parse-github-assistant

Copy link
Copy Markdown

🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review.

Tip

  • Keep pull requests small. Large PRs will be rejected. Break complex features into smaller, incremental PRs.
  • Use Test Driven Development. Write failing tests before implementing functionality. Ensure tests pass.
  • Group code into logical blocks. Add a short comment before each block to explain its purpose.
  • We offer conceptual guidance. Coding is up to you. PRs must be merge-ready for human review.
  • Our review focuses on concept, not quality. PRs with code issues will be rejected. Use an AI agent.
  • Human review time is precious. Avoid review ping-pong. Inspect and test your AI-generated code.

Note

Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect.

Caution

Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code.

@kokokoXUY

Copy link
Copy Markdown
Author

Ready for review.

Scope: item 1 of #10681 only. The comparison now requires Math.abs(prValue - baseValue) >= 2 before the percentage thresholds decide the status, so the 0.52 ms → 0.66 ms (+26.2%) entry from #10656 no longer fails the job, while changes that clear the floor classify exactly as before.

Checked before submitting:

  • the workflow still parses as YAML after the edit;
  • with the values from Flaky and misconfigured CI checks block dependency PRs #10681 — the noise case (0.52 → 0.66, +26.9% by my arithmetic) moves from regression=true to regression=false, while 10 → 30 (+200%) and 30 → 10 (−66.7%) keep their previous classification.

What I could not check locally: the workflow run itself needs the benchmark artifacts from a previous run, so this is a logic-level check of the edited branch, not an end-to-end run.

Two points that are a maintainer's call:

  • 2 ms is the lower end of the range the issue suggests. It is one named constant if you prefer a different value, or a run-to-run variance measure instead.
  • The comparison logic is inline in the workflow heredoc and has no unit test today. Extracting it into a checked-in JavaScript module would make it testable; I am happy to do that in this PR if you want it.

Prepared with an AI coding agent and reviewed before submission.

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: d4c76f0f-a7df-4dac-ad07-fcb6dc167d83

📥 Commits

Reviewing files that changed from the base of the PR and between 82792be and 6a7d613.

📒 Files selected for processing (1)
  • .github/workflows/ci-performance.yml

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The CI performance workflow now requires benchmark differences to meet a 2 ms absolute threshold before it applies the existing relative status thresholds.

Changes

Benchmark comparison thresholds

Layer / File(s) Summary
Apply the absolute noise floor
.github/workflows/ci-performance.yml
The workflow checks that the absolute PR-minus-baseline difference is at least 2 ms before applying the existing relative thresholds. Smaller differences retain the default status.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 6a7d6

Benchmark changes under 2 ms no longer trigger status changes; larger changes retain the existing percentage thresholds. The implementation matches that noise-filtering goal, with no concrete merge risk identified.

Architecture Summary

Architecture risk: 🔵 Low · up to 6a7d6

The changed surface does not map to a changed system, dependency edge, entrypoint, or external dependency.

Changed systems: None identified.

Architecture concerns
No architecture-level concerns identified.

Review details

Before / after behavior

  • observed — Modified behavior in .github/workflows/ci-performance.yml: Adds a 2 ms absolute noise-floor constant for benchmark comparisons.
  • observed — Modified behavior in .github/workflows/ci-performance.yml: Calculates the signed absolute difference and requires its magnitude to be at least 2 ms before applying the existing >50%, >25%, and <−25% relative status thresholds. Changes below the floor remain at the default status.
🚥 Pre-merge checks | ✅ 7
✅ Passed checks (7 passed)
Check name Status Explanation
Title check ✅ Passed The title begins with the required ci: prefix and uses an uppercase first letter after the prefix. It accurately describes the benchmark noise-floor change.
Description check ✅ Passed The description includes all required template sections. It explains the issue, approach, verification results, limitation, and task status with sufficient detail.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Security Check ✅ Passed The pull request changes only the benchmark status calculation in .github/workflows/ci-performance.yml. The added code performs numeric subtraction and Math.abs comparison against a constant; it a…
Engage In Review Feedback ✅ Passed PASS: The supplied review metadata reports no CodeRabbit review threads and zero actionable findings in the current review. Therefore, no review feedback comment required engagement, implementation, o…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@mtrezza mtrezza closed this Sep 26, 2026
@mtrezza

mtrezza commented Sep 26, 2026

Copy link
Copy Markdown
Member

Rejected approach

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.

2 participants