-
Notifications
You must be signed in to change notification settings - Fork 60
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.
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
Add benchmarks to CI #2139
Add benchmarks to CI #2139
Conversation
a460a14
to
405a387
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would indeed do this only once it merges. That way, we ensure we ensure we don't clog up any PRs but still get a signal if the benches fail.
What's your view on limiting the number of TX benches? |
Benchmarks are only executed on a merge commit to main. Updated retention to 14 days, given the output is only 1.5MB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I think if we run them all only on a merge to master it should be fine. Its nice to have all of that data |
Resolves #1954
This is both a good and a bad idea. The good part is that we can frequently check incremental improvements and regressions. I currently set the criterion report retention to 3 days, but that might be too short.
The bad idea about this implementation is that it clogs up CI for ~20 minutes. And the results can be affected by other runs. We can opt to limit the number of tests (1 tx, 5 tx, 10 tx, 50 tx, 100 tx), make it run only as a triggerable action or only run it on merge commits.