Add UnboundedIterator Trait

[oberien]

The TrustedLen PR (tracking issue) is an effort to improve the performance of FromIterator / Extend code using specialization when working with iterators reporting a correct size_hint (i.e. are marked with the TrustedLen trait). Unfortunately, it does not work for infinite iterators, which a fixed number of elements are taken from. Instead, currently calling e.g. .take(n).collect::<Vec<_>>() on an unbounded iterator produces a loop adding one element at a time to the vector with capacity checks on each iteraton. The optimized implementation of vec!, which should be used here as well, just uses calloc as we'd expect when creating a zeroed vector.
Using any number different from zero results in the same loop with the iterator approach. The macro produces partially unrolled asm with SIMD instructions instead.

This PR introduces the unsafe UnboundedIterator trait, which marks infinite iterators. This marker is used for optimizations.

Its contract is described as the following:

An iterator that will never return None.

Any iterator implementing this trait will either continue to return
values infinitely, or diverge.
Additionally, its .size_hint must return (usize::MAX, None).

# Safety

This trait must only be implemented when the contract is upheld.

Improvements

• TrustedLen is implemented whenever .take(n) is called on an UnboundedIterator. This optimizes cases similar to repeat(x).take(n).collect().
• Calling .fuse() on an UnboundedIterator becomes a noop.
• Calling .cycle() on an UnboundedIterator becomes a noop.

Possible Future Improvements (TBD)

