Skip to content

Add performance regression tests to CI #2548

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add performance regression tests to CI #2548

wants to merge 14 commits into from
+169 −21

Conversation

Shigoto-dev19
Copy link
Contributor

@Shigoto-dev19 Shigoto-dev19 commented Oct 14, 2025 •
edited
Loading

Closes #2545.
Depends on #2586.

How it works

The performance regression workflow in CI ensures that compile, prove, and verify times remain consistent across commits. It is driven by two workflows with a single source-of-truth baseline stored in the repo at tests/perf-regression/perf-regression.json.

  • PR checks (checks.yml): the matrix includes “Performance Regression”. The shared script run-ci-tests.sh runs in check mode (defaults to PERF_MODE=--check) and reads the baseline file directly from the repository. It then runs the benchmarks and compares current results to the stored values to detect regressions. No artifacts are downloaded, and no labels are consulted.

  • Baseline updates (dump-perf-baseline.yml): this is a separate, manual workflow (workflow_dispatch) that runs on the same branch. It executes the same performance suite in dump mode (PERF_MODE=--dump), regenerates perf-regression.json using GitHub runners, and **commits the updated file to the same branch. This becomes the canonical baseline for subsequent PR checks.

How to use

1) Open a PR (check mode)

  • The Checks workflow runs the Performance Regression matrix entry.
  • run-ci-tests.sh executes in --check, reading tests/perf-regression/perf-regression.json directly from the repo.
  • Outcome: The PR is validated against the committed baseline; no file changes occur.

2) Update the baseline (dump mode)

  • Trigger the manual workflow “Dump Performance Regression Baseline” (or run the script npm run regression:ci:dump-perf).
  • CI runs the performance suite in --dump on stable GitHub runners and **commits the refreshed perf-regression.json to the same branch.
  • Outcome: The baseline in the same branch is updated and used by all future PR checks.

Notes

  • The committed tests/perf-regression/perf-regression.json is the canonical baseline; PR checks read it directly.
  • There are no labels and no artifacts in this flow.
  • The first baseline must be seeded once (either via the dedicated dump workflow after merge, or via a one-off seed commit); See Add the very first performance regression baseline #2586.
  • Baselines should not be committed manually in normal operation; always update via the dump workflow to ensure consistent results from GitHub runners.

@Shigoto-dev19 Shigoto-dev19 force-pushed the shigoto/performance-regression-ci-tests branch from 206f511 to c0aeb4a Compare October 14, 2025 09:05
@Shigoto-dev19 Shigoto-dev19 force-pushed the shigoto/performance-regression-ci-tests branch from c0aeb4a to b0568fc Compare October 14, 2025 09:55
@Shigoto-dev19 Shigoto-dev19 force-pushed the shigoto/performance-regression-ci-tests branch from 955e6f5 to ee33eaa Compare October 14, 2025 16:50
@Shigoto-dev19 Shigoto-dev19 force-pushed the shigoto/performance-regression-ci-tests branch 3 times, most recently from 51b3fdb to f5f1909 Compare October 15, 2025 07:52
@Shigoto-dev19 Shigoto-dev19 force-pushed the shigoto/performance-regression-ci-tests branch 2 times, most recently from 17774b6 to c83ae06 Compare October 15, 2025 08:15
@Shigoto-dev19 Shigoto-dev19 force-pushed the shigoto/performance-regression-ci-tests branch from c83ae06 to e512d76 Compare October 15, 2025 08:57
@Shigoto-dev19 Shigoto-dev19 force-pushed the shigoto/performance-regression-ci-tests branch from b2ff575 to 437f588 Compare October 15, 2025 11:20
@Shigoto-dev19 Shigoto-dev19 force-pushed the shigoto/performance-regression-ci-tests branch from 437f588 to 4702ace Compare October 15, 2025 11:38
@Shigoto-dev19 Shigoto-dev19 marked this pull request as ready for review October 15, 2025 14:41
@Shigoto-dev19 Shigoto-dev19 requested review from a team as code owners October 15, 2025 14:41
bleepbloopsify
bleepbloopsify reviewed Oct 15, 2025
View reviewed changes
Copy link
Contributor

@bleepbloopsify bleepbloopsify left a comment

Choose a reason for hiding this comment

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

This actions workflow suffers from something I call "doing too much"

Actions philosophy usually goes something like this:
If you want to change how the action behaves, just use git to change what the underlying repository looks like. The action should follow "open-closed" principle, so most of the logic should live in your perf-regression.ts, rather than out here.

Adding a case to Build-and-Test-Server is a bit redundant if you introduce a separate workflow, but makes sense if you want to keep them under the same umbrella.

