Convert to Criterion Benching

[michaelciraci]

Closes #159

[michaelciraci]

I know you want quick benchmarks. 100 ms is fairly small to me but I think we could go higher for slightly more accurate benchmarks

[ejmahler]

The old ones I think were on the order of 1s per benchmark, so we definitely have room to increase these.

[michaelciraci]

I ended up committing the Cargo.lock file because without it, the MSRV tests keep giving errors that criterion dependencies are too old for 1.77. Alternatively we could probably add a couple lines to the pipeline to force a specific version for 1.77, but I think just using a Cargo.lock file to pick exact versions would be more maintainable than a list of commands for specific versions in the dependency tree.

[ejmahler]

Another option is to pin those problematic crates to working versions in the cargo.toml file - that's what we did before raising the msrv. It lets us lock versions of just the crates that are a problem, while leaving the rest untouched.

[ejmahler]

Ah, I see you already did that with getrandom. If there are too many problems with criterion on 1.77, we could also just increase the msrv. What's the earliest rust version that would work?

[michaelciraci]

I feel like we shouldn't increase the MSRV just for a test dependency. I think the worst-case scenario is we make a separate benchmarking crate in this repo and just cd into that to run benchmarks.

[HEnquist]

Agree with just bumping the MSRV here. A low floor is nice when it's free to maintain, but its value is limited, and once a dev only dependency like criterion starts forcing pins or extra structure to keep it, that value doesn't justify the cost anymore.

[ejmahler]

Is there a way with this system of isolating individual benchmarks, or groups? For example, right now I could run cargo bench bench_power3_planned_scalar_f64 and it would isolate just the power3 f64 benchmarks. No need to run the f32 ones, or the power2 ones etc, if i'm specifically working on a power3 feature.

[michaelciraci]

You can still apply a filter. For that for example, you could do:

cargo bench --bench bench_check_scalar_2to1024 -- bench_power3_planned_scalar_f64

[HEnquist]

When anyway working on this, should we also update the names? For example benches/bench_check_scalar_2to1024.rs currently does 2 to 200, not 1024. Ideally to names that doesn't include the length :)

[michaelciraci]

When anyway working on this, should we also update the names? For example benches/bench_check_scalar_2to1024.rs currently does 2 to 200, not 1024. Ideally to names that doesn't include the length :)

Done

[ejmahler]

Since I was working on some performance stuff for #183, I decided to use that as a way to evaluate the new criterion bencher. My impression was positive. Thoughts I came away with:

• There was warning spam about deprecated features when running the benchmark. I noticed that we're on a quite old version of criterion. I think we should upgrade criterion all the way up to 0.8, which will requirea MSRV of 1.86. I was thinking we'll probably upgrade to this or past it anyways, since 1.86 is where target_feature 1.1 landed, which would let us eliminate some unsafe blocks.
• The group feature is really useful for the kind of benchmarking we do: https://bheisler.github.io/criterion.rs/book/user_guide/comparing_functions.html I found it to be a way better experience than what we've done up to this point, using the paste crate or just manual copy/paste + naming conventions to compare two different algorithms, or sclar vs sse etc.
  • Example usage: https://github.com/ejmahler/RustFFT/blob/large-cross-fft-experiment/benches/bench_rustfft_sse.rs#L370
• I found that I was able to get criterion to use the built in bencher's compact output format via the following:
  • cargo bench factor31 -- --output-format=bencher
  • But one of the non-bench binaries keeps trying to intercept the flags, and since the output-format flag is criterion-only, it throws an error and halts the benchmarking process.
  • Thus, you have to specify the specific benchmark binary to use:
  • cargo bench --bench bench_rustfft_scalar factor31 -- --output-format=bencher
  • This is fine i think. The compact output is nice because we have a few python scripts in the tools dir that we can use to quickly hack together things like log-log charts from benchmarks, and that script parses the bencher output. I see that criterion can also output a json file, although I haven't tried that yet.
• I haven't tried it yet, but has this whole process been updated for wasm simd?

After using it, I don't have any concerns about moving forward with this. @HEnquist Since you do a lot of profiling work, I'm curious to hear your thoughts, and I want to make sure this will cover your needs.

[michaelciraci]

I originally used an old criterion to minimize how much we had to bump the MSRV, but I just changed criterion to use 0.8 which will put the MSRV at 1.86.

[michaelciraci]

I haven't tried it yet, but has this whole process been updated for wasm simd?

I was able to run it with this command:

CARGO_TARGET_WASM32_WASIP1_RUNNER="wasmtime run --dir=." cargo bench --target wasm32-wasip1 --features wasm_simd --bench bench_rustfft_wasm_simd

[michaelciraci]

Also, one thought is if we bump the MSRV to 1.86, we might as well move to the 2024 edition.

I can make the change in this PR, but it will then cause cargo check to emit quite a few warnings which is going to make this PR touch many more files (although the changes will probably be trivial). I alternatively can make this change in a separate PR.

[HEnquist]

Looks like a clear improvement to me, no concerns. Getting off nightly for benches is worth it on its own, and the baseline save/compare will be useful for before-and-after on a kernel change.

No problem for the planner tuning in #182 either. It has its own timing harness in tools/planner_tuning, but that solves a different problem: it interleaves several candidate recipes for the same length within one run of one build, and takes the min. Criterion is the better tool for comparing before and after a change, which is the other half of what I do. So the two do not overlap.

[ejmahler]

I was able to run it with this command:

Excellent, that's even an improvement over the old workflow.

I can make the change in this PR, but it will then cause cargo check to emit quite a few warnings which is going to make this PR touch many more files (although the changes will probably be trivial). I alternatively can make this change in a separate PR.

Agreed that separate PR is the right approach.

[ejmahler]

Merged. Thanks for coming back to this 2 years after suggesting the change :D