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

Implement Fuse with Option #70366

Merged
merged 2 commits into from
Mar 25, 2020
Merged

Implement Fuse with Option #70366

merged 2 commits into from
Mar 25, 2020

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Mar 24, 2020

The former done flag was roughly similar to an Option tag, but left
the possibity of misuse. By using a real Option, we can set None
when the iterator is exhausted, removing any way to call it again. We
also allow niche layout this way, so the Fuse may be smaller.

The FusedIterator specialization does want to ignore the possibility
of exhaustion though, so it uses unsafe { intrinsics::unreachable() }
to optimize that branch away. The entire Fuse implementation is now
isolated in its own module to contain that unsafety.

r? @scottmcm

The former `done` flag was roughly similar to an `Option` tag, but left
the possibity of misuse. By using a real `Option`, we can set `None`
when the iterator is exhausted, removing any way to call it again. We
also allow niche layout this way, so the `Fuse` may be smaller.

The `FusedIterator` specialization does want to ignore the possibility
of exhaustion though, so it uses `unsafe { intrinsics::unreachable() }`
to optimize that branch away. The entire `Fuse` implementation is now
isolated in its own module to contain that unsafety.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 24, 2020
@cuviper
Copy link
Member Author

cuviper commented Mar 24, 2020

This was extracted from #70332 -- let's go ahead and get a perf run:

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Mar 24, 2020

⌛ Trying commit 212e6ce with merge bad75affe428dbcd6e24d9ae6a248ac2e9364c7f...

@cuviper
Copy link
Member Author

cuviper commented Mar 24, 2020

If this one also slows down codegen, I think it might help to use direct match instead of all those uses of as_ref(), as_mut(), and ?.

