make all_equal() faster #342
Conversation
|
Looks good! |
|
An alternative implementation could be: self.try_fold(None, |acc, x| acc.filter(|&prev| x != prev).xor(Some(Some(x)))) |
|
@axelf4 I haven't done any benchmarks, have you by any chance? I'd be worried that that would perform worse than this PR's implementation because of the extra branching in the closure body. But it's very hard to tell without benchmarks. |
| @@ -1310,9 +1310,13 @@ pub trait Itertools : Iterator { | |||
| /// assert!(data.into_iter().all_equal()); | |||
| /// ``` | |||
| fn all_equal(&mut self) -> bool | |||
| where Self::Item: PartialEq, | |||
| where Self: Sized, | |||
There was a problem hiding this comment.
I think requiring Sized is ok. Can anyone with more expertise confirm this is unproblematic?
There was a problem hiding this comment.
There are already lots of methods you can't call on an unsized iterator, so I don't think this is problematic.
There was a problem hiding this comment.
I'm a little mystified. The documentation for Iterator::all indicates that the only bound is F: FnMut(Self::Item) -> bool, but the implementation of Iterator::all also requires Self: Sized. Why is this bound elided from the documentation?
There was a problem hiding this comment.
I believe the documentation (sometimes?) leaves out Self: Sized bounds because they’re so common 😕
There was a problem hiding this comment.
Per Reddit, I think this is a bug. I've filed an issue: rust-lang/rust#62899.
| @@ -100,6 +100,7 @@ fn dedup() { | |||
|
|
|||
| #[test] | |||
| fn all_equal() { | |||
| assert!("".chars().all_equal()); | |||
There was a problem hiding this comment.
Good idea to include this corner case. Should we add the "one element" corner case, too?
There was a problem hiding this comment.
Yes, it will be a nice addtition. I have added it.
There was a problem hiding this comment.
Adding a quickcheck test would be a great alternative to either of those (something for another PR)
|
This should not need to require Self: Sized, and I'd prefer to rewrite it so that it is not a breaking change (in my opinion). I'm not sure I understand the short circuit argument - in what sense does this short circuit more than dedup does? They should both visit the same number of iterator elements. |
|
@bluss The |
|
Oh. We are long overdue for this improvement then. One major part of benchmark speed up could be that Looking at those upsides it seems ok to have a breaking change. I'm not sure in what context it would be a detectable breaking change, it should only change which implementation is picked when we call this method on a trait object. ... and it turns out the trait object question is moot, because trait object support has been broken in Itertools 0.8.0, That's something we could revisit, but let's only add back trait object support if we can find a good reason. Clearly there was no test for it. With that, I don't think adding |
|
If the trait object question is moot, I'm happy to merge this! bors r+ |
|
bors r+ |
342: make all_equal() faster r=jswrenn a=fyrchik Hello! This PR adresses #282 issue. Variant with `dedup` does not short circuit, but one with `all` does. I have also added some benchmarks and test for an empty iterator. ``` test all_equal ... bench: 999,832 ns/iter (+/- 217,245) test all_equal_default ... bench: 4,814,277 ns/iter (+/- 315,335) test all_equal_for ... bench: 2,096,174 ns/iter (+/- 165,596) ``` Let me know, what do you think. Co-authored-by: Evgenii <[email protected]>
Build succeeded |
Hello!
This PR adresses #282 issue. Variant with
dedupdoes not short circuit, but one withalldoes.I have also added some benchmarks and test for an empty iterator.
Let me know, what do you think.