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 TakeWhileRef fold #884

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Owen-CH-Leung
Copy link
Contributor

#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]

Specialize TakeWhileRef fold using fold_while logic
@Philippe-Cholet
Copy link
Member

@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 Owen-CH-Leung reopened this Feb 26, 2024
@Owen-CH-Leung
Copy link
Contributor Author

@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

let acc = init;
let original_iter = self.iter.clone();

let result = self.iter.clone().fold_while(acc, |acc, x| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not clone it a second time.

@Owen-CH-Leung
Copy link
Contributor Author

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
Copy link
Member

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.

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

2 participants