-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
FusedIterator
marker trait and iter::Fuse
specialization
#1581
Merged
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
1fd4a15
`FusedIterator` marker trait and `iter::Fuse` specialization
Stebalien 4b24c5d
note associated type alternative
Stebalien 2f7a030
remove potentially confusing iffs
Stebalien 055c500
Fix type names in motivation/microbenchmark.
Stebalien c697b6a
Add unresolved question about removal of the Fuse::done unnecessary f…
Stebalien File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,247 @@ | ||
- Feature Name: fused | ||
- Start Date: 2016-04-15 | ||
- RFC PR: (leave this empty) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
Add a marker trait `FusedIterator` to `std::iter` and implement it on `Fuse<I>` and | ||
applicable iterators and adapters. By implementing `FusedIterator`, an iterator | ||
promises to behave as if `Iterator::fuse()` had been called on it (i.e. return | ||
`None` forever after returning `None` once). Then, specialize `Fuse<I>` to be a | ||
no-op iff `I` implements `FusedIterator`. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
Iterators are allowed to return whatever they want after returning `None` once. | ||
However, assuming that an iterator continues to return `None` can make | ||
implementing some algorithms/adapters easier. Therefore, `Fused` and | ||
`Iterator::fuse` exist. Unfortunately, the `Fused` iterator adapter introduces a | ||
noticeable overhead. Furthermore, many iterators (most if not all iterators in | ||
std) already act as if they were fused (this is considered to be the "polite" | ||
behavior). Therefore, it would be nice to be able to pay the `Fused` overhead | ||
iff necessary. | ||
|
||
Microbenchmarks: | ||
|
||
```text | ||
test fuse ... bench: 200 ns/iter (+/- 13) | ||
test fuse_fuse ... bench: 250 ns/iter (+/- 10) | ||
test myfuse ... bench: 48 ns/iter (+/- 4) | ||
test myfuse_myfuse ... bench: 48 ns/iter (+/- 3) | ||
test range ... bench: 48 ns/iter (+/- 2) | ||
``` | ||
|
||
```rust | ||
#![feature(test, specialization)] | ||
extern crate test; | ||
|
||
use std::ops::Range; | ||
|
||
#[derive(Clone, Debug)] | ||
#[must_use = "iterator adaptors are lazy and do nothing unless consumed"] | ||
pub struct MyFuse<I> { | ||
iter: I, | ||
done: bool | ||
} | ||
|
||
pub trait Fused: Iterator {} | ||
|
||
trait IterExt: Iterator + Sized { | ||
fn myfuse(self) -> MyFuse<Self> { | ||
MyFuse { | ||
iter: self, | ||
done: false, | ||
} | ||
} | ||
} | ||
|
||
impl<I> Fused for MyFuse<I> where MyFuse<I>: Iterator {} | ||
impl<T> Fused for Range<T> where Range<T>: Iterator {} | ||
|
||
impl<T: Iterator> IterExt for T {} | ||
|
||
impl<I> Iterator for MyFuse<I> where I: Iterator { | ||
type Item = <I as Iterator>::Item; | ||
|
||
#[inline] | ||
default fn next(&mut self) -> Option<<I as Iterator>::Item> { | ||
if self.done { | ||
None | ||
} else { | ||
let next = self.iter.next(); | ||
self.done = next.is_none(); | ||
next | ||
} | ||
} | ||
} | ||
|
||
impl<I> Iterator for MyFuse<I> where I: Iterator + Fused { | ||
#[inline] | ||
fn next(&mut self) -> Option<<I as Iterator>::Item> { | ||
self.iter.next() | ||
} | ||
} | ||
|
||
impl<I> ExactSizeIterator for MyFuse<I> where I: ExactSizeIterator {} | ||
|
||
#[bench] | ||
fn myfuse(b: &mut test::Bencher) { | ||
b.iter(|| { | ||
for i in (0..100).myfuse() { | ||
test::black_box(i); | ||
} | ||
}) | ||
} | ||
|
||
#[bench] | ||
fn myfuse_myfuse(b: &mut test::Bencher) { | ||
b.iter(|| { | ||
for i in (0..100).myfuse().myfuse() { | ||
test::black_box(i); | ||
} | ||
}); | ||
} | ||
|
||
|
||
#[bench] | ||
fn fuse(b: &mut test::Bencher) { | ||
b.iter(|| { | ||
for i in (0..100).fuse() { | ||
test::black_box(i); | ||
} | ||
}) | ||
} | ||
|
||
#[bench] | ||
fn fuse_fuse(b: &mut test::Bencher) { | ||
b.iter(|| { | ||
for i in (0..100).fuse().fuse() { | ||
test::black_box(i); | ||
} | ||
}); | ||
} | ||
|
||
#[bench] | ||
fn range(b: &mut test::Bencher) { | ||
b.iter(|| { | ||
for i in (0..100) { | ||
test::black_box(i); | ||
} | ||
}) | ||
} | ||
``` | ||
|
||
# Detailed Design | ||
[design]: #detailed-design | ||
|
||
``` | ||
trait FusedIterator: Iterator {} | ||
|
||
impl<I: Iterator> FusedIterator for Fuse<I> {} | ||
|
||
impl<A> FusedIterator for Range<A> {} | ||
// ...and for most std/core iterators... | ||
|
||
|
||
// Existing implementation of Fuse repeated for convenience | ||
pub struct Fuse<I> { | ||
iterator: I, | ||
done: bool, | ||
} | ||
|
||
impl<I> Iterator for Fuse<I> where I: Iterator { | ||
type Item = I::Item; | ||
|
||
#[inline] | ||
fn next(&mut self) -> Self::Item { | ||
if self.done { | ||
None | ||
} else { | ||
let next = self.iterator.next(); | ||
self.done = next.is_none(); | ||
next | ||
} | ||
} | ||
} | ||
|
||
// Then, specialize Fuse... | ||
impl<I> Iterator for Fuse<I> where I: FusedIterator { | ||
type Item = I::Item; | ||
|
||
#[inline] | ||
fn next(&mut self) -> Self::Item { | ||
// Ignore the done flag and pass through. | ||
// Note: this means that the done flag should *never* be exposed to the | ||
// user. | ||
self.iterator.next() | ||
} | ||
} | ||
|
||
``` | ||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
1. Yet another special iterator trait. | ||
2. There is a useless done flag on no-op `Fuse` adapters. | ||
3. Fuse isn't used very often anyways. However, I would argue that it should be | ||
used more often and people are just playing fast and loose. I'm hoping that | ||
making `Fuse` free when unneeded will encourage people to use it when they should. | ||
|
||
# Alternatives | ||
|
||
## Do Nothing | ||
|
||
Just pay the overhead on the rare occasions when fused is actually used. | ||
|
||
## Associated Type | ||
|
||
Use an associated type (and set it to `Self` for iterators that already provide | ||
the fused guarantee) and an `IntoFused` trait: | ||
|
||
```rust | ||
#![feature(specialization)] | ||
use std::iter::Fuse; | ||
|
||
trait FusedIterator: Iterator {} | ||
|
||
trait IntoFused: Iterator + Sized { | ||
type Fused: Iterator<Item = Self::Item>; | ||
fn into_fused(self) -> Self::Fused; | ||
} | ||
|
||
impl<T> IntoFused for T where T: Iterator { | ||
default type Fused = Fuse<Self>; | ||
default fn into_fused(self) -> Self::Fused { | ||
// Currently complains about a mismatched type but I think that's a | ||
// specialization bug. | ||
self.fuse() | ||
} | ||
} | ||
|
||
impl<T> IntoFused for T where T: FusedIterator { | ||
type Fused = Self; | ||
|
||
fn into_fused(self) -> Self::Fused { | ||
self | ||
} | ||
} | ||
``` | ||
|
||
For now, this doesn't actually compile because rust believes that the associated | ||
type `Fused` could be specialized independent of the `into_fuse` function. | ||
|
||
While this method gets rid of memory overhead of a no-op `Fuse` wrapper, it adds | ||
complexity, needs to be implemented as a separate trait (because adding | ||
associated types is a breaking change), and can't be used to optimize the | ||
iterators returned from `Iterator::fuse` (users would *have* to call | ||
`IntoFused::into_fused`). | ||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
Should this trait be unsafe? I can't think of any way generic unsafe code could | ||
end up relying on the guarantees of `Fused`. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/iff necessary/if possible/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I meant "if and only if necessary". Did you read an extra "not" into that sentence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I indeed did.
That being said,
iff
is equivalent to logic-↔ and“nice to be able to pay the
Fused
overhead” → “necessary” ∧“necessary” → “nice to be able to pay the
Fused
overhead”doesn’t really look right to me. /me shrugs.
EDIT: I propose “only when necessary” as replacement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. FYI, when writing that sentence, I didn't mean to include the "nice to be able to" in the iff. That is, I meant "pay the overhead" → "necessary to pay the overhead" ∧ "necessary to pay the overhead" -> "pay the overhead". However, I guess the second part is implied.