Skip to content
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

Merged

Conversation

Philippe-Cholet
Copy link
Member

@Philippe-Cholet Philippe-Cholet commented Jan 13, 2024

Related to #755

cargo bench --bench specializations "interleave(_shortest)?/fold"

interleave/fold            [2.1749 µs 2.1782 µs 2.1817 µs]
                           [754.64 ns 757.16 ns 760.51 ns]
                           [-66.092% -65.545% -65.107%]

interleave_shortest/fold   [1.6414 µs 1.6460 µs 1.6511 µs]
                           [412.05 ns 413.60 ns 415.41 ns]
                           [-74.904% -74.580% -74.173%]

@Philippe-Cholet Philippe-Cholet added this to the next milestone Jan 13, 2024
}
init = a.fold(init, |mut acc, x| {
acc = f(acc, x);
if let Some(y) = b.next() {
Copy link
Member

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).

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

@phimuemue phimuemue left a 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?

src/adaptors/mod.rs Outdated Show resolved Hide resolved
@Philippe-Cholet
Copy link
Member Author

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.

@Philippe-Cholet Philippe-Cholet added this pull request to the merge queue Jan 16, 2024
Merged via the queue into rust-itertools:master with commit 98344ad Jan 16, 2024
9 checks passed
@Philippe-Cholet Philippe-Cholet deleted the fold-interleave branch January 16, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants