Skip to content

Specialize Iterator::eq{_by} for TrustedLen iterators #137122

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Specialize Iterator::eq{_by} for TrustedLen iterators #137122

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4064a6b
Specialize `Iterator::{eq|cmp|partial_cmp}_by` for `TrustedLen` itera…
yotamofek Feb 16, 2025
d9f58d4
This optimization can't be applied to `{partial_}cmp{_by}`
yotamofek Feb 16, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion library/core/src/iter/traits/iterator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use super::super::{
Product, Rev, Scan, Skip, SkipWhile, StepBy, Sum, Take, TakeWhile, TrustedRandomAccessNoCoerce,
Zip, try_process,
};
use super::TrustedLen;
use crate::array;
use crate::cmp::{self, Ordering};
use crate::num::NonZero;
Expand Down Expand Up @@ -3762,7 +3763,7 @@ pub trait Iterator {
}
}

match iter_compare(self, other.into_iter(), compare(eq)) {
match SpecIterCompare::spec_iter_compare(self, other.into_iter(), compare(eq)) {
ControlFlow::Continue(ord) => ord == Ordering::Equal,
ControlFlow::Break(()) => false,
}
Expand Down Expand Up @@ -3984,6 +3985,41 @@ pub trait Iterator {
}
}

trait SpecIterCompare<B: Iterator>: Iterator {
fn spec_iter_compare<F, T>(self, b: B, f: F) -> ControlFlow<T, Ordering>
where
F: FnMut(Self::Item, B::Item) -> ControlFlow<T>;
}

impl<A: Iterator, B: Iterator> SpecIterCompare<B> for A {
#[inline]
default fn spec_iter_compare<F, T>(self, b: B, f: F) -> ControlFlow<T, Ordering>
where
F: FnMut(A::Item, <B as Iterator>::Item) -> ControlFlow<T>,
{
iter_compare(self, b, f)
}
}

impl<A: Iterator + TrustedLen, B: Iterator + TrustedLen> SpecIterCompare<B> for A {
#[inline]
fn spec_iter_compare<F, T>(self, b: B, f: F) -> ControlFlow<T, Ordering>
where
F: FnMut(Self::Item, <B as Iterator>::Item) -> ControlFlow<T>,
{
if let (_, Some(a)) = self.size_hint()
Copy link
Member

Choose a reason for hiding this comment

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

pondering: What if, instead of specialization, iter_compare just always checked if the size_hints? That way it can work even for things that are neither exact nor trusted.

Something with a size hint of (2, Some(10)) can't possibly be equal to one with (14, None), for example.

And for ESIs, which almost always return let len = self.len(); (len, Some(len)), the compiler can probably optimize the two checks into one. And for the default (0, None) hint the compiler will optimize away the check because obviously the other one can't be shorter than zero.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thing is - I'm not sure it would be acceptable if Iterator::eq started returning wrong results due to "wrong" size hints. It won't be UB, but I don't think any other iterator combinators can return wrong results because of a buggy size_hint(). IMHO even ESI's guarantee (which is less than TrustedLen's) might not be strong enough.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be equivalent to, say, for_each not being run for the last elements of an iterator if its upper bound hint is incorrectly smaller than the number of elements it can yield.
Copy link
Member

Choose a reason for hiding this comment

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

An incorrect size_hint is absolutely enough to trigger garbage-in-garbage-out. The default size_hint of (0, None) is always correct, so this would only be an issue if someone explicitly overrides it incorrectly, and that's not allowed.

Of course it's not allowed to be UB if someone implements size_hint wrong, but implementing size_hint wrong is no different from implementing fold wrong, for example: it's 100% allowed to make other things misbehave.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a really good point. Hadn't looked at it that way.
But I do think that as a user, I would be much more surprised if an incorrect size_hint implementation caused anything other than wrong estimations for with_capacity or something, rather than that an incorrect fold will just blow everything up. Maybe the word "hint" makes it sound less consequential. 🤷

Anyways, @the8472 's concern about eliding side effects might be a deal breaker, so I'll wait for the libs team decision on that before trying out different approaches for implementations.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, part of the problem is that, today, the size_hint is really only used by collect, which doesn't even use the upper part of it, just the bottom part.

I often wish there was instead just a suggested_reserve() -> usize or something that was more obviously both 1) just for the collect case, and 2) explicitly documented as allowed to be garbage.
&& let (_, Some(b)) = b.size_hint()
{
let ord = a.cmp(&b);
if ord != Ordering::Equal {
return ControlFlow::Continue(ord);
}
}

iter_compare(self, b, f)
}
}

/// Compares two iterators element-wise using the given function.
///
/// If `ControlFlow::Continue(())` is returned from the function, the comparison moves on to the next
Expand Down
Loading