chore: move to criterion benches from nightly#709
chore: move to criterion benches from nightly#709clarfonthey merged 3 commits intorust-lang:masterfrom
Conversation
|
@clarfonthey also mb it's a good idea to make loop bodies separate functions, so it doesn't cause duplication when adopted to gungraun. But I've tried to make changes minimal |
| extern crate test; | ||
|
|
||
| use test::{Bencher, black_box}; | ||
| #![expect(missing_docs)] // criterion_group! generates a public bench entrypoint |
There was a problem hiding this comment.
Until a better solution arises, I'm fine just adding this line to every dedicated test/bench module tbh. Right now we don't really have a way to say "missing docs on non-test items"
|
Honestly, there are plenty of things to criticise about the benchmarks, but since this is just strictly an improvement, I'm inclined to just merge this as-is so we can nitpick the actual benchmark code separately. But I'll wait a bit and take a fresh look for anything particularly egregious in a bit before merging. |
|
Re: "should we just check that everything compiles," I made a mistake when updating Benchmarking in CI is not only costly but also very susceptible to noise so I would recommend against it for statistical benches. It might be worth running a small gungraun benchmark on every target though, but we can visit that later. |
I mean Line 62 in e16b65c is triggered for every nightly test 
https://github.com/rust-lang/hashbrown/actions/runs/23953355374/job/69866090026 The question I wanted to ask is: should I preserve it, remove the run entirely, run only a single iteration, or do something else? Criterion takes much longer to run than the nightly bench. update: You can it here: https://github.com/rust-lang/hashbrown/actions/runs/23945600072/job/69841110628?pr=709 17 minutes vs 3 minutes before |
|
Oh, I had totally forgotten about those, and agree that we can replace them with just running clippy for now. Statistical tests don't really work well in CI due to all the noise. |
OK, so I'll edit ci.sh? |
|
Figured out how to push changes to the branch after much frustration, but that was the specific changes I was thinking of. (Since they're not included in the diff, I can't just suggest them there.) |
|
Do you want any other changes? I can port what we have now to Gangraun. Personally, I’m thinking of storing a JSON in the repo and having CI track whether the number has changed since the last run by more than some delta, plus a job that posts comments on PRs with diff and instructions for updating this JSON |
|
Honestly, was mostly waiting for you to notice the changes I made + take another look before merging. Stuff like porting to gungraun can happen in a second PR. |
|
So, the only thing that I think might be worth doing in this PR (and if you'd rather do it later, that's fine) is combining the benches into a single package; separating them into files is still fine. Since they can be filtered by name, it feels better to just combine them into one place, and it would simplify Right now, the benches otherwise don't seem to be well organised and it feels like it just increases friction to have them separated into different packages like this. (crates? packages? benches? the terminology is weird here since the individual benchmarks feel like benches, but according to cargo, the whole module is a separate bench.) |
And i was waiting for you :) I'd change the same |
I'll merge them |
|
@clarfonthey done |
2 participants
Shape of benches is mostly unchanged. Most of noise comes from switching from
#[bench]to passing c and usingc.bench_function.I've changed insert bench to allocate outside of the loop, can revert to prev behavior.
And I'm not sure about ci, should we only check that benches compile or run only on linux? Full suit takes a lot of time and without saving results it looks wastefull.
Follow-up to #707 (comment)