The pattern I expect to see here usually goes something like:

  1. Add test case to Build-and-Test-Server and run-ci-tests.sh
  2. run-ci-tests.sh calls your test or runs a bash script (check cache-regression case for an example

Your bash script should:

  1. do the download, and / or be responsible for the artifact location (looks like its tests/perf-regression/perf-regression.json).
  2. run the specific tests.

This way we don't have an action responsible for "Dump or Check", we have two locations where we simply "call" your script to allow it to be dumped properly.

suggestions:

  1. drop: this workflow. do not look for other JSON blobs that may or may not exist. only this copy of the repo exists as far as this script is concerned
  2. drop: a workflow that runs the perf-regression dumping (we have it in npm already). You might add one that runs the dump without actually dumping anything, just to make sure the flow works, but not required.
  3. procedure: if you want to update perf-regression, just run the script locally and check it into git
  4. procedure: run-ci-tests should just run the perf regression test, without having to introduce another git action step

question: how large are the perf regression JSONs? we might be able to upload them to GCP if they're quite large?

@Shigoto-dev19
Copy link
Contributor Author

question: how large are the perf regression JSONs? we might be able to upload them to GCP if they're quite large?

The final size is 1826 bytes.

@bleepbloopsify
Copy link
Contributor

question: how large are the perf regression JSONs? we might be able to upload them to GCP if they're quite large?

The final size is 1826 bytes.

well, never mind then, that is quite small

@Trivo25
Copy link
Member

Trivo25 commented Oct 16, 2025

This actions workflow suffers from something I call "doing too much"

Actions philosophy usually goes something like this: If you want to change how the action behaves, just use git to change what the underlying repository looks like. The action should follow "open-closed" principle, so most of the logic should live in your perf-regression.ts, rather than out here.

Adding a case to Build-and-Test-Server is a bit redundant if you introduce a separate workflow, but makes sense if you want to keep them under the same umbrella.

The pattern I expect to see here usually goes something like:

1. Add test case to `Build-and-Test-Server` and `run-ci-tests.sh`

2. `run-ci-tests.sh` calls your test or runs a bash script (check `cache-regression` case for an example

Your bash script should:

1. do the download, and / or be responsible for the artifact location (looks like its `tests/perf-regression/perf-regression.json`).

2. run the specific tests.

This way we don't have an action responsible for "Dump or Check", we have two locations where we simply "call" your script to allow it to be dumped properly.

suggestions:

1. drop: this workflow. do not look for other JSON blobs that may or may not exist. only this copy of the repo exists as far as this script is concerned

2. drop: a workflow that runs the perf-regression dumping (we have it in npm already). You might add one that runs the `dump` without actually dumping anything, just to make sure the flow works, but not required.

3. procedure: if you want to update `perf-regression`, just run the script locally and check it into git

4. procedure: run-ci-tests should just run the perf regression test, without having to introduce another git action step

question: how large are the perf regression JSONs? we might be able to upload them to GCP if they're quite large?

agree with the exception of

procedure: if you want to update perf-regression, just run the script locally and check it into git

because I think we will have to dump the data on the runners where we will also check the tests otherwise we might get too big of a variance

@Shigoto-dev19
Copy link
Contributor Author

agree with the exception of

procedure: if you want to update perf-regression, just run the script locally and check it into git

because I think we will have to dump the data on the runners where we will also check the tests otherwise we might get too big of a variance

I also agree with Florian, that's why I took the approach to do both dump and check in CI to eliminate notable variance from having different performance results run from different machines.

@bleepbloopsify
Copy link
Contributor

because I think we will have to dump the data on the runners where we will also check the tests otherwise we might get too big of a variance

ah of course, this makes sense

@Shigoto-dev19 Shigoto-dev19 mentioned this pull request Oct 20, 2025
Merged
@Shigoto-dev19 Shigoto-dev19 marked this pull request as draft October 20, 2025 10:39
@Shigoto-dev19 Shigoto-dev19 force-pushed the shigoto/performance-regression-ci-tests branch from 8b45c6d to 666c0c8 Compare October 20, 2025 19:05
@Shigoto-dev19 Shigoto-dev19 force-pushed the shigoto/performance-regression-ci-tests branch from 666c0c8 to cad4af9 Compare October 20, 2025 19:50
@Shigoto-dev19 Shigoto-dev19 marked this pull request as ready for review October 20, 2025 20:41
bleepbloopsify
bleepbloopsify approved these changes Oct 20, 2025
View reviewed changes
Copy link
Contributor

@bleepbloopsify bleepbloopsify left a comment

Choose a reason for hiding this comment

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

I like this much better! "It is CURRENT_REF" type 💩

I dislike gh as a dependency, but since it's already here I guess we can let it hang out a bit longer.

Small nit about a script that only calls one command, at that point do we really need a script?

otherwise, 🚢 it

tests/perf-regression/dump-perf-ci.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is interesting, but is there a reason we need it?

My understanding of GH actions is like this:

We trigger the action -> it calls the bash script

not:
we run the bash code -> it calls the gh workflow

CI is meant to make sure that the scripts we can run locally stays running, hence "Continuous" of "Continuous Integration"

request: delete this file, since all it does it call gh workflow run (with preset flags, but man gh should take care of that if necessary).

if it's meant to be manual, just direct people to the web interface and have them call the action manually (since workflow_dispatch is available as a trigger)

Copy link
Contributor

Choose a reason for hiding this comment

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

an anecdotal aside just to make sure I'm thinking about it the right way:

given the question: would you write a script to encode how we call git on our repository so everyone can do it the same way?

my answer would be: no, unless you need to chain multiple brittle git invocations in a row.

so in this case, I would probably only write a script for it if you need to run multiple git commands in a row.

I think this PR is fine without this script at all 📦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bleepbloopsify bleepbloopsify bleepbloopsify approved these changes

@ymekuria ymekuria Awaiting requested review from ymekuria ymekuria is a code owner automatically assigned from o1-labs/exploration-team

@Trivo25 Trivo25 Awaiting requested review from Trivo25

Assignees

No one assigned

Labels

no changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add performance regression tests to CI

3 participants

@Shigoto-dev19 @bleepbloopsify @Trivo25