match self.iter {
Some(ref iter) => iter,
// SAFETY: the specialized iterator never sets `None`
None => unsafe { intrinsics::unreachable() },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally all of these should be hint::unreachable_unchecked() today I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that better? For core that's free to use intrinsics, going through hint just adds more inline indirection...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearer meaning, IMO. But not that important, certainly no need to change things in this PR.

Copy link
Member

@RalfJung RalfJung Mar 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that better?

For example, if we make the hint method panic in debug-builds at some point (to detect incorrect use), then using the intrinsic would not benefit from that check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One should note that this code triggers UB if an iterator implements FusedIterator by error. The implementor don't even have to use unsafe in order to achieve that.

I don't think so. Regardless of I's FusedIterator implementation or anything in I, it can't set the iter field of Fuse to None. This modules never sets iter to None if I is FusedIterator.

@bors
Copy link
Contributor

bors commented Mar 24, 2020

☀️ Try build successful - checks-azure
Build commit: bad75affe428dbcd6e24d9ae6a248ac2e9364c7f (bad75affe428dbcd6e24d9ae6a248ac2e9364c7f)

@rust-timer
Copy link
Collaborator

Queued bad75affe428dbcd6e24d9ae6a248ac2e9364c7f with parent 2dcf54f, future comparison URL.

@timvermeulen
Copy link
Contributor

Does this affect when drop handlers are called? (I don't know how important this is)

@scottmcm
Copy link
Member

scottmcm commented Mar 25, 2020

@timvermeulen Yes it does.

@rust-lang/libs I was going to r+ this, as it's a size win and seems fine in perf. But now that #70374 showed up too, I wanted to ask how you feel about different approaches here. (And to double-check that where Drop happens isn't something that needs to be preserved.)

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR drops the impl TrustedRandomAccess for Fuse<I> if I is !FusedIterator, which seems unintentional.

  unsafe impl<I> TrustedRandomAccess for Fuse<I>
  where
-     I: TrustedRandomAccess,
+     I: TrustedRandomAccess + FusedIterator,

Otherwise looks good to me.

@cuviper
Copy link
Member Author

cuviper commented Mar 25, 2020

@dtolnay that was semi-intentional, because what is that supposed to do if we've already set None? The old code just ignored done and called the iterator regardless.

But TrustedRandomAccess is an internal trait anyway, and all who implement that are already FusedIterator too. Maybe we should just formalize that?

@dtolnay
Copy link
Member

dtolnay commented Mar 25, 2020

The caller of get_unchecked guarantees that the fuse isn't already None, because they have a specific index at which they know an element exists. So you should be able to use the same approach as in as_inner_mut.

@cuviper
Copy link
Member Author

cuviper commented Mar 25, 2020

@dtolnay OK, done!

@dtolnay
Copy link
Member

dtolnay commented Mar 25, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Mar 25, 2020

📌 Commit 4f429c0 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 25, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 25, 2020
Implement Fuse with Option

The former `done` flag was roughly similar to an `Option` tag, but left
the possibity of misuse. By using a real `Option`, we can set `None`
when the iterator is exhausted, removing any way to call it again. We
also allow niche layout this way, so the `Fuse` may be smaller.

The `FusedIterator` specialization does want to ignore the possibility
of exhaustion though, so it uses `unsafe { intrinsics::unreachable() }`
to optimize that branch away. The entire `Fuse` implementation is now
isolated in its own module to contain that unsafety.

r? @scottmcm
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 25, 2020
Implement Fuse with Option

The former `done` flag was roughly similar to an `Option` tag, but left
the possibity of misuse. By using a real `Option`, we can set `None`
when the iterator is exhausted, removing any way to call it again. We
also allow niche layout this way, so the `Fuse` may be smaller.

The `FusedIterator` specialization does want to ignore the possibility
of exhaustion though, so it uses `unsafe { intrinsics::unreachable() }`
to optimize that branch away. The entire `Fuse` implementation is now
isolated in its own module to contain that unsafety.

r? @scottmcm
This was referenced Mar 25, 2020
@AnthonyMikh
Copy link
Contributor

@dtolnay since #70374 modifes Fuse too, may you take a look at that PR as well?

@scottmcm
Copy link
Member

Thanks for taking this on, BTW, @cuviper!

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#70226 (use checked casts and arithmetic in Miri engine)
 - rust-lang#70319 (correctly normalize constants)
 - rust-lang#70352 (Add long error explanation for E0710 )
 - rust-lang#70366 (Implement Fuse with Option)
 - rust-lang#70379 (fix incorrect type name in doc comments)

Failed merges:

 - rust-lang#70375 (avoid catching InterpError)

r? @ghost
@bors bors merged commit 530c320 into rust-lang:master Mar 25, 2020
@cuviper cuviper deleted the option-fuse branch April 3, 2020 18:32
@cuviper cuviper restored the option-fuse branch April 3, 2020 18:32
@cuviper cuviper deleted the option-fuse branch April 3, 2020 18:37
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 4, 2020
Match options directly in the Fuse implementation

Rather than using `as_ref()`, `as_mut()`, and `?`, we can use `match` directly to save a lot of generated code. This was mentioned as a possibility in rust-lang#70366 (comment), and I found that it had a very large impact on rust-lang#70332 using `Fuse` within `Chain`. Let's evaluate this change on its own first.
Centril added a commit to Centril/rust that referenced this pull request Apr 5, 2020
Match options directly in the Fuse implementation

Rather than using `as_ref()`, `as_mut()`, and `?`, we can use `match` directly to save a lot of generated code. This was mentioned as a possibility in rust-lang#70366 (comment), and I found that it had a very large impact on rust-lang#70332 using `Fuse` within `Chain`. Let's evaluate this change on its own first.
Centril added a commit to Centril/rust that referenced this pull request Apr 9, 2020
Implement Chain with Option fuses

The iterators are now "fused" with `Option` so we don't need separate state to track which part is already exhausted, and we may also get niche layout for `None`. We don't use the real `Fuse` adapter because its specialization for `FusedIterator` unconditionally descends into the iterator, and that could be expensive to keep revisiting stuff like nested chains. It also hurts compiler performance to add more iterator layers to `Chain`.

This change was inspired by the [proposal](https://internals.rust-lang.org/t/proposal-implement-iter-chain-using-fuse/12006) on the internals forum. This is an alternate to rust-lang#70332, directly employing some of the same `Fuse` optimizations as rust-lang#70366 and rust-lang#70750.

r? @scottmcm
@dtolnay dtolnay assigned dtolnay and unassigned scottmcm Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants