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

Jump to bottom

Simplify some Iterator methods. #64572

Closed
wants to merge 1 commit into from
Closed

Simplify some Iterator methods. #64572

Show file tree
Hide file tree
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
6 changes: 5 additions & 1 deletion src/libcore/iter/adapters/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2404,7 +2404,11 @@ impl<I, T, E> Iterator for ResultShunt<'_, I, E>
type Item = T;

fn next(&mut self) -> Option<Self::Item> {
self.find(|_| true)
match self.iter.next() {
Some(Ok(v)) => Some(v),
Some(Err(e)) => { *self.error = Err(e); None },
None => None,
}
}

fn size_hint(&self) -> (usize, Option<usize>) {
Expand Down
53 changes: 21 additions & 32 deletions src/libcore/iter/traits/iterator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ pub trait Iterator {
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
fn nth(&mut self, mut n: usize) -> Option<Self::Item> {
for x in self {
while let Some(x) = self.next() {
Copy link
Member

@bluss bluss Sep 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is a good simplification

if n == 0 { return Some(x) }
n -= 1;
}
Expand Down Expand Up @@ -1855,18 +1855,15 @@ pub trait Iterator {
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
fn all<F>(&mut self, f: F) -> bool where
fn all<F>(&mut self, mut f: F) -> bool where
Copy link
Member

@bluss bluss Sep 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'd like to keep these Iterator::all etc simplifications, there are examples of where removing them would be a big regression. We have already put in the work to make sure we thread internal iteration methods through various adaptors correctly, so to remove the use of try_fold here at the top level would be a step back.

This change only partly rolls back what we have worked on for internal iteration — other entry points to it still remain, like sum, max, min, fold and try_fold. And those remaining happen to be the most useful in stable Rust (because implementing try_fold is not possible in stable, so custom iterators can't fully take advantage of this particular all magic). It would also be inconsistent to partly remove it, I think we should keep it all.

Here's an example with a microbenchmark of what kind of improvement it can be rust-itertools/itertools#348 In this example it was fold, but it will also be all when try_fold is stable.

I think we should go through fold/try_fold everywhere we can, for the structural improvements that gives (bypasses complicated state machines that composed iterators might have in .next())

Copy link
Member

Choose a reason for hiding this comment

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

Of course the code inside Iterator::all is not beautiful to read as it is now, and if it could be simplified while keeping the same features, that would be fantastic.

Self: Sized, F: FnMut(Self::Item) -> bool
{
#[inline]
fn check<T>(mut f: impl FnMut(T) -> bool) -> impl FnMut(T) -> LoopState<(), ()> {
move |x| {
if f(x) { LoopState::Continue(()) }
else { LoopState::Break(()) }
while let Some(x) = self.next() {
if !f(x) {
return false;
Copy link
Contributor

@tesuji tesuji Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

TIL that Iterator::all is that complex.
Here is the result from godbolt:https://godbolt.org/z/QlHZ0m

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'm inexperienced with godbolt, but the results seem to strongly support my simplification, i.e. the microbenchmark static instruction count drops from 29 to 9 and the cycle count drops from 735 to 260. Is that right? Is there anything else in there of note?

Copy link
Member

Choose a reason for hiding this comment

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

The MCA results are misleading here, because of the warning at the bottom of the output that "note: program counter updates are ignored" and thus it doesn't understand the loops. If you consider that the first one is 4x unrolled, the cycle difference is not unreasonable.

Copy link
Member

Choose a reason for hiding this comment

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

The code in that godbolt link isn't actually correct:

while let Some(&x) = a.iter().next() {

a.iter() is called on every loop iteration so it's only looking at the first element in the slice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. I updated the link: https://godbolt.org/z/QlHZ0m

}
}

self.try_for_each(check(f)) == LoopState::Continue(())
true
}

/// Tests if any element of the iterator matches a predicate.
Expand Down Expand Up @@ -1908,19 +1905,16 @@ pub trait Iterator {
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
fn any<F>(&mut self, f: F) -> bool where
fn any<F>(&mut self, mut f: F) -> bool where
Self: Sized,
F: FnMut(Self::Item) -> bool
{
#[inline]
fn check<T>(mut f: impl FnMut(T) -> bool) -> impl FnMut(T) -> LoopState<(), ()> {
move |x| {
if f(x) { LoopState::Break(()) }
else { LoopState::Continue(()) }
while let Some(x) = self.next() {
if f(x) {
return true;
}
}

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

/// Searches for an element of an iterator that satisfies a predicate.
Expand Down Expand Up @@ -1967,19 +1961,16 @@ pub trait Iterator {
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
fn find<P>(&mut self, predicate: P) -> Option<Self::Item> where
fn find<P>(&mut self, mut predicate: P) -> Option<Self::Item> where
Self: Sized,
P: FnMut(&Self::Item) -> bool,
{
#[inline]
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(()) }
while let Some(x) = self.next() {
if predicate(&x) {
return Some(x);
}
}

self.try_for_each(check(predicate)).break_value()
None
}

/// Applies function to the elements of iterator and returns
Expand All @@ -1999,19 +1990,17 @@ pub trait Iterator {
/// ```
#[inline]
#[stable(feature = "iterator_find_map", since = "1.30.0")]
fn find_map<B, F>(&mut self, f: F) -> Option<B> where
fn find_map<B, F>(&mut self, mut f: F) -> Option<B> where
Self: Sized,
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) {
Some(x) => LoopState::Break(x),
None => LoopState::Continue(()),
while let Some(x) = self.next() {
if let Some(y) = f(x) {
return Some(y);
}
}
None

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

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