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

Implement custom fold for ZipSlice #762

Conversation

Owen-CH-Leung
Copy link
Contributor

As per discussion in #755 , this PR implements custom fold logic for ZipSlices<T, U> and create benchmark test to measure the performance gain compared with the default fold.

Running the zipdot i32 zipslices fold benchmark using the default fold logic, the time was time: [42.749 ns 43.163 ns 43.782 ns]

Running the zipdot i32 zipslices fold benchmark using the custom fold logic, the time was time: [42.523 ns 42.621 ns 42.737 ns]

implement custom fold, and create benchmark test for measuring performance improvement
@Philippe-Cholet
Copy link
Member

I was gonna say I was surprised by the unsafe use, especially for such time win but ZipSlices is not part of the itertools library, it's only part of benches.

Copy link
Member

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

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

I appreciate the help on #755. However, as @Philippe-Cholet suggests, our focus is on improving the performance of user-facing iterators. Specializing this particular fold adds maintenance cost without any performance gain (even internally).

@jswrenn jswrenn closed this Sep 27, 2023
@Owen-CH-Leung
Copy link
Contributor Author

Ahh yesss I misread. I thought it's under src when I was revising it. Thanks both for your comments. I'll continue to see if custom fold can be implemented in other iterators

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