Improved the use of Extend in a few Iterator methods
#65219
Conversation
- Reimplemented `Iterator::partition` to use a full iterator `extend` on one of the two collections, rather than pushing to both of them one element at a time. - Reimplemented `Iterator::unzip` to use a full iterator `extend` on one of the two collections, rather than pushing to both of them at the same time
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @cuviper |
There was a problem hiding this comment.
You call this an improvement, but do you have anything to quantify that?
In theory I could see this helping performance if the left container type implements a folding extend, combined with an iterator that benefits from folds like Chain or FlatMap. It would be nice to demonstrate this with a benchmark though.
No; this is just based on published best-practice for iterators. In particular I'm not sure about the partitioner, because the main benefit of using However, it should be an improvement in most cases on |
|
OK, so besides allowing internal iteration (folding), allocating for a good It would be nice if |
|
Hmm, you could potentially pre-allocate both collections with an empty iterator that lies about its size, something like: pub struct SizeHint<T> {
hint: (usize, Option<usize>),
item: PhantomData<T>,
}
impl<T> Iterator for SizeHint<T> {
type Item = T;
fn next(&mut self) -> Option<T> {
None
}
fn size_hint(&self) -> (usize, Option<usize>) {
self.hint
}
} |
Let's not bring that back (#34155). Lying about So this PR brings up an interesting question about whether implementations of |
Ha! I am amused how close that was to what I just wrote. Good point about early-terminating |
That's... a good idea. I was thinking that I was annoyed that there's no way to hint extend, and was thinking of (and rejecting) various transducer-based ideas for how to fix it, but that actually makes way more sense. I'll see about adding & documenting something like that as a separate PR. |
This is a good point. I suppose it's a case of being under-specified (similar to #52279) |
|
There's some precedent in the standard library for iterator adapters doing extra work to emulate ordinary iteration for the purpose of preserving observable side effects (for instance, this part of the |
|
Follow-up: regardless of the behavior of
So I'll make an update that fulfills that contract even if |
|
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 |
|
As a matter of precedent, tuples supply an |
It is more useful if it does not require the iterator to be fully consumed. This leaves handling of 'overflow' to the caller. Since the definition of Also, It's of course perfectly fine for some utility methods such as |
To be clear, that's just for the unit |
There was a problem hiding this comment.
This code is getting complicated, which increases the need for benchmarks to justify it. The change to extending through &mut self, allowing a followup to consume the remainder, also means they can't take advantage of internal iteration anymore, so we're only left with possible pre-allocation benefits.
I think the partition implementation also needs to fully consume the same way.
I was going to ask about this part anyway– is there any reason that EDIT: See follow-up, here: #65219 (comment) |
I'm working on a benchmark, as well as a regression test for the changed behavior demonstrated in #65219 (comment) |
- Simplified unzip implementation (removed size_hint checks) - Added forwarding try_fold implementation to &mut T: Iterator
|
For reference: I looked into implementing |
|
Quick note: This code change includes a closure. I'd like to replace it with a method, but I tried this: fn invert<T>(predicate: impl FnMut(T) -> bool) -> impl FnMut(T) -> bool {
move |value| !predicate(value)
}
// I also tried FnMut(&T) -> bool and FnMut(&'a T) -> bool + 'a
// ...
right.extend(fused.filter(invert(predicate)));And got this error: Which appears to be lifetime related issue that I can't figure out how to solve. At this point I'm leaning toward reverting the change to |
|
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 |
|
Ping from triage. @cuviper any updates on this? Thanks. |
JohnCSimon
added
S-waiting-on-author
|
Ping from triage. |
JohnCSimon
added
the
S-inactive
|
Ping from triage. Thank you! |
Iterator::partitionto use a full iteratorextendon one of the two collections, rather than pushing to both of them one element at a time.Iterator::unzipto use a full iteratorextendon one of the two collections, rather than pushing to both of them at the same time