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

Merged
merged 1 commit into from
Sep 19, 2025
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 48 additions & 4 deletions 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 @@ -3816,10 +3817,7 @@ pub trait Iterator {
}
}

match iter_compare(self, other.into_iter(), compare(eq)) {
ControlFlow::Continue(ord) => ord == Ordering::Equal,
ControlFlow::Break(()) => false,
}
SpecIterEq::spec_iter_eq(self, other.into_iter(), compare(eq))
}

/// Determines if the elements of this [`Iterator`] are not equal to those of
Expand Down Expand Up @@ -4038,6 +4036,42 @@ pub trait Iterator {
}
}

trait SpecIterEq<B: Iterator>: Iterator {
fn spec_iter_eq<F>(self, b: B, f: F) -> bool
where
F: FnMut(Self::Item, <B as Iterator>::Item) -> ControlFlow<()>;
}

impl<A: Iterator, B: Iterator> SpecIterEq<B> for A {
#[inline]
default fn spec_iter_eq<F>(self, b: B, f: F) -> bool
where
F: FnMut(Self::Item, <B as Iterator>::Item) -> ControlFlow<()>,
{
iter_eq(self, b, f)
}
}

impl<A: Iterator + TrustedLen, B: Iterator + TrustedLen> SpecIterEq<B> for A {
#[inline]
fn spec_iter_eq<F>(self, b: B, f: F) -> bool
where
F: FnMut(Self::Item, <B as Iterator>::Item) -> ControlFlow<()>,
{
// we *can't* short-circuit if:
match (self.size_hint(), b.size_hint()) {
// ... both iterators have the same length
((_, Some(a)), (_, Some(b))) if a == b => {}
// ... or both of them are longer than `usize::MAX` (i.e. have an unknown length).
((_, None), (_, None)) => {}
// otherwise, we can ascertain that they are unequal without actually comparing items
_ => return false,
}
Comment on lines +4061 to +4069
Copy link
Contributor

Choose a reason for hiding this comment

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

Just saw this in This Week in Rust and had a drive by question -- is this match construction equivalent to:

        if self.size_hint().1 != b.size_hint().1 {
            return false;
        }

? If so, is it worth changing? To my naive eyes, the latter generates different assembly. Not sure if it's meaningfully faster or anything, but it also looks like it might be simpler.

Additionally, I don't know how common the case is where someone calls eq() on two iterators where one's size_hint() is (usize::MAX, Some(usize::MAX)) and the other is (usize::MAX, None), but it would be "even more efficient" if we compared size_hint().0 != size_hint().0, even though we would specialize for fewer cases ⚖️

Anyway, just wondering if either of these would be helpful and figured the folks already on this thread would be the experts!😅

Copy link
Contributor Author

@yotamofek yotamofek Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Just saw this in This Week in Rust and had a drive by question -- is this match construction equivalent to:

        if self.size_hint().1 != b.size_hint().1 {
            return false;
        }

?

Yeah, you're absolutely right! I do think the match version is slightly easier to reason about, but yours is definitely more concise, and functionally the same.

If so, is it worth changing? To my naive eyes, the latter generates different assembly. Not sure if it's meaningfully faster or anything, but it also looks like it might be simpler.

Seems like if you lay out both functions in a way similar to how they're used here, you do get the (exact) same codegen:

https://godbolt.org/z/MTqboa4M1

So it's worth changing only if you think it improves readability, I don't think it'll have any effect on perf. I'm not an official member of any Rust team, just happen to be the PR's author, but you can definitely go ahead and open a PR with your suggestion, and if it makes sense to the libs team then that's absolutely fine by me, FWIW :)

Additionally, I don't know how common the case is where someone calls eq() on two iterators where one's size_hint() is (usize::MAX, Some(usize::MAX)) and the other is (usize::MAX, None), but it would be "even more efficient" if we compared size_hint().0 != size_hint().0, even though we would specialize for fewer cases ⚖️

TrustedLen's safety docs explicitly say that the trait's consumers must inspect the upper bound, but the wording is a little unclear so not sure if that's a strict requirement or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like if you lay out both functions in a way similar to how they're used here, you do get the (exact) same codegen:

https://godbolt.org/z/MTqboa4M1

Oh, good call! That's a clever catch and really surprising haha.

So it's worth changing only if you think it improves readability

Yeah, I find it slightly more readable, if for no other reason than the current match makes me ask, "It looks like we're inlining PartialEq for Option<T> -- I wonder why," but I'm also biased towards my own interpretations and don't feel strongly enough to go through the whole CI and review process!

TrustedLen's safety docs explicitly say that the trait's consumers must inspect the upper bound

Ah, yes, thanks for bringing that up! I did read that, but I totally forgot about it 😆 Thanks for helping me understand!


iter_eq(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 Expand Up @@ -4078,6 +4112,16 @@ where
}
}

#[inline]
fn iter_eq<A, B, F>(a: A, b: B, f: F) -> bool
where
A: Iterator,
B: Iterator,
F: FnMut(A::Item, B::Item) -> ControlFlow<()>,
{
iter_compare(a, b, f).continue_value().is_some_and(|ord| ord == Ordering::Equal)
}

/// Implements `Iterator` for mutable references to iterators, such as those produced by [`Iterator::by_ref`].
///
/// This implementation passes all method calls on to the original iterator.
Expand Down
Loading