Specialise count, last and nth for Cloned and Map iterators

[nagisa]

Map/Cloned does not call the mapping/cloning function unnecessarily anymore for count, last and nth, which has a serious benefit to performance, especially with expensive mapping functions.

For example

(1..10).map(|x| {
    for y in 1..50 { black_box(y); }
    black_box(x)
}).last()

runs in 25 ns/iter (+/- 9) compared to 238 ns/iter (+/- 21) with nightly.

[rust-highfive]

r? @nikomatsakis

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

[nagisa]

Thinking now, this might need an RFC since it breaks people’s assumptions with their side-effectful Clone impls and mapping functions.

Please advise.

[bluss]

My idea has been that these can only be specialized if there is no user-visible difference from the default Iterator-trait provided implementation of last, nth, count. We can change, good to discuss explicitly.

[bluss]

I believe these specializations aren't of very high importance. Better support for bidirectional or random access would be bigger.

[ranma42]

Just nitpicking... is there a reason why last does not use the same syntax as next and nth? (i.e. self.iter.last().map(|a| (self.f)(a)))

[nagisa]

Because it doesn’t compile this way. self is by value here and is therefore partially moved by self.iter which does not allow capturing self into the closure.

[ranma42]

Oh, you're right, I did not realise that the signature was different... but then self.iter.last().map(self.f) should work just fine, right?

[nagisa]

Yeah, it works.

[nagisa]

I think they are important. If another method is added to Iterator in future releases, there could be incentive to make it run clone/mapping over all elements (because that’s what other functions do currently) even if not doing so would be very advantageous to performance of said method on Map-like iterators.

[alexcrichton]

I'm totally fine with this, pedantically this is a breaking change but practically I highly doubt that it will be. I'll tag this with T-libs so we can discuss but our conclusion may just be to clarify the documentation of the methods on the Iterator trait to indicate that the entire iterator may not be exhausted.

[bluss]

@alexcrichton It's not about breaking changes but about how we want iterators to behave

[bluss]

@alexcrichton It breaks existing code. Examples: (1), (2), (3), (4)

.count() is a method known to consume the iterator, with side effects. I think we should maintain it can only be specialized if the difference is invisible (for example: the slice iterator).

[nikomatsakis]

r? @alexcrichton

[nagisa]

I’m willing to limit the scope to Cloned only, as suggested by @bluss on IRC.

While it is still theoretically breaking, I think the impact should be non-existent in the real world.

[Stebalien]

Definitely not map. The last documentation even explicitly says "loops through the entire iterator". I'm hesitant about cloned but, if you do want to do that, you should change the documentation to specify that it only clones when necessary.

[ranma42]

If I am not mistaken, this would impact both side-effectful Clone and Drop implementations (it might be obvious to most devs, but the Drop part has not yet been mentioned so far).

Has there been any investigation on what implementations of the Iterator trait need an explicit specialisation for count, last and nth and which ones can be automatically optimised by LLVM?
This would be more fragile, because updates to the code generation logic and to LLVM might cause regressions, but we might try to make it more robust by extending the codegen tests to also verify that (when optimisations are enabled), rustc generates the desired code.

[Kimundi]

The count()-not-exhausting issue might be an argument for just giving in and adding an .drain() method to iterators that runs them to exhaustion, and is defined as such.

This would address the fair-to-common usecase where someone just wants to chain a method to run the iterator chain, instead of using the for syntax with a _ pattern, and where the best current workaround is calling count() and ignoring the result.

[alexcrichton]

Just as an update, the libs team has had quite a full agenda the past few weeks (dealing with FCP both ending and starting anew), but we'll be sure to get around to this next week!

[bluss]

@Kimundi I think an actual .foreach(f) is the strictly more versatile alternative; it can serve as both. It's requested all the time by, I guess, a section of programmers used to that kind of style from other languages. Either way, we can't change the semantics of .count().

[alexcrichton]

Ok, thanks for the patience here! The libs team talked about this and the conclusion was that these iterator adaptor methods should always preserve the same semantics as the current default implementations, specifically with respect to side effects that may be seen.

In light of that I'm going to close this as "unfortunately we can't do this", and otherwise I've opened a tracking issue for auditing those changes we've already made in the standard library.

[bluss]

Thank you, that sounds great for iterators.