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 Positions::[r]fold #813

Merged

Conversation

Philippe-Cholet
Copy link
Member

@Philippe-Cholet Philippe-Cholet commented Dec 7, 2023

Related to #755

First, I have to say that after adding many benchmarks in #806, building specialization benchmarks is now slower: 1mn53s hence more than 4mn to run this benchmarking command twice:

cargo bench --bench specializations "positions/r?fold"

positions/fold          time:   [908.94 ns 911.83 ns 914.93 ns]
positions/fold          time:   [440.01 ns 442.34 ns 445.05 ns]
                        change: [-52.042% -51.081% -49.844%]

positions/rfold         time:   [580.24 ns 581.23 ns 582.15 ns]
positions/rfold         time:   [612.87 ns 614.28 ns 615.81 ns]
                        change: [+5.5346% +6.7070% +7.8680%]

Benchmarks all rely on core::slice::Iter which does not specialize rfold but I did not thought it would be slower.
It's not by much but still. What do you think?

Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

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

Thanks @Philippe-Cholet, looks good to me.

I think we should replace iter/count by Enumerate before adding further specializations.

@Philippe-Cholet
Copy link
Member Author

Philippe-Cholet commented Dec 8, 2023

@phimuemue
After changing fold (to use enumerate below), I re-ran the benchmark and roughly got 500 ns (so slower). My guess is that it's because + count is done much more often and not once. (The +1 operations are only moved to enumerate.)

    let count = self.count;
    let mut f = self.f;
    self.iter.enumerate().fold(init, |mut acc, (idx, val)| {
        if f(val) {
            acc = func(acc, idx + count);
        }
        acc
    })

A very small loss for rfold.

@Philippe-Cholet Philippe-Cholet added this to the next milestone Dec 8, 2023
@phimuemue
Copy link
Member

@phimuemue After changing fold (to use enumerate below), I re-ran the benchmark and roughly got 500 ns (so slower). My guess is that it's because + count is done much more often and not once. (The +1 operations are only moved to enumerate.)

    let count = self.count;
    let mut f = self.f;
    self.iter.enumerate().fold(init, |mut acc, (idx, val)| {
        if f(val) {
            acc = func(acc, idx + count);
        }
        acc
    })

A very small loss for rfold.

I meant replacing iter/count in Positions entirely, not just in [r]fold. So, I suggest merging this PR, and afterwards replacing Positions's fields.

@Philippe-Cholet Philippe-Cholet added this pull request to the merge queue Dec 8, 2023
Merged via the queue into rust-itertools:master with commit 0fc4675 Dec 8, 2023
8 checks passed
@Philippe-Cholet Philippe-Cholet deleted the r-fold-positions branch December 8, 2023 17:51
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.

3 participants