Skip to content

Optimise Iterator::{max, max_by, min, min_by}. #24180

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.

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
Apr 10, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
93 changes: 57 additions & 36 deletions src/libcore/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -743,12 +743,12 @@ pub trait Iterator {
#[stable(feature = "rust1", since = "1.0.0")]
fn max(self) -> Option<Self::Item> where Self: Sized, Self::Item: Ord
{
self.fold(None, |max, y| {
match max {
None => Some(y),
Some(x) => Some(cmp::max(x, y))
}
})
select_fold1(self,
|_| (),
// switch to y even if it is only equal, to preserve
// stability.
|_, x, _, y| *x <= *y)
.map(|(_, x)| x)
}

/// Consumes the entire iterator to return the minimum element.
Expand All @@ -766,12 +766,12 @@ pub trait Iterator {
#[stable(feature = "rust1", since = "1.0.0")]
fn min(self) -> Option<Self::Item> where Self: Sized, Self::Item: Ord
{
self.fold(None, |min, y| {
match min {
None => Some(y),
Some(x) => Some(cmp::min(x, y))
}
})
select_fold1(self,
|_| (),
// only switch to y if it is strictly smaller, to
// preserve stability.
|_, x, _, y| *x > *y)
.map(|(_, x)| x)
}

/// `min_max` finds the minimum and maximum elements in the iterator.
Expand Down Expand Up @@ -869,21 +869,16 @@ pub trait Iterator {
#[inline]
#[unstable(feature = "core",
reason = "may want to produce an Ordering directly; see #15311")]
fn max_by<B: Ord, F>(self, mut f: F) -> Option<Self::Item> where
fn max_by<B: Ord, F>(self, f: F) -> Option<Self::Item> where
Self: Sized,
F: FnMut(&Self::Item) -> B,
{
self.fold(None, |max: Option<(Self::Item, B)>, y| {
let y_val = f(&y);
match max {
None => Some((y, y_val)),
Some((x, x_val)) => if y_val >= x_val {
Some((y, y_val))
} else {
Some((x, x_val))
}
}
}).map(|(x, _)| x)
select_fold1(self,
f,
// switch to y even if it is only equal, to preserve
// stability.
|x_p, _, y_p, _| x_p <= y_p)
.map(|(_, x)| x)
}

/// Return the element that gives the minimum value from the
Expand All @@ -903,21 +898,16 @@ pub trait Iterator {
#[inline]
#[unstable(feature = "core",
reason = "may want to produce an Ordering directly; see #15311")]
fn min_by<B: Ord, F>(self, mut f: F) -> Option<Self::Item> where
fn min_by<B: Ord, F>(self, f: F) -> Option<Self::Item> where
Self: Sized,
F: FnMut(&Self::Item) -> B,
{
self.fold(None, |min: Option<(Self::Item, B)>, y| {
let y_val = f(&y);
match min {
None => Some((y, y_val)),
Some((x, x_val)) => if x_val <= y_val {
Some((x, x_val))
} else {
Some((y, y_val))
}
}
}).map(|(x, _)| x)
select_fold1(self,
f,
// only switch to y if it is strictly smaller, to
// preserve stability.
|x_p, _, y_p, _| x_p > y_p)
.map(|(_, x)| x)
}

/// Change the direction of the iterator
Expand Down Expand Up @@ -1024,6 +1014,37 @@ pub trait Iterator {
}
}

/// Select an element from an iterator based on the given projection
/// and "comparison" function.
///
/// This is an idiosyncratic helper to try to factor out the
/// commonalities of {max,min}{,_by}. In particular, this avoids
/// having to implement optimisations several times.
#[inline]
fn select_fold1<I,B, FProj, FCmp>(mut it: I,
mut f_proj: FProj,
mut f_cmp: FCmp) -> Option<(B, I::Item)>
where I: Iterator,
FProj: FnMut(&I::Item) -> B,
FCmp: FnMut(&B, &I::Item, &B, &I::Item) -> bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some reason the comparison function takes 4 arguments, when each client only wants 2 of them, instead of just making it take the projections and having max() / min() use |x| x for the projection function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree it would be nice but the lifetimes don't work out, B is disconnected from &I::Item. I think the fact we move sel and x around internally means that this approach can't work. :(

{
// start with the first element as our selection. This avoids
// having to use `Option`s inside the loop, translating to a
// sizeable performance gain (6x in one case).
it.next().map(|mut sel| {
let mut sel_p = f_proj(&sel);

for x in it {
let x_p = f_proj(&x);
if f_cmp(&sel_p, &sel, &x_p, &x) {
sel = x;
sel_p = x_p;
}
}
(sel_p, sel)
})
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<'a, I: Iterator + ?Sized> Iterator for &'a mut I {
type Item = I::Item;
Expand Down
31 changes: 31 additions & 0 deletions src/libcoretest/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -901,3 +901,34 @@ fn bench_multiple_take(b: &mut Bencher) {
}
});
}

fn scatter(x: i32) -> i32 { (x * 31) % 127 }

#[bench]
fn bench_max_by(b: &mut Bencher) {
b.iter(|| {
let it = 0..100;
it.max_by(|&x| scatter(x))
})
}

// http://www.reddit.com/r/rust/comments/31syce/using_iterators_to_find_the_index_of_the_min_or/
#[bench]
fn bench_max_by2(b: &mut Bencher) {
fn max_index_iter(array: &[i32]) -> usize {
array.iter().enumerate().max_by(|&(_, item)| item).unwrap().0
}

let mut data = vec![0i32; 1638];
data[514] = 9999;

b.iter(|| max_index_iter(&data));
}

#[bench]
fn bench_max(b: &mut Bencher) {
b.iter(|| {
let it = 0..100;
it.map(scatter).max()
})
}