-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Conversation
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 6fdd4f3 with merge ec7a02385a47e4dfcfa6ca70e1f167ba4a3f8985... |
self.iter = None; | ||
} | ||
nth | ||
fuse!(self.iter.nth(n)) |
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.
This is beautiful. Nicely done.
☀️ Try build successful - checks-azure |
Queued ec7a02385a47e4dfcfa6ca70e1f167ba4a3f8985 with parent f6fe99c, future comparison URL. |
@@ -37,35 53,36 @@ where | |||
|
|||
#[inline] | |||
default fn next(&mut self) -> Option<<I as Iterator>::Item> { |
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.
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.)
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.
Hmm, it's been like that for a little while...
https://github.com/rust-lang/rust/pull/35656/files#diff-d93bf2799d551e25b17e877b23a37db3L1659-R1726
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, we discovered problematic usage of associated type defaults much older, still, it needed to be fixed however old. :)
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.
@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.
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.
Finished benchmarking try commit ec7a02385a47e4dfcfa6ca70e1f167ba4a3f8985, comparison URL. |
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 @bors r |
📌 Commit 6fdd4f3 has been approved by |
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.
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
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
Rather than using
as_ref()
,as_mut()
, and?
, we can usematch
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 usingFuse
withinChain
. Let's evaluate this change on its own first.