-
Notifications
You must be signed in to change notification settings - Fork 313
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
Specialize Interleave[Shortest]::fold
#849
Specialize Interleave[Shortest]::fold
#849
Conversation
src/adaptors/mod.rs
Outdated
} | ||
init = a.fold(init, |mut acc, x| { | ||
acc = f(acc, x); | ||
if let Some(y) = b.next() { |
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.
I see we must check if ‘b.next‘ returns something. But if it returns None, couldn’t we exploit this by using try_fold and early-out, leaving the rest to a simple a.fold?
I could imagine that this might be better when b has very few elements compared to a, and I would hope that the compiler is able to generate the early-out without additional checks (it needs to check for None, anyway).
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 makes me think of one the first specializations done in this large streak: https://github.com/rust-itertools/itertools/pull/774/files#diff-5423381b620ccb5a8ab55224d730c8729d676199d27701ac61043cd6773e4617R63-R66
There are probably some others that could possibly benefit from this idea (anytime there are two iterators I guess).
I'm not sure benchmarks would show improvements but it's definitely worth considering.
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.
Benchmark is now -65% instead of -75% initially but I pushed it as it seems to be a better decision for the general case.
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.
As always, LGTM.
Note: We should clean up these adaptors.
flag/phase should probably have the same name (maybe next_is_from_it1 or similar), and we may want to fuse the iterators also for the shortest-variant, or am I missing something?
05e0acf
to
a4e7486
Compare
They could surely be a bit more similar (in another PR) at least the names. I'm pretty sure that fuse the iterators in the shortest-variant would be a breaking change for unfused iterators though. |
Related to #755