Implement Iterator::fold for .chain(), .cloned(), .map() and the VecDeque iterators. #37315

bluss · 2016-10-20T22:52:50Z

Chain can do something interesting here where it passes on the fold
into its inner iterators.

The lets the underlying iterator's custom fold() be used, and skips the
regular chain logic in next.

Also implement .fold() specifically for .map() and .cloned() so that any
inner fold improvements are available through map and cloned.

The same way, a VecDeque iterator fold can be turned into two slice folds.

These changes lend the power of the slice iterator's loop codegen to
VecDeque, and to chains of slice iterators, and so on.
It's an improvement for .sum() and .product(), and other uses of fold.

rust-highfive · 2016-10-20T22:53:00Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2016-10-20T23:29:50Z

src/libcollections/vec_deque.rs

@@ -1829,6 +1811,42 @@ fn wrap_index(index: usize, size: usize) -> usize {
    index & (size - 1)
 }

+/// Returns the two slices that cover the VecDeque's valid range
+trait RingSlices : Sized {
+    fn slice(self, from: usize, to: usize) -> Self;


Could this be replaced with a bound on Index?

No, we're using either Index or IndexMut

cristicbz · 2016-10-21T15:07:28Z

Github won't let me comment on lines you didn't change. You could also modify the select_fold1_helper on line 2119 to something like:

fn select_fold1<I,B, FProj, FCmp>(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
{
    let mut projected = it.map(|x| (f_proj(&x), x));
    projected.next().map(|(sel_p, sel)| {
        projected.fold((sel_p, sel), |(sel_p, sel), (x_p, x)| {
            if f_cmp(&sel_p, &sel, &x_p, &x) {
                (x_p, x)
            } else {
                (sel_p, sel)
            }
        })
    })
}

This should help min and max when coupled with your map & fold improvements.

cristicbz · 2016-10-21T15:17:13Z

last could also be implemented as self.fold(None, |_, x| Some(x)) to take advantage of any potential special-casing of fold (though, of course, for Chain specifically there is an even better impl).

bluss · 2016-10-21T15:53:21Z

As said on irc, I'd prefer to have add-on improvements in a separate PR.

cristicbz · 2016-10-21T16:00:46Z

Makes sense, I'll create a spearate PR off my branch this gets merged. Very excited about this idea of using fold and a hypothetical fold_while as an iterator-provided internal iteration protocol in addition to the external protocol provided by next.

cristicbz · 2016-10-21T16:09:21Z

src/libcollections/vec_deque.rs

+    fn slice(self, from: usize, to: usize) -> Self;
+    fn split_at(self, i: usize) -> (Self, Self);
+
+    fn ring_slices(buf: Self, head: usize, tail: usize) -> (Self, Self) {


Maybe some #[inline]-s here and on the slice/split_at methods below would help ensure the generated code stays the same as before? (don't have any data that it doesn't and since code is generic, it'll always be available to inline)

Since it is generic it should not matter I think. It is sort of a higher level operation anyway.

cristicbz · 2016-10-23T13:12:50Z

Some further validation for this idea: I specialised the fold implementation of the hashmap RawTable's iterators in a similar fashion for some significant perf benefits in #37362.

bluss · 2016-10-23T13:19:44Z

Cool.

The improvement of .fold() on VecDeque enables unrolling and vectorization. I've used the same idea in ndarray, to much improve its generalized iterator the same way (it has a special case for contiguous data that was already good, but this helps the general cases by an order of magnitude). In ndarray this is not a fringe use case, instead it is used to improve the iteration of non-contiguous arrays across the board, improving in place arithmetic ops, array-to-array mapping and more operations for them.

The reasoning for this PR is

VecDeque's iterator .fold() is an order of magnitude improvement (see issue VecDeque's iterator could be optimized #30805 for the size of that)
Forwarding fold on map, cloned, map, flat_map, is important since that allows the improved versions of fold be used from the underlying iterators.

bluss · 2016-10-23T13:22:39Z

So while .chain() can be improved to optimize better (and I hope we will, I have some test cases for that), this PR is still relevant until we can make the VecDeque's iterator's .next() optimize better on its own, or ndarray's generalized iterator's .next() optimize better on its own and so on. Let the user's improved .fold() shine through.

alexcrichton · 2016-10-25T03:22:53Z

@bors: r+

Nice wins! Neat idea propagating through adapters like map and cloned

bors · 2016-10-25T03:22:54Z

📌 Commit f095f0d has been approved by alexcrichton

bors · 2016-10-25T11:08:19Z

⌛ Testing commit f095f0d with merge 8a2de11...

bors · 2016-10-25T11:22:13Z

💔 Test failed - auto-linux-64-debug-opt

bluss · 2016-10-25T12:32:37Z

Thank you. I'll have to investigate this llvm assertion..

bluss · 2016-10-25T13:49:34Z

I ruled out the VecDeque and cloned(), .map() changes, so it must be .chain() or .flat_map() (causing the assertion).

Implement .fold() specifically for .map() and .cloned() so that any inner fold improvements are available through map and cloned.

Chain can do something interesting here where it passes on the fold into its inner iterators. The lets the underlying iterator's custom fold() be used, and skips the regular chain logic in next.

bluss · 2016-10-25T20:22:59Z

I haven't solved it, but I have removed the FlatMap::fold method that caused the debuginfo-related llvm assertion when building librustc. It turned out to be the fold of something using flat_map here that caused it.

This way the other improvements can go in first. The remaining parts should not cause any llvm assertions.

alexcrichton · 2016-10-25T21:18:53Z

@bors: r+

bors · 2016-10-25T21:18:55Z

📌 Commit a16626f has been approved by alexcrichton

Implement Iterator::fold for .chain(), .cloned(), .map() and the VecDeque iterators. Chain can do something interesting here where it passes on the fold into its inner iterators. The lets the underlying iterator's custom fold() be used, and skips the regular chain logic in next. Also implement .fold() specifically for .map() and .cloned() so that any inner fold improvements are available through map and cloned. The same way, a VecDeque iterator fold can be turned into two slice folds. These changes lend the power of the slice iterator's loop codegen to VecDeque, and to chains and flat maps of slice iterators, and so on. It's an improvement for .sum() and .product(), and other uses of fold.

bors · 2016-10-26T18:43:33Z

⌛ Testing commit a16626f with merge 3a25b65...

Implement Iterator::fold for .chain(), .cloned(), .map() and the VecDeque iterators. Chain can do something interesting here where it passes on the fold into its inner iterators. The lets the underlying iterator's custom fold() be used, and skips the regular chain logic in next. Also implement .fold() specifically for .map() and .cloned() so that any inner fold improvements are available through map and cloned. The same way, a VecDeque iterator fold can be turned into two slice folds. These changes lend the power of the slice iterator's loop codegen to VecDeque, and to chains of slice iterators, and so on. It's an improvement for .sum() and .product(), and other uses of fold.

bors · 2016-10-26T21:58:14Z

rust-highfive assigned alexcrichton Oct 20, 2016

Mark-Simulacrum reviewed Oct 20, 2016

View reviewed changes

cristicbz reviewed Oct 21, 2016

View reviewed changes

bluss changed the title ~~Special case Iterator::fold for .chain(), .flat_map(), .cloned(), .map() and the VecDeque iterators.~~ Implement Iterator::fold for .chain(), .flat_map(), .cloned(), .map() and the VecDeque iterators. Oct 22, 2016

cristicbz mentioned this pull request Oct 23, 2016

Implement fold on hash::table::{Iter,IterMut}. #37362

Closed

bluss mentioned this pull request Oct 25, 2016

Debuginfo generation is broken with LLVM 3.8 #36774

Closed

bluss added 3 commits October 25, 2016 15:50

Special case .fold() for VecDeque's iterators

15a9586

iter: Implement .fold() for .cloned() and .map()

780acda

Implement .fold() specifically for .map() and .cloned() so that any inner fold improvements are available through map and cloned.

iter: Implement .fold() for .chain()

a16626f

Chain can do something interesting here where it passes on the fold into its inner iterators. The lets the underlying iterator's custom fold() be used, and skips the regular chain logic in next.

bluss force-pushed the fold-more branch from f095f0d to a16626f Compare October 25, 2016 20:17

bluss changed the title ~~Implement Iterator::fold for .chain(), .flat_map(), .cloned(), .map() and the VecDeque iterators.~~ Implement Iterator::fold for .chain(), .cloned(), .map() and the VecDeque iterators. Oct 25, 2016

Manishearth mentioned this pull request Oct 26, 2016

Rollup of 11 pull requests #37416

Closed

bors merged commit a16626f into rust-lang:master Oct 26, 2016

bluss deleted the fold-more branch October 26, 2016 21:59

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 30, 2016

This was referenced Nov 2, 2016

Iterator performance tips AtheMathmo/rulinalg#86

Open

LLVM assertion on debuginfo generation. #37633

Closed

bluss mentioned this pull request Dec 19, 2016

Add method fold_with to Producer and UnindexedProducer rayon-rs/rayon#184

Merged

ChristopherBiscardi mentioned this pull request Dec 9, 2023

replaced map().sum() with fold ChristopherBiscardi/advent-of-code#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Iterator::fold for .chain(), .cloned(), .map() and the VecDeque iterators. #37315

Implement Iterator::fold for .chain(), .cloned(), .map() and the VecDeque iterators. #37315

bluss commented Oct 20, 2016 •

edited

Loading

rust-highfive commented Oct 20, 2016

Mark-Simulacrum Oct 20, 2016

bluss Oct 20, 2016

cristicbz commented Oct 21, 2016 •

edited

Loading

cristicbz commented Oct 21, 2016

bluss commented Oct 21, 2016

cristicbz commented Oct 21, 2016

cristicbz Oct 21, 2016

bluss Oct 21, 2016

cristicbz commented Oct 23, 2016

bluss commented Oct 23, 2016 •

edited

Loading

bluss commented Oct 23, 2016

alexcrichton commented Oct 25, 2016

bors commented Oct 25, 2016

bors commented Oct 25, 2016

bors commented Oct 25, 2016

bluss commented Oct 25, 2016

bluss commented Oct 25, 2016 •

edited

Loading

bluss commented Oct 25, 2016 •

edited

Loading

alexcrichton commented Oct 25, 2016

bors commented Oct 25, 2016

bors commented Oct 26, 2016

bors commented Oct 26, 2016

Implement Iterator::fold for .chain(), .cloned(), .map() and the VecDeque iterators. #37315

Implement Iterator::fold for .chain(), .cloned(), .map() and the VecDeque iterators. #37315

Conversation

bluss commented Oct 20, 2016 • edited Loading

rust-highfive commented Oct 20, 2016

Mark-Simulacrum Oct 20, 2016

Choose a reason for hiding this comment

bluss Oct 20, 2016

Choose a reason for hiding this comment

cristicbz commented Oct 21, 2016 • edited Loading

cristicbz commented Oct 21, 2016

bluss commented Oct 21, 2016

cristicbz commented Oct 21, 2016

cristicbz Oct 21, 2016

Choose a reason for hiding this comment

bluss Oct 21, 2016

Choose a reason for hiding this comment

cristicbz commented Oct 23, 2016

bluss commented Oct 23, 2016 • edited Loading

bluss commented Oct 23, 2016

alexcrichton commented Oct 25, 2016

bors commented Oct 25, 2016

bors commented Oct 25, 2016

bors commented Oct 25, 2016

bluss commented Oct 25, 2016

bluss commented Oct 25, 2016 • edited Loading

bluss commented Oct 25, 2016 • edited Loading

alexcrichton commented Oct 25, 2016

bors commented Oct 25, 2016

bors commented Oct 26, 2016

bors commented Oct 26, 2016

bluss commented Oct 20, 2016 •

edited

Loading

cristicbz commented Oct 21, 2016 •

edited

Loading

bluss commented Oct 23, 2016 •

edited

Loading

bluss commented Oct 25, 2016 •

edited

Loading

bluss commented Oct 25, 2016 •

edited

Loading