• Calling a consumer function on an unbounded iterator can produce a compile warning, as it most likely indicates a bug in the code. More on this in Unresolved Questions.
• With this PR, repeat(0).take(n).collect::<Vec<_>> allocates the size on the heap and uses memset to set the values to 0. This could be optimized to use __rustc_alloc_zeroed just as vec![0; n] is optimized to do.
• Another marker trait can be introduced, which marks iterators that return at least one value (first .next() call will return Some). (One problem here is code like let iter = _; iter.next(); iter.cycle(), as any call to .next() invalidates the property). This can be used for further improvements to Cycle as it can be an UnboundedIterator if the underlying iterator returns at least one value (currently it's only unbounded if the inner iterator is unbounded). Additionally, FlatMap with an outer iterator producing at least one element and an unbounded inner iterator can be unbounded as well.
• In an intermediate representation inside the rust compiler it should be possible to elide all None-branches from code handling return values of .next() calls on unbounded iterators. For now, I wasn't able to find a case where llvm wasn't able to do this already with static linking. With dynamic libraries, though, this is a possible optimization (if we trust the library file).

Unbounded Iterator Implementers

• "Generators": Repeat, RangeFrom
• Simple: Rev, Fuse, Map, Inspect, Zip, Enumerate, Peekable, Cloned, Box
• With specialization for size_hint: Filter, SkipWhile, FilterMap, StepBy, FlatMap, Skip
• Cycle: If the inner iterator is unbounded, T is in theory not required to be Clone. Unfortunately, removing it from the UnboundedIterator implementation results in conflicting implementation in the specialization trait implementation.
• Chain: In theory, a Chain with the first, second or both iterators being unbounded can be an UnboundedIterator. Unfortunately, afaik it's not possible to express this right now. I tried some auto trait hackery, but ran into Negative impls of Auto Traits (OIBIT) don't take into account Trait Bounds #46813 . Thus, currently it's only implemented if the first one is unbounded.

Alternatives

• Introduce size_hint2 on Iterator returning more information than size_hint, which can also indicate unbounded iterators. This will be a large public change, which will probably require an RFC first. Additionally, static analyses e.g. to warn on calling consumers on unbounded iterators might become harder. Optimizations would happen inside each function, which will match on the return value of size_hint2 instead of specialization, increasing the cyclomatic complexity of these functions.
• Instead of using an unsafe marker trait, a safe trait could be used with fn next(&mut self) -> Self::Item, which expresses the contract of UnboundedIterator in the type system.

Implementation Notes / Questions

As this is my first contribution to Rust, I'd like to evaluate on some of my design decisions and ask some questions. Also I'd highly appreciate any feedback on my code.

• Which feature gate should I use when implementing UnboundedIterator for StepBy, iterator_step_by or unbounded_iter? I decided to go with the former, as that was also done by FusedIterator.
• Is it currently possible to express the three possible combinations of trait bounds for Chain as described in Unbounded Iterator Implementers?
• Can the implementation of UnboundedIterator on Cycle remove the Clone trait bound as mentioned in Unbounded Iterator Implementers?
• Currently, to make .fuse() into a noop, I use FusedIterator as supertrait: trait UnboundedIterator: FusedIterator. While this results in the desired optimization, I'd rather like to express it using impl<I: UnboundedIterator> FusedIterator for I. Unfortunately, that conflicts with impl<'a, I: FusedIterator + ?Sized> FusedIterator for &'a mut I {}.
  Additionally, making FusedIterator a supertrait of UnboundedIterator requires Chain<A, B> to have both A fused and B fused due to the implementation of FusedIterator for Chain. Is there a better way to express that any UnboundedIterator is also a FusedIterator, which passes the compiler?
• I used a new specialization trait for structs requiring specialization of their size_hint method instead of directly specializing Iterator and UnboundedIterator because it currently breaks type inference: Specialization influnces inference #36262 (comment)

Unresolved Questions

1. Introducing UnboundedIterator allows us to detect some possibly infinite loops, namely calling consumer functions on them. For example repeat(0).count() will result in an infinite loop. I'd suggest that a compile warning should be produced in these cases. In most cases, this is a bug. And in the few cases where this is intended behaviour, #[allow(...)] can be used. Consuming methods are count, last, fold, all, max, min, max_by_key, max_by, min_by_key, min_by, sum and product. The functions collect and partition might fit here as well, but their contract does not actually state that they consume all elements. So there can be an implementation of FromIterator which only consumes a finite number of elements. Nonetheless, usually this points to a bug, so I think calling these two methods should still produce a warning.
   When discussing this in the Rust discord guild I got mixed feedback, which is why I'm adding this as part of the Unresolved Questions. What do you think about this?
   Also, in which layer would this change be done? As far as I understand, adding a new warning requires a new lint. Would it be possible to add this similar to #[deprecated] as an annotation which is added to the default implementations of these functions on UnboundedIterator?
2. Should TrustedLen be implemented for UnboundedIterators? Any UnboundedIterator does follow the contract, but does that implementation actually add anything useful?
3. Should every implementation of UnboundedIterator specialize the underlying struct's size_hint method to ensure returning (usize::MAX, None)? Currently types that don't need specialization do return this with their current implementation. But maybe with a later PR this might change. Specializing all of them better ensures the contract in case of changes not keeping in mind the UnboundedIterator contract.
4. Currently there is the constraint of size_hint needing to return (usize::MAX, None) for UnboundedIterator implementers. This is both because Take has a size_hint implementation, which works correctly if the underlying iterator returns these values, and because it makes sense. Should this constrait be removed and Take specialized instead?

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[kennytm]

The @rust-lang/libs team should be more familiar with this.

r? @alexcrichton

[kennytm]

Regarding repeat(0).count(), I think it's better implemented as a clippy lint instead of a built-in one.

[durka]

• Instead of using an unsafe marker trait, a safe trait could be used with fn next(&mut self) -> Self::Item, which expresses the contract of UnboundedIterator in the type system.

I really like this idea. unsafe trait is vague (basically just means "please read the docs") and type constraints are better than docs. Are there any problems with this alternative? One issue I can think of is it will be necessary to name the method something other than next (next_item?) to avoid name conflicts.

[oberien]

The main problem with a safe trait with next_item is that then there are two methods to get an item from an iterator. Being a safe trait, we can't ensure that user implementations of .next() and .next_item() will be the same (i.e. .next() is just doing Some(self.next_item()). Thus, in the following analysis we must assume that .next() may return None.

• To trigger all optimizations ensuring that the iterator is infinite, we'll need to specialize every function of Iterator on every implementor of that trait for unbounded iterators to use .next_item() instead of .next(). With an unsafe trait we can currently leave the None-elimination to llvm and later add it to rustc ifneedbe.
• Having two methods to theoretically do the same, but actually be allowed to produce different results feels like bad library design IMHO. If those implementations do differ, there may be hard-to-spot bugs.
• We can't implement FusedIterator for UnboundedIterator due to the conflicting implementation for &'a mut I. Instead, we need to specialize Fuse. (This is not really a problem, just requires a bit more code).

Concluding going with a safe trait is most likely possible (at least I haven't found any impossible problems just thinking about it). But having two different methods, which in theory should have the same implementation, but are allowed to have different implementations feels like a source for bugs.

[alexcrichton]

Thanks for the PR @oberien! This is a pretty weighty feature though so I think we'll be discussing this as a libs triage meeting at least.

In terms of motivation to start out with, you've mentioned take(n).collect() in the OP, but are there other compelling motivations for such a feature in libstd? Are there other common patterns which may be optimized better as well? Just want to make sure we fully understand the motivation before adding the feature!

[oberien]

My original motivation was that I wanted to show off Rust to a friend:
He was writing C# code for image manipulation, in which he allocated a buffer and wrote a constant into each element in a for-loop for initialization. Not having much experience with C# I asked him why he wasn't just using Enumerable.Repeat and the similar, as I'd write that code in rust as iter::repeat( 0xFFFFFF00).take(100000).collect(). He told me, that using enumerables has a high cost, as they do quite some fancy stuff behind the curtain.
So I wanted to show off how amazing Rust is with all of its zero-cost-abstractions and that the above code would produce assembler as optimized as it gets (as it's the case with vec![0xFFFFFF00; 100000]).
But godbolt taught me otherwise.

Thus, I analyzed the root cause of this issue and found out, that Rust does not "know" about infinite iterators. So I thought about "tagging" unbounded iterators and posted about it on reddit.

After digging deeper into a possible UnboundedIterator marker trait and implementing it, I realized that it can be used for further improvements, which I listed in this PR, like optimizing .cycle() and .fuse() to a noop.

tl;dr: While my original intend was to optimize the specific case of .take(n).collect(), an UnboundedIterator trait can be used for some further optimizations. Moreover, I think that having more information about Rust code may help later along the way to enable more optimizations or handle edge-cases better.

[alexcrichton]

Hm ok, so to me that sounds like most of this boils down to creating a vector. It makes sense to me that there's other possible optimizations such as cycle and fuse but (IMO at least) those are relatively niche and we're unlikely to get enough benefit from them to warrant an unsafe marker trait in the standard library.

Along those lines is it possible perhaps to achieve the same goal with vectors a different method? There's already a ton of specializations for creating vectors in via collection, and perhaps a special case is needed for an iterator like Take?

[oberien]

I don't think that this optimization is possible without marking unbounded iterators. Take can only be optimized with (or similar to) TrustedLen if the underlying iterator is unbounded. But deciding if the underlying iterator is unbounded can not easily be done without marking "generators", and transitively marking adaptors which are based on unbounded iterators.
I think it might be possible to optimize Vec for some special cases like Take<Repeat<T>>. But someone could write code like (0..).map(|i| i * i).take(10).collect() to get the first 10 square numbers, which would not be optimized if we don't manually implement that exact use case.
While we could just say "use vec![] instead of repeat(x).take(n).collect(), cases like the above can not be expressed differently without unsafe code (e.g. uninitialized) in an optimized way. But they could be optimized by llvm to produce a partially unrolled loop of SIMD instructions, writing 32 or more values at a time without bounds checks, which is currently not the case due to the missing unbounded iterator bound.

I do see the heaviness of this PR and in fact I was thinking about adding a drawback "Are these special-case optimizations worth that much code?". But I do think that this optimization is something people expect (at least I and a few friends do) and which would add to rusts promise of zero-cost abstractions.

[alexcrichton]

Ok we discussed this at @rust-lang/libs triage today and the conclusion was that we'd like to avoid the UnboundedIterator trait for now if possible. We didn't feel that it was motivated enough to add a whole new trait for this, but perhaps we could investigate other means of achieving the same goal?

[shepmaster]

#47370 may be related

[varkor]

As @shepmaster says, I think there could be other benefits of an UnboundedIterator trait in defining new semantics for infinite iterators (safely being able to call min/max on them without worrying about infinite loops, etc.), but it's probably a large enough change that it requires an RFC.

[oberien]

The problem this PR wants to tackle is optimizations around unbounded / infinite iterators. This implies that we somehow need to mark infinite iterators in order to deal with them. This marking is required to be done transitively over iterator adapters to catch all (most) cases (and not just manually implemented ones).

I decided to go with a new marker trait, because it's minimally invasive. It only changes cases which need to be touched and doesn't influence anything different. Thus, I think it's also the solution with the smallest code diff without changing existing stuff, because it's literally just marking unbounded iterators in libcore transitively.
Additionally, it allows specialization of adapters and consumers in the case of an underlying unbounded iterator.
Therefore, I don't think that I can achieve the proposed optimization(s) of this PR with less intrusion into libcore.

Coming back to my original suggestion using a new UnboundedIterator trait, the motivation increased with the latest discussions around infinite iterators in #47169 and #47370. The optimizations of #47370 are currently limited to the generators themselves and aren't transitively applied. With the UnboundedIterator trait, adapters can be specialized to call the underlying optimized functions instead of relying on the default implementations, which might end up producing an infinite loop. For example Chain.max uses the default implementation of Iterator, resulting in an infinite loop instead of applying the possible optimizations of #47370 (once it lands).

The first alternative is reducing the motivation. Instead of a new trait we could impl<I: TrustedLen> TrustedLen for Take<I>,impl TrustedLen for Repeat and RangeFrom. A specialization of Take::size_hint would be required to handle the (x, None) case correctly. This would enable the optimization of repeat(x).take(n).collect().
Advantages are that this is a very minimal change, in fact it's a diff of only +45-10. It also avoids creating a new trait.
Disadvantages are that only a few aggregates are supported, which might be enough for simple cases, but doesn't optimize more complex ones. Namely, Cycle, StepBy, Filter, FilterMap, Peekable, SkipWhile, Skip, FlatMap, Fuse and Inspect are not marked being infinite iterators when the inner is one. Moreover, as this alternative doesn't "mark" unbounded iterators, it doesn't allow specialization or special handling of unbounded iterators in any way. This includes not allowing any other optimization.

A different but similar approach is to tackle the same motivation without a new trait, but changing existing behaviour of other traits. The idea is to use TrustedLen to actually mark unbounded iterators. Currently the only guarantee if size_hint's upper bound is None is that more than usize::MAX elements are produced. We could add that (usize::MAX, None) is used iff the iterator will never return None. Any other (x, None) still indicates more than usize::MAX elements. In a first rough search through libcore's codebase I haven't found any iterator implementing TrustedLen which returns (usize::MAX, None), but I could be wrong.
Advantages are that this does in fact allow marking unbounded iterators without introducing a new trait.
Disadvantages are we still can't mark all the instances of unbounded iterators we're able to mark with a new trait. For example we're not able to implement TrustedLen on Skip, because we can't handle the case of (x, None) (we don't know how many more values than usize::MAX are returned and thus don't know how to adjust the size hint accordingly). Additionally, it changes the current definition of TrustedLen, which people might already rely on. Moreover, adding more different guarantees to TrustedLen means that code using it is required to handle more edge cases and thus use more branches. When optimizing functions for unbounded iterators, we always need to branch on the result of size_hint. Not only does this increase the cyclomatic complexity of code using TrustedLen, but it also moves the optimization using specialization inside rustc to llvm's dead code elimination.

Another idea is to increase the motivation and tackle a greater problem. My suggestion here would be to change the current approach of size_hint together with TrustedLen into a more versatile version. I've heared several people complain about the current limitations of size_hint, with which some cases can't be expressed. The idea is to introduce a new enum among the lines of

enum Size {
  Unknown,
  Bounded(usize, usize),
  BoundedLower(usize),
  LargerThanUsize,
  Unbounded,
}

Such a return value can be used for a lot of special cases. For example io::Bytes on a BufRead could return Unknown while reading and Bounded(x, x) once reading is finished but there's still data in the buffer.
That enum could be returned either from a new function Iterator::size_hint2 together with a TrustedLen2 trait, from a new unsafe fn Iterator::size, or from a new unsafe trait Size.

This change will allow way more widespread optimizations and possibilities for a lot of other kinds of iterators. But it'll be quite a large change and definitely require an RFC and further discussion.

In conclusion I think that we should definitely tackle unbounded iterators and possible optimizations surrounding them. I think that we should mark iterators to allow optimizations of any kind and also leave the option for future special case handling.
While the first suggestion fixes the main issue I had for which I started looking into unbounded iterators, it doesn't allow any further optimizations. And I think that those optimizations including making cycle and fuse into a noop on unbounded iterators and forwarding the optimizations of #47370 transitively are worth introducing code to mark infinite iterators.
I still like the original approach of a new marker trait most, because it allows specialization and is easier readable than the second suggestion which would instead introduce branches. If a new trait is still not motivated enough, I'd vote for the second suggestion introducing (usize::MAX, None) as marker for unbounded iterators as a complex compromise. The first alternative would also be ok to fix the immediate case of the collect optimization, but I'd still like to discuss further ideas how to handle unbounded iterators and their possible optimizations.

[scottmcm]

The optimizations of #47370 are currently limited to the generators themselves and aren't transitively applied. With the UnboundedIterator trait, adapters can be specialized to call the underlying optimized functions instead of relying on the default implementations, which might end up producing an infinite loop.

I think it's important to remember that these aren't optimizations, these are behaviour changes. Which, from my PoV at least, means they should only be done if they work reliably, consistently, and obviously on a clear class of things. And given the number of iterator adapters we have that take closures, there's definitely no way for that to happen even for the iterators we control, let alone all the iterators that exist in the wild or haven't yet been written.

Right now, every fold-like method on Iterator (aka that takes self and doesn't return an iterator) diverges on infinite iterators. That's consistent; that's unsurprising. I don't think that should change.

[oberien]

I think it's important to remember that these aren't optimizations, these are behaviour changes.

I didn't want to move the discussion around #47370 here. I simply wanted to state, that currently if these type of changes are implemented (be it through an RFC in the future, or #47533, or completely new ideas on unbounded iterators), they will only be available to their respective types themselves. They won't be available transitively through iterator adapters. In fact, there isn't currently no way to transitively forward these changes through some adapters for infinite iterators.
This is where this PR comes in, because it allows specialization of adapters for these exact cases.

And given the number of iterator adapters we have that take closures, there's definitely no way for that to happen even for the iterators we control, let alone all the iterators that exist in the wild or haven't yet been written.

With specialization, each adapter can be inspected individually, and only changed if all conditions and guarantees hold. For example, given #47533 lands, Cycle<Repeat>::nth would still call the default implementation of Iterator::nth and thus drop the possible optimization (disregarding that Cycle should be a noop anyway). On the other hand side, Filter<Repeat>::nth can't be optimized due to possible side-effects of the closure.

Right now, every fold-like method on Iterator (aka that takes self and doesn't return an iterator) diverges on infinite iterators. That's consistent; that's unsurprising. I don't think that should change.

That's not changed at all by this PR. On the contrary, I suggest to produce a compile warning when a consumer is called on an infinite iterator, as it currently produces an infinite loop, which most likely indicates a bug. @kennytm suggested to perform this as clippy lint, which would fit well IMO.

[oberien]

@alexcrichton What do you think about the alternatives presented 3 comments above?

[kennytm]

Ping from triage @alexcrichton!

Not sure if this should be kept open. There is also a response at #47082 (comment).

[alexcrichton]

@oberien hm sorry I think I'm getting a little lost now... I thought the motivation of this change was solely repeat(n).take().collect() into a vector? It seems a number of the referenced PRs/issues have been closed instead of merged, so is that still the primary motivation? You mentioned that this was possible without a new trait, could that be done instead?

[bluss]

Why not use TrustedLen for this? It is unstable, and it can be redesigned to accommodate this too.

We can't use (usize::MAX, None) because a chain: a.chain(b) that is longer than usize::MAX will use that size hint, even if it is not unbounded.

[oberien]

@alexcrichton The main reason why I dug into this topic of unbounded iterators was the repeat(x).take(n).collect() case. But the more I thought about this, the more I realized that handling infinite iterators differently / marking them allows for further optimizations. Examples are calling .fuse() or .cycle() on them. #47370 was closed because the change has too much side-effects and needs to go through the RFC process first. When it lands, it will only be a very small optimization for very local code. Marking unbounded iterators allows the optimizations to be applied transitively through the type system.
The exact case of repeat(x).take(n).collect() can be optimized with relatively little code, but most deviations wouldn't be covered, like repeat(x).zip(repeat(y)).take(n).collect().
Because I think that marking unbounded iterators has some benefits today, but allows for even further optimizations in the future, I decided to discuss different ways of marking them in #47082 (comment).

@bluss Good catch. I don't think that we are able to find any other variant of that tuple, which isn't taken yet, so maybe this idea will require changing the behaviour of Chain::size_hint, which wouldn't be preferable. Another option could be to add a method TrustedLen::is_unbounded for marking, to not influence size_hint.

[alexcrichton]

@oberien it seems that the discussion around unbounded iterators is much broader, so maybe this PR could be cut back to just dealing with the immediate vec creation problem?

[oberien]

I guess we should go with my first suggestion in #47082 (comment) then:

The first alternative is reducing the motivation. Instead of a new trait we could impl<I: TrustedLen> TrustedLen for Take<I>,impl TrustedLen for Repeat and RangeFrom. A specialization of Take::size_hint would be required to handle the (x, None) case correctly. This would enable the optimization of repeat(x).take(n).collect().
Advantages are that this is a very minimal change, in fact it's a diff of only +45-10. It also avoids creating a new trait.
Disadvantages are that only a few aggregates are supported, which might be enough for simple cases, but doesn't optimize more complex ones. Namely, Cycle, StepBy, Filter, FilterMap, Peekable, SkipWhile, Skip, FlatMap, Fuse and Inspect are not marked being infinite iterators when the inner is one. Moreover, as this alternative doesn't "mark" unbounded iterators, it doesn't allow specialization or special handling of unbounded iterators in any way. This includes not allowing any other optimization.

I implemented this behaiviour test-wise before creating that comment: oberien@806002e Is that genrally a change that you'd merge for enabling the immediate optimization? If that's the case I can go ahead closing this PR and creating a new one for that branch.

How should the discussion around unbounded iterators be continued? Or do you think that it doesn't add enough value in general for it to be continued?

[alexcrichton]

Ah yeah that patch looks good to me! Perhaps a thread on internals to discuss the infinite iterator issue?

[oberien]

Closing in favour of #47944