search

rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.69k stars 303 forks source link

Rewrite `MultiProduct` #835

Closed Philippe-Cholet closed 7 months ago

Philippe-Cholet commented 8 months ago

Fixes #337. Closes #603 since this is an alternative.

834 legitimately asked that we work on the bugfix #603. But after rebasing it, I saw that I could finally fuse the iterator (and therefore have a working specialization test). I thought of using #603 as a base but I eventually chose to make my own changes, starting over. I however credited @JakobDegen where it made sense.

I made commits as atomic as I could.

So this fuses the iterator which is a breaking change, which won't affect most people.

jswrenn commented 8 months ago

Thanks for this! I'll give it a review tomorrow!

Philippe-Cholet commented 8 months ago

TL;DR The choice I made in https://github.com/rust-itertools/itertools/pull/835/commits/db5d9062a1e399f86807f773615aa6c747be3275 does not seem to affect performance.

I just tried cur: Option<I::Item>, in each MultiProductIter (as before in "master") instead of cur: Option<Vec<I::Item>>, in MultiProductInner (as in this PR), benchmarked both and got similar performances (both a bit slower for next than in "master" +8%, but with bugfix and fused).

EDIT: It's not as polished as this PR but this is the alternative I'm talking about.

codecov-commenter commented 7 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (62d2b65) 93.43% compared to head (085990c) 93.69%.

Files Patch % Lines
src/adaptors/multi_product.rs 97.56% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #835 +/- ## ========================================== + Coverage 93.43% 93.69% +0.25% ========================================== Files 48 48 Lines 6778 6755 -23 ========================================== - Hits 6333 6329 -4 + Misses 445 426 -19 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Philippe-Cholet commented 7 months ago

I rebased on master and added 3 commits to comment "state and values" with code.

Philippe-Cholet commented 7 months ago

I merely rebased (no conflict) to see if the new CI still accepts this. Do we merge this (and #853) now or in the next release?

jswrenn commented 7 months ago

I'll cut a release with our non-breaking changes first, probably tomorrow.

Philippe-Cholet commented 7 months ago

Now that "0.12.1" is released, I renamed the associated milestone and created a new one. I think it's time to merge this and approve/merge #853 as well, right?