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.

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

use try_fold instead of try_for_each to reduce compile time #64885

Merged
merged 1 commit into from
Oct 2, 2019
Merged

use try_fold instead of try_for_each to reduce compile time #64885

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
27 changes: 14 additions & 13 deletions src/libcore/iter/traits/iterator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1859,14 +1859,13 @@ pub trait Iterator {
Self: Sized, F: FnMut(Self::Item) -> bool
{
#[inline]
fn check<T>(mut f: impl FnMut(T) -> bool) -> impl FnMut(T) -> LoopState<(), ()> {
move |x| {
fn check<T>(mut f: impl FnMut(T) -> bool) -> impl FnMut((), T) -> LoopState<(), ()> {
move |(), x| {
if f(x) { LoopState::Continue(()) }
else { LoopState::Break(()) }
}
}

self.try_for_each(check(f)) == LoopState::Continue(())
self.try_fold((), check(f)) == LoopState::Continue(())
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on equality check vs pattern matching here, can it have an effect or none at all?

Copy link
Contributor Author

@andjo403 andjo403 Oct 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

made a quick diff in godbolt and there is less code to inline so that is something that I can do
ZN72$LT$example..LoopState$LT$C$C$B$GT$$u20$as$u20$core..cmp..PartialEq$GT$2eq17h37dbcaf2df999e09E is a lot to inline

Copy link
Member

@scottmcm scottmcm Oct 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I would hope it has no effect, since LoopState<(),()> is an i1 in LLVM...

...and it is in -O, but very different in debug: https://rust.godbolt.org/z/LKOpZ7

Looks like the PartialEq::eq that gets generated is pretty bad, and it's still bad removing the generics: https://rust.godbolt.org/z/o6Nuaw Could there be a "this is a field-less enum so just compare the discriminants" path in the derive? It looks, unfortunately, like as u8 == 1 is the shortest-emitted-IR way to do these checks. And we're avoiding the derives in other places too, like

rust/src/libcore/cmp.rs

Lines 632 to 638 in 702b45e

#[stable(feature = "rust1", since = "1.0.0")]
impl Ord for Ordering {
#[inline]
fn cmp(&self, other: &Ordering) -> Ordering {
(*self as i32).cmp(&(*other as i32))
}
}

Copy link
Member

@bluss bluss Oct 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oh interesting. So we could improve here just by implementing PartialEq manually, or even adding a separate method for just discriminant comparison. But then pattern matching works well too. Like, just a method for ".is_continue()"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but the pattern match avoids having a function to inline completely as long as a function is used the llvm-ir will contain a call and a function

}

/// Tests if any element of the iterator matches a predicate.
Expand Down Expand Up @@ -1913,14 +1912,14 @@ pub trait Iterator {
F: FnMut(Self::Item) -> bool
{
#[inline]
fn check<T>(mut f: impl FnMut(T) -> bool) -> impl FnMut(T) -> LoopState<(), ()> {
move |x| {
fn check<T>(mut f: impl FnMut(T) -> bool) -> impl FnMut((), T) -> LoopState<(), ()> {
move |(), x| {
if f(x) { LoopState::Break(()) }
else { LoopState::Continue(()) }
}
}

self.try_for_each(check(f)) == LoopState::Break(())
self.try_fold((), check(f)) == LoopState::Break(())
}

/// Searches for an element of an iterator that satisfies a predicate.
Expand Down Expand Up @@ -1972,14 +1971,16 @@ pub trait Iterator {
P: FnMut(&Self::Item) -> bool,
{
#[inline]
fn check<T>(mut predicate: impl FnMut(&T) -> bool) -> impl FnMut(T) -> LoopState<(), T> {
move |x| {
fn check<T>(
mut predicate: impl FnMut(&T) -> bool
) -> impl FnMut((), T) -> LoopState<(), T> {
move |(), x| {
if predicate(&x) { LoopState::Break(x) }
else { LoopState::Continue(()) }
}
}

self.try_for_each(check(predicate)).break_value()
self.try_fold((), check(predicate)).break_value()
}

/// Applies function to the elements of iterator and returns
Expand All @@ -2004,14 +2005,14 @@ pub trait Iterator {
F: FnMut(Self::Item) -> Option<B>,
{
#[inline]
fn check<T, B>(mut f: impl FnMut(T) -> Option<B>) -> impl FnMut(T) -> LoopState<(), B> {
move |x| match f(x) {
fn check<T, B>(mut f: impl FnMut(T) -> Option<B>) -> impl FnMut((), T) -> LoopState<(), B> {
move |(), x| match f(x) {
Some(x) => LoopState::Break(x),
None => LoopState::Continue(()),
}
}

self.try_for_each(check(f)).break_value()
self.try_fold((), check(f)).break_value()
}

/// Searches for an element in an iterator, returning its index.
Expand Down