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

Match options directly in the Fuse implementation #70750

Merged
merged 2 commits into from
Apr 6, 2020

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Apr 3, 2020

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 #70366 (comment), and I found that it had a very large impact on #70332 using Fuse within Chain. Let's evaluate this change on its own first.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2020
@cuviper
Copy link
Member Author

cuviper commented Apr 3, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Apr 3, 2020

⌛ Trying commit 6fdd4f3 with merge ec7a02385a47e4dfcfa6ca70e1f167ba4a3f8985...

self.iter = None;
}
nth
fuse!(self.iter.nth(n))
Copy link
Member

Choose a reason for hiding this comment

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

This is beautiful. Nicely done.

@bors
Copy link
Contributor

bors commented Apr 4, 2020

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

@rust-timer
Copy link
Collaborator

Queued ec7a02385a47e4dfcfa6ca70e1f167ba4a3f8985 with parent f6fe99c, future comparison URL.

@@ -37,35 +53,36 @@ where

#[inline]
default fn next(&mut self) -> Option<<I as Iterator>::Item> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to the tweaks in this PR... there's a default fn here and elsewhere that shouldn't be (Specialization should be negotiated through internal traits.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, we discovered problematic usage of associated type defaults much older, still, it needed to be fixed however old. :)

Copy link
Member

Choose a reason for hiding this comment

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

@Centril Maybe open an E-Easy issue for it with instructions for a new contributor to pick up? I agree that it's "unrelated to the tweaks in this PR", so I don't think it needs to happen here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@scottmcm Oh sure, I didn't mean it should be fixed in this PR. :)

I've filed #70796.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit ec7a02385a47e4dfcfa6ca70e1f167ba4a3f8985, comparison URL.

@scottmcm
Copy link
Member

scottmcm commented Apr 4, 2020

This looks fairly neutral, but I suspect that's largely due to most iterators hitting the specialization, not the interesting ones. We've seen a bunch of examples where this is helpful (the for-vs-try_fold conversation, the len! in the slice iterator, etc) and I think this code is independently good, so...

@bors r+

@bors
Copy link
Contributor

bors commented Apr 4, 2020

📌 Commit 6fdd4f3 has been approved by scottmcm

@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 Apr 4, 2020
@scottmcm scottmcm assigned scottmcm and unassigned shepmaster Apr 4, 2020
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#70553 (move OS constants to platform crate)
 - rust-lang#70665 (Do not lose or reorder user-provided linker arguments)
 - rust-lang#70750 (Match options directly in the Fuse implementation)
 - rust-lang#70782 (Stop importing the float modules in documentation)
 - rust-lang#70798 ("cannot resolve" → "cannot satisfy")
 - rust-lang#70808 (Simplify dtor registration for HermitCore by using a list of destructors)
 - rust-lang#70824 (Remove marker comments in libstd/lib.rs macro imports)

Failed merges:

r? @ghost
@bors bors merged commit 618ba73 into rust-lang:master Apr 6, 2020
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
@cuviper cuviper deleted the direct-fuse branch May 30, 2020 21:55
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.

7 participants