perf: Let &mut T iterators forward for_each and friends #68472
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @matthewjasper re. specializations. |
There was a problem hiding this comment.
Thanks for the PR!
I'm a bit confused though. Your PR comment doesn't quite seem to fit the changes ("specialization on T: Sized" -> that's not your code specializes on, right?). Also, I'm not sure how exactly this would improve things. Could you provide us with a minimal example code where this PR would improve the method implementation that is picked? That would be great!
Or does your edit imply that this PR is not required anymore at all?
I don't know a lot about the interaction of trait objects and specialization, so someone else would need to check regarding that.
src/libcore/iter/traits/iterator.rs
Outdated
| where | ||
| T: Iterator, | ||
| { | ||
| default fn spec_try_fold<B, F, R>(&mut self, init: B, f: F) -> R |
There was a problem hiding this comment.
This default is not necessary and should be removed. This implementation is overwriting another one, but we don't expect this implementation to be overwritten (right?). So we shouldn't put default here.
src/libcore/iter/traits/iterator.rs
Outdated
| (**self).try_fold(init, f) | ||
| } | ||
|
|
||
| default fn spec_try_for_each<F, R>(&mut self, f: F) -> R |
src/libcore/iter/traits/iterator.rs
Outdated
| @@ -83,7 +83,7 @@ fn _assert_is_object_safe(_: &dyn Iterator<Item = ()>) {} | |||
| on( | |||
| _Self = "{integral}", | |||
| note = "if you want to iterate between `start` until a value `end`, use the exclusive range \ | |||
| syntax `start..end` or the inclusive range syntax `start..=end`" | |||
| syntax `start..end` or the inclusive range syntax `start..=end`" | |||
There was a problem hiding this comment.
Please undo all formatting changes like this (which I assume your editor/rustfmt automatically did). At least I think stray formatting changes are not really great.
| #[stable(feature = "rust1", since = "1.0.0")] | ||
| impl<I: Iterator + ?Sized> Iterator for &mut I { | ||
| type Item = I::Item; | ||
|
|
||
| #[inline] |
There was a problem hiding this comment.
Have you measured the impact of these inline attributes you are adding? I'd prefer not to use #[inline] unless we are pretty sure it leads to faster code somewhere.
There was a problem hiding this comment.
I thought #[inline] is a sensible default because it lets LLVM decide whether to inline or not, and all iterator methods in libcore that I can find have this attribute.
There was a problem hiding this comment.
AFAIK (we really need a de-facto standard guide on inline...) that they are not necessary on generic items as their definitions will be available in downstream crates anyway.
There was a problem hiding this comment.
That is correct, but next itself isn't generic, so I believe here it does make a difference? And afaik for generic functions the attribute is ignored, so adding it shouldn't hurt.
|
Since rust/src/libcore/iter/traits/iterator.rs Lines 1768 to 1779 in e0bbe79 chain specify optimized versions of these rust/src/libcore/iter/adapters/chain.rs Lines 83 to 103 in e0bbe79 &mut we end up calling the slower, default implementation instead of the optimized one.
Originally I though that I got tripped by this but it turns out that it requires more work to trigger. fn consume(iter: &mut impl Iterator<Item = i32>) {
// Calls the optimized version
iter.for_each(|x| println!("{}", x));
// Calls the default
(&mut iter).for_each(|x| println!("{}", x)); |
Might have done something wrong with specialization, but the idea is to specialize on
It wasn't an issue in the PR I linked, but it could still happen otherwise. |
There was a problem hiding this comment.
Since
impl Iterator for &mut Tdoes not specifytry_foldand friends, rust will instead call into the default methods specified onIterator.
That problem I understand. What I didn't understand initially is why the solution had to be so overly complicated. But I think I got it now:
The easiest would be to just overwrite the for_each method in the impl<I: Iterator + ?Sized> Iterator for &mut I and call (**self).for_each(f), right? But I assume you tried that and it didn't work because I is unsized while for_each requires Self: Sized. So your idea was to use specialization to be able to call the correct implementation if T is Sized. The next idea would be to add the specialization impl impl<I: Iterator> Iterator for &mut I (Sized this time!). But that doesn't currently work because default types are strange.
And that's why you added another trait and used specialization there, yes? If so, including the reasoning explaining your code in the PR description is very much appreciated. At least for someone like me who doesn't immediately understand why things are done the way they are.
Might have done something wrong with specialization, but the idea is to specialize on
impl<T> Iterator for &mut T where T: Sizedwhile still allowingimpl<T> Iterator for &mut T where T: ?Sized
I was just confused as neither, the impl that is specialized and the specializing impl, had a Sized or ?Sized bound.
|
In general, I'm still not sure if the potential speed gains are worth increasing the implementation complexity like that. A specific motivating example would probably help. I will talk to others about this! |
|
This would be a big improvement for situations where you want to use an iterator adaptor without losing ownership of the original iterator. The let mut iter = [1, 2, 3].iter();
let sum: i32 = iter.by_ref().take(2).fold(0, |acc, i| acc + i);
assert_eq!(sum, 3);
assert_eq!(iter.next(), Some(&3));At the moment the second line will use external iteration because |
|
Ping from triage: @Marwes can you address the review comments? |
|
Down prioritized this since it seemed like a lot of complexity for little gain, @timvermeulen s example is convincing however. Will try and simplify and do this over the weekend. |
Currently `impl Iterator for &mut T` does not forward `try_fold` to the inner `T` which means that it is easy to lose optimized implementations of this method and methods that forward to it. By using specialization on `T: Sized` we can allow calls on non-trait objects to still forward to the inner methods.
Marwes
force-pushed
the
iterator_by_ref
branch
from
839d8b2 to
a21554c
Compare
|
Simplified this to not require an extra trait (mimicked This also only specializes |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
|
LukasKalbertodt
added
S-waiting-on-author
|
As you might have noticed, we had a bit of a problem with specialization in the standard library recently. So for now we don't accept any more changes which include specialization until we have some tests in place to ensure we do not accidentally expose that specialization. But this will likely take some time. Since there is no activity in this PR for two weeks and because of that moratorium, I will close this PR for now. Thanks anyway! |
|
Now that |
Currently
impl Iterator for &mut Tdoes not forwardfor_eachto theinner
Twhich means that it is easy to lose optimized implementationsof these methods. (See #68046)
By using specialization on
T: Sizedwe can allow calls on non-traitobjects to still forward to the inner methods.
This isn't tested yet and there are more methods that should be
forwarded, just posting this as a POC and to see if it is something that
is wanted.
EDIT: I believe I were wrong in #68046 and I did not get the slow default implementation.
by_refcalled on a&mut Twill just yield&mut Tagain, so you more or less have to use&mut iteron a&mut Tto get the slow implementations which this fixes. Might warrant a clippy lint at least if this looks to heavy handed.