Make the specialized Fuse still deal with None #86765
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Queuing for a performance run, especially relative to #86766. @bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 6f5e933 with merge 014fd74e3bfafd8c10f931b6fbc233a19f81413b... |
|
☀️ Try build successful - checks-actions |
|
Queued 014fd74e3bfafd8c10f931b6fbc233a19f81413b with parent 868c702, future comparison URL. |
|
Finished benchmarking try commit (014fd74e3bfafd8c10f931b6fbc233a19f81413b): comparison url. Summary: This benchmark run did not return any significant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
| // SAFETY: unsafe function forwarding to unsafe function with the same requirements | ||
| Some(ref mut iter) => unsafe { SourceIter::as_inner(iter) }, | ||
| // SAFETY: the specialized iterator never sets `None` | ||
| None => unsafe { intrinsics::unreachable() }, |
There was a problem hiding this comment.
The trait impl is being removed due to the unreachable here, right? I think it might be possible to replace this with a panic instead.
If someone pulled the HRTB shenaningans then it would only lead to a panic, not to unsafety. If that's not good enough then maybe SourceIter could be changed to return an Option<&mut>.
There was a problem hiding this comment.
I personally don't think we should use a panic for code that is otherwise allowable, however weird it may be.
Changing SourceIter to return an Option sounds promising, assuming there actually is a reasonable fallback at the caller -- I suppose going back to normal not-in-place collection. Then you wouldn't even need the seemingly-unrelated I: FusedIterator constraint here.
There was a problem hiding this comment.
I suppose going back to normal not-in-place collection.
That would probably be bad for compile times since it would need two different code paths in that case.
If we can assume that None implies an exhausted iterator then it could work and simply return an empty Vec. But I'll have to look through all the implementations to see if that's a valid assumption.
There was a problem hiding this comment.
Then you wouldn't even need the seemingly-unrelated I: FusedIterator constraint here.
In place iteration works roughly like this
- retrieve source iter to get a write pointer
- perform loop that gets elements from the iterator and writes to the pointer
- retrieve source iter again to drop the remainder and then steal the allocation
Which means that once we have started iterating the source must stay retrievable. Which means the iterator cannot be fused through the option because that would drop the source. So it can only work for FusedIterator which doesn't switch the option variant.
Under those circumstances it should be safe since user code doesn't have access to the iterator during that time and can't do the variance coercion.
There was a problem hiding this comment.
OK, yeah, you need it to not drop. I agree the user doesn't have access during your steps, but they could have played the variance game before it got there at all, so it might be None to start with. Then an empty collection is fine.
|
Reviewing on the basis of the code here looking reasonable, relying on other statements that this fixes the underlying problem it intends to fix. @bors r+ |
|
📌 Commit 6f5e933 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Fixes #85863 by removing the assumption that we'll never see a cleared iterator in the
I: FusedIteratorspecialization. Now allFusemethods check for the possibility thatself.iterisNone, and the specialization only avoids setting that toNonein&mut selfmethods.