itertools icon indicating copy to clipboard operation
itertools copied to clipboard

Specialize TakeWhileRef fold

Open Owen-CH-Leung opened this issue 1 year ago • 4 comments

#755

This PR introduces a custom fold method for TakeWhileRef. The optimization hinges on the fold_while logic:

Benchmark Results:

Default fold: time: [53.963 ns 54.083 ns 54.209 ns] Custom fold: time: [53.210 ns 53.359 ns 53.511 ns]

Owen-CH-Leung avatar Feb 26 '24 03:02 Owen-CH-Leung

@Owen-CH-Leung I'm just curious about why you closed this. I did not look into the details.

TakeWhileRef is a bit of a pain for me. Because of &mut I, it's not clonable so it's not tested yet in "tests/specializations.rs" and not benchmarked in "benches/specializations.rs". Any improvement on that front would be appreciated.

Philippe-Cholet avatar Feb 26 '24 11:02 Philippe-Cholet

@Owen-CH-Leung I'm just curious about why you closed this. I did not look into the details.

TakeWhileRef is a bit of a pain for me. Because of &mut I, it's not clonable so it's not tested yet in "tests/specializations.rs" and not benchmarked in "benches/specializations.rs". Any improvement on that front would be appreciated.

Opps just wrong click.

Yeah I was trying to use the fold_while implementation to implement fold for TakeWhileRef, but I think my changes will now leave the original iterator unchanged and thus it's failing one of the doc test take_while_ref. And I couldn't think of a way to keep the performance gain while also having the intended effect as illustrated in the take_while_ref doctest

Owen-CH-Leung avatar Feb 26 '24 13:02 Owen-CH-Leung

TakeWhileRef is a bit of a pain for me. Because of &mut I, it's not clonable so it's not tested yet in "tests/specializations.rs" and not benchmarked in "benches/specializations.rs". Any improvement on that front would be appreciated.

I think the design &mut I is chosen such that the API take_while_ref can directly mutate the iterator without transferring the ownership or cloning the iterator. This will make the iterator more performant I believe. Are we open to changing the API such as making it take over the ownership of the iterator / creating interior mutability ?

Owen-CH-Leung avatar Mar 04 '24 14:03 Owen-CH-Leung

I don't think we are open of changing the API (see #710). Plus, as I did not play much with take_while_ref, I'm not sure to fully understand its purpose.

About testing, we currently have a fn test_specializations<I>(it: &I) where ..., I: Clone; but I think we could have a similar fn test_specializations_no_iter_clone<I>(get_it: impl Fn() -> I) where ...; where the function get_it could clone or do something else for unclonable iterators but that's only an idea for now: I tried applying it a bit but it's definitely not straightforward.

Philippe-Cholet avatar Mar 04 '24 14:03 Philippe-Cholet