rust-itertools / itertools

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

Specialize TakeWhileRef fold #884

Open Owen-CH-Leung opened 7 months ago

Owen-CH-Leung commented 7 months ago

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]

Philippe-Cholet commented 7 months ago

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

Owen-CH-Leung commented 7 months ago

@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 commented 7 months ago

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 ?

Philippe-Cholet commented 7 months ago

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.