Benchmark examples - #1442
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1442 +/- ##
==========================================
- Coverage 89.67% 89.65% -0.02%
==========================================
Files 60 60
Lines 8565 8568 +3
Branches 8565 8568 +3
==========================================
+ Hits 7681 7682 +1
- Misses 561 562 +1
- Partials 323 324 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
After increasing the noise level and reducing the sample size to what, I think, are more sensible values for the examples - other functions might benefit from different values - I get the following, which seems nice. So this is ready to review: |
There was a problem hiding this comment.
Pull request overview
Adds Criterion-based benchmarking for the built-in CLI examples, introducing a bench feature to disable/relax certain one-time initialisation paths so examples can be executed repeatedly during benchmarking.
Changes:
- Add
benches/examples.rsCriterion benchmark that runs each built-in example via the CLI entrypoint. - Add a
benchCargo feature and[[bench]]target configuration, plus Criterion as a dev-dependency. - Gate a couple of “initialise once” behaviours behind
cfg!(feature = "bench")to support repeated runs (logging init + dangerous-options global flag).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/model/parameters.rs | Allows re-setting the dangerous-options OnceLock in bench builds (same-value only), to support repeated model loads in one process. |
| src/cli.rs | Skips logger initialisation in bench builds to avoid log I/O affecting benchmark runs. |
| Cargo.toml | Adds Criterion dev-dependency, declares examples bench target, and introduces bench feature. |
| Cargo.lock | Locks Criterion and transitive dependencies. |
| benches/examples.rs | Implements Criterion benchmarks for each example using handle_example_run_command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This reverts commit 59a391a.
ed329da to
ee7c648
Compare
|
Looks promising! Could you add something to the dev guide to explain how to use this, including how to generate reports? |
|
Looks really neat! I like the plots. Longer term, maybe we could even think about doing automated performance regression testing, so we'd get notified on PRs about it the same way we do about code coverage. It's slightly annoying that you have to enable an extra feature to detect whether it's being benchmarked, but the alternative suggestions I got from AI didn't work 🤷. Not the most important thing though. |
|
Check this: https://github.com/benchmark-action/github-action-benchmark It supports |
|
@tsmbland @alexdewar docs added, so this is ready for re-review. It would be good if your could check this locally, as well, to make sure all works as expected. |
|
@dalonsoa How do I see the comparison in the report? I ran |
Actually don't worry! Just cleared |
There was a problem hiding this comment.
This is really neat! I like how it has to warm up, like an old TV (do we need it to warm up before every example though?)
Way beyond the scope of this PR, but it would be nice to have a shorthand way to compare performance against main, without having to manually switch branches and run the benchmarking on each branch.
Also, some way to get running times for investment vs dispatch into the reports?
Co-authored-by: Tom Bland <t.bland@imperial.ac.uk>
|
I've no idea why we need the warm up time, but I'm sure that time can be tweaked. Open an issue about the comparison with main, so we don't forget and can tackle it in the future. It's a great idea. |
dc2917
left a comment
dc2917
left a comment
There was a problem hiding this comment.
This all looks good to me overall.
It does seems slightly odd that it still tries to run the test suite, just to report that they were all ignored.
It also seems odd that successive runs (i.e. no changes to the code) report performance increases/decreases - perhaps it's worth increasing the threshold?
|
It seems that there is a bit of a mess between benches and tests. I'm not entirely sure what should run with each command. See, for example: On the last bit, I guess it might depend on what is being run in the computer, in addition to the benches. I set a 5%, but it might be we want a higher value. Don't know what the expected variability might be just from noise. |
Ah OK, yeah, clearly a known issue. It's only a minor thing anyway.
Yes, I think you're right (first time I ran the benchmarks, compilation crashed a bunch of my running programs, so there wasn't much running during the tests; second time, I'd opened browser and Teams etc back up). So yeah, 5% seems entirely reasonable. |
Description
Add the benchmarking of the examples using Criterion. Some bits of the code had to be skip using
features, as they are meant to be run only once and when doing benchmarking, they were run many times.To run the example benchmarks do
cargo bench --features bench --bench examples. The--features benchis what skips the bits of the code we don't want.When doing this, you will get something like (you will see the comparison only if run more than once, so it has something to compare with).
I need to tweak the settings to remove the warnings and play a bit more with it - that's why this is a draft -, but this is the basic idea that we can roll to other critical functions that we want to optimise in the future.
It also produces nice reports:
Fixes #1443
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks