Skip to content
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 Criterion::measurement function #369

Closed
wants to merge 4 commits into from
Closed

Conversation

oberien
Copy link

@oberien oberien commented Jan 8, 2020

I wanted to write a benchmark function generic over the measurement such that I can easily benchmark once using cycles and once using time (see #355). However, there was no way to generically perform the measurements, which is why I added this function. The clone might not necessarily be required, but otherwise lifetime foo would become complicated, and usually it's a unit-struct anyway.

See the example in the doc-comment for an example use-case.

@bheisler
Copy link
Owner

Huh, that's an interesting idea. I hadn't thought of that! One problem I can foresee is that you now have defined multiple benchmarks with the same name, and those will conflict.

@oberien
Copy link
Author

oberien commented Jan 18, 2020

The benchmark "iter" is already defined twice in different doc-comments, if that's what you mean (also "function_name" is used multiple times):

criterion.rs/src/lib.rs

Lines 312 to 316 in 72a27c4

/// fn bench(c: &mut Criterion) {
/// c.bench_function("iter", move |b| {
/// b.iter(|| foo())
/// });
/// }

criterion.rs/src/lib.rs

Lines 355 to 365 in 72a27c4

/// fn bench(c: &mut Criterion) {
/// c.bench_function("iter", move |b| {
/// b.iter_custom(|iters| {
/// let start = Instant::now();
/// for _i in 0..iters {
/// black_box(foo());
/// }
/// start.elapsed()
/// })
/// });
/// }

I've changed it anyways.

If instead you mean that calling the bench function multiple times in my example results in weird behaviour, the main reason to make Criterion generic for me is to have two different benchmarks (as in benchmark binaries) with different measurements, in which case it's also fine to use the same name. Otherwise one can pass the name as argument to that function.

@bheisler
Copy link
Owner

Hey, sorry I was unclear. What I meant was this:
c.bench_function("measurement", move |b| {
If you call this function with two or more different measurements, you've created two or more different benchmarks with the ID "measurement". The benchmark ID must be unique between all of the different benchmarks in all of the benchmark binaries in your project because it's used as part of the path to the directory that stores the results. If you reuse IDs, the benchmarks will overwrite each other's measurements and have other strange behavior. As far as I can tell, there is no reliable way for Criterion.rs to detect this to warn the user.

On the other hand, I suppose that if you're using iter_custom in the first place you're probably a more advanced user, so maybe it's OK. The documentation should warn about that and provide a correct example though.

@oberien
Copy link
Author

oberien commented Jan 28, 2020

I added a more complex example showcasing measurement using both WallTime and (stubbed) Cycles, which pass different names to the bench function. I hope this makes it clearer that different names should be used.

Maybe it would make sense to add a note to Criterion::bench_function and BenchmarkGroup::bench_function stating that names must be unique over all benchmarks in the project.

@lemmih
Copy link
Collaborator

lemmih commented Jul 26, 2021

This looks useful to me. Are you up for resolving the conflicts?

@lemmih lemmih added the Need Information Need further information from the submitter label Jul 26, 2021
@lemmih
Copy link
Collaborator

lemmih commented Dec 28, 2021

Closing due to inactivity.

@lemmih lemmih closed this Dec 28, 2021
@oberien
Copy link
Author

oberien commented Jan 12, 2022

Sorry for the late response. I've rebased my fork to the latest master.

@lemmih
Copy link
Collaborator

lemmih commented Jan 12, 2022

I can't reopen after you're rebased your branch. You'll have to create a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Need Information Need further information from the submitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants