Skip to content
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

Annotate all core iterators with their fusedness #34343

Closed
wants to merge 1 commit into from

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Jun 18, 2016

For almost all of these iterators the fusedness is straightforward and
unlikely to change (e.g. making a memory-safe slice iterator that is not
fused is actually a hard thing), but there is exactly one exception:
Chain. The extension to make it fused is trivial and shouldn't have
any performance implications.

For almost all of these iterators the fusedness is straightforward and
unlikely to change (e.g. making a memory-safe slice iterator that is not
fused is actually a hard thing), but there is exactly one exception:
`Chain`. The extension to make it fused is trivial and shouldn't have
any performance implications.
@rust-highfive
Copy link
Collaborator

r? @aturon

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

@Stebalien
Copy link
Contributor

Related to #32999 and rust-lang/rfcs#1581.

@steveklabnik
Copy link
Member

I find "fusedness" to be a bit confusing here.

@tbu-
Copy link
Contributor Author

tbu- commented Jun 19, 2016

It's only in the commit description. I don't know a proper word for it.

@steveklabnik
Copy link
Member

Even in the text, "this iterator is fused." We don't talk about what "fused" means anywhere in the docs that I'm aware of.

@tbu-
Copy link
Contributor Author

tbu- commented Jun 19, 2016

Ah, you mean the word in general. The motivation for this word was the fuse function on the Iterator trait. The itertools crate also uses the terminology.

I'm not particularly attached to this naming. :) It seems at least two people came up with it independently, I haven't seen @Stebalien's RFC and pull request before.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 19, 2016
@alexcrichton
Copy link
Member

We may want to be careful when doing this as it could possibly put us in an uncomfortable position in the future if we were to want to not have an iterator be fused for something like performance reasons. I feel like docs like this have come up in the past and I forget what we decided.

Historically though we may have always qualified this with "The current implementation (not guaranteed) ...", but that seems not too useful.

@tbu-
Copy link
Contributor Author

tbu- commented Jun 19, 2016

I went through each of the iterators, and except for the case noted (Chain), there's only one trivial memory-safe variant to implement it. Just going over the iterators can make clear that nothing will change there, some examples:

  • slice iterators: I can think of no way to produce another element after the slice.
  • IntoIter iterators: Returning a non-None element requires conjuring up an element out of thin air, basically no way to do that.
  • endless iterators: These are trivially fused.
  • Map iterator: This kind of iterator is trivially fused if the contained iterator is.

etc.

@aturon
Copy link
Member

aturon commented Jun 21, 2016

I have mixed feelings about this change -- in general, I feel like code using iterators should be robust against non-fused iterators, because of the potential for refactorings that change the underlying iterator to a non-fused one.

How common is it to write code that relies on fusedness anyway?

I could imagine instead recommending using explicit fusedness when you need to rely on it -- and then using specialization to ensure that there's no performance penalty on already-fused iterators. That would then make this all a true implementation detail, and provide a more explicit way to state the guarantees you want to rely on.

@tbu-
Copy link
Contributor Author

tbu- commented Jun 21, 2016

@aturon If the only concern is refactoring, then please look at the iterators. These are all simple iterators in the core crate, and I'm stating that "nothing will change here" in the future.

Just take the slice iterator for example: Can you give me any way to turn it into a non-fused iterator?

Fusedness is important for some stuff, e.g. the dropping method of Itertools, which is the eager version of skip in the standard library, or for continuing to use an iterator after a for loop which may or may not have been interrupted.

@aturon
Copy link
Member

aturon commented Jun 21, 2016

@tbu- I don't mean refactoring in std itself (I agree that we can probably reach a fair degree of certainty there in at least some cases), but rather in the code using an iterator. That is, you might start out by using one simple iterator from std, and then later realize that you need to do something more complicated, and accidentally move away from being fused.

What do you think about the idea of making fuse "zero cost" via specialization, so that it doesn't add any overhead to iterators that are already fused? Then clients can safely always use fuse to make a clear, explicit semantic requirement, without worrying about any costs.

@tbu-
Copy link
Contributor Author

tbu- commented Jun 22, 2016

@aturon I like that, we should definitely do that.

However, I don't think we can nudge people in the direction of using Fuse<Iter> instead of Iter for a few reasons:

  1. Convenience: It works without fuse so people will use it.
  2. Fuse hides the interior iterator, you can no longer call methods on it, e.g. as_slice on slice iterators.
  3. It changes the type, so you can't do it when the iterator is embedded in a struct you don't control.

For these reasons, I think it'd be useful to make the guarantees we can make, that people will rely on anyway, and which we won't change anyway.
I still think we should make fuse zero-cost wherever we can, perhaps via a FusedIterator trait as @Stebalien proposed.

@brson
Copy link
Contributor

brson commented Jun 27, 2016

I agree that the jargon 'fused' is hard to interpret. Can the word be hyperlinked somewhere useful?

Alex also suggested just expanding what 'fused' means, like "once this iterator returns None it will continue to return None".

@steveklabnik
Copy link
Member

Maybe the term should be on the std::iter page, which can then be linked?

On Jun 27, 2016, 17:48 -0400, Brian Andersonnotifications@github.com, wrote:

I agree that the jargon 'fused' is hard to interpret. Can the word be hyperlinked somewhere useful?


You are receiving this because you commented.
Reply to this email directly,view it on GitHub(#34343 (comment)), ormute the thread(https://github.com/notifications/unsubscribe/AABsiiwd3iq4DPMhanBaMjwi_ycT3v3Dks5qQEVGgaJpZM4I48_u).

@alexcrichton
Copy link
Member

The libs team discussed this yesterday and @aturon will comment more to the conclusions (placing something in his inbox via this comment)

@bors
Copy link
Contributor

bors commented Jul 19, 2016

☔ The latest upstream changes (presumably #34898) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

Unfortunately @aturon's been pretty busy, but now that #35656 is open I'm going to close this in favor of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants