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

Macro bench_specializations #786

Conversation

Philippe-Cholet
Copy link
Member

@Philippe-Cholet Philippe-Cholet commented Oct 20, 2023

Related to #766
Macro to be able to benchmark iterator methods.

Currently, there is only one example in the used macro (put none would lead to a compiler failure).
The expression $iterator is supposed to be fast, negligeable compared to the rest. Otherwise, we could use bencher.iter_batched(|| { $iterator }, |mut it| { ... }, ...).

@Philippe-Cholet Philippe-Cholet force-pushed the benchmark-specializations branch 2 times, most recently from 59baaf3 to 50961c6 Compare October 26, 2023 09:50
@Philippe-Cholet Philippe-Cholet marked this pull request as ready for review October 26, 2023 09:53
@Philippe-Cholet Philippe-Cholet force-pushed the benchmark-specializations branch from 50961c6 to 110a0aa Compare October 26, 2023 10:02
Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Philippe-Cholet, thanks for this. I'm inclined to merge this, but I have some questions:

  • There's $iterator and $first_its. When do we use which? Could we unify (and, thus, potentially simplify code)?
  • (Not directly related to this PR:) What's our general benchmarking story? Are benchmarks to be run within our CI or before we cargo publish a new version to ensure we have no regressions?

Please let me know if I you want to amend something or if I should merge this right away.

black_box(x);
})
}));
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also incllude all (such as the specialization tests)?

Copy link
Member Author

@Philippe-Cholet Philippe-Cholet Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I listed the currently missing (stable) methods:

Iterator:

  • Depends on fold by default: for_each partition reduce max min max_by_key max_by min_by_key min_by.
  • Depends on try_fold by default: all any find find_map position cmp partial_cmp eq ne lt le gt ge.
  • Depends on try_rfold by default: rposition.

DoubleEndedIterator:

  • Depends on try_rfold by default: rfind.

I'm inclined to not add them at the moment as they rely on one of [try_][r]fold.
Obviously, we can change our mind later.

Comment on lines +122 to +169
DoubleEndedIterator
ExactSizeIterator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat idea! Maybe we should do this in specialization tests, too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it too.
ExactSizeIterator might be unnecessary since we are I think unlikely to specialize len but it was easy to add once DoubleEndedIterator was added.

benches/specializations.rs Show resolved Hide resolved
@Philippe-Cholet
Copy link
Member Author

Philippe-Cholet commented Oct 26, 2023

@phimuemue
$first_its is an array of length 1000. The first item is $iterator, the second is roughly { let mut it = $iterator; it.nth(0); it } and so on (see bench_first_its for its real definition). It's meant to be immutably borrowed and is useful to benchmark size_hint (and the similar len) on not just a fresh iterator but on the first 1000 steps, it's useful because it does not measure nth calls as it would likely be way slower than size_hint. I agree it's weird at first sight.
$iterator is meant to be used in all other (most) cases. And I avoided any clone to not assume that $iterator is clonable.

I don't know what's "our general benchmarking story". I assume we simply use them to avoid performance regressions (and most likely real improvements) in pull requests like the recent and various fold specializations. @jswrenn might know more.

I changed the macro syntax a lot before I was satisfied. What do you think?

Before merging, I would like @jswrenn to give a little feedback on it as he wrote the initial issue and might have a different idea in mind?

Plus, I'm discovering a lot of "criterion" parts here. What do you think about my first comment (copied below)? I think it's decent to assume $iterator to be fast (as allocations should be done in the initialization block), right?

The expression $iterator is supposed to be fast, negligeable compared to the rest. Otherwise, we could use bencher.iter_batched(|| { $iterator }, |mut it| { ... }, ...).

@Philippe-Cholet
Copy link
Member Author

@phimuemue I'm also a bit unsure about () use in [r]fold specializations. Maybe the compiler would optimize it away in some cases?

@Philippe-Cholet Philippe-Cholet force-pushed the benchmark-specializations branch from 110a0aa to d1df7bf Compare October 27, 2023 09:28
@Philippe-Cholet
Copy link
Member Author

Now that there are no opened PR about "fold" specializations, I moved benchmarks of fold specializations out of "bench1.rs" to put them in the new "specializations.rs".
There are no other iterator method specializations in "bench1.rs".

I should probably take time to create other "specialization benchmarks" to avoid work in future PRs. But it will be another PR though.

@Philippe-Cholet
Copy link
Member Author

@jswrenn As you opened #766 (to improve benchmarking), I'd like to know if this is similar to what you had in mind.

Similar to `nth` benchmark.

With this, all iterator method specializations are considered except for methods that rely on `[r]fold/try_[r]fold` by default.
We might want to add others later.
@Philippe-Cholet Philippe-Cholet force-pushed the benchmark-specializations branch from e8fd17b to 25b2cd8 Compare November 11, 2023 13:20
@Philippe-Cholet Philippe-Cholet added this pull request to the merge queue Nov 13, 2023
Merged via the queue into rust-itertools:master with commit 9d84fe1 Nov 13, 2023
8 checks passed
@Philippe-Cholet Philippe-Cholet deleted the benchmark-specializations branch November 13, 2023 08:49
@jswrenn jswrenn mentioned this pull request Nov 14, 2023
@jswrenn jswrenn added this to the next milestone Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants