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 WithPosition::fold #772

Merged
merged 4 commits into from
Oct 6, 2023

Conversation

Philippe-Cholet
Copy link
Member

@Philippe-Cholet Philippe-Cholet commented Oct 2, 2023

Each call to f is done with a Position variant known at compile time so I think it might be optimized away in some cases?

Without specialization: [5.3231 µs 5.3487 µs 5.3806 µs]
With specialization:    [2.6974 µs 2.7042 µs 2.7132 µs]

We should mention #755 in PRs that specialize fold (such as this one) to help keep track of the advancement of the TODO list I'm gonna make there.

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.

Hi @Philippe-Cholet! Nice idea.

{
if !self.handled_first {
// Haven't seen the first item yet, and there might be one to give.
self.handled_first = true;
Copy link
Member

@phimuemue phimuemue Oct 2, 2023

Choose a reason for hiding this comment

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

Do we actually need to set handled_first=true? self is consumed and handled_first seems unused afterwards.

}
}
}
if let Some(mut head) = self.peekable.next() {
Copy link
Member

@phimuemue phimuemue Oct 3, 2023

Choose a reason for hiding this comment

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

I'm somehow bothered because we call peekable.next here, even though we called peekable.peek some lines before. I'm not sure if we can easily avoid this (without convoluting control flow too much)...

Maybe something like this (only a draft - didn't test it):

    if let Some(head) = if !handled_first {
        if let Some(first) = self.peekable.next() {
            match self.peekable.next() {
                Some(head) => {
                    init = {
                        f(init, (Position::First, first));
                        Some(head)
                    }
                }
                None => return f(init, (Position::Only, first)),
            }
        }
    } else {
        self.peekable.next()
    } {
        // Have seen the first item, and there's something left.
        init = self.peekable.fold(init, |acc, mut item| {
            std::mem::swap(&mut head, &mut item);
            f(acc, (Position::Middle, item))
        });
        // The "head" is now the last item.
        init = f(init, (Position::Last, head));
    }

@Philippe-Cholet You can use your judgement here.

@Philippe-Cholet
Copy link
Member Author

Philippe-Cholet commented Oct 3, 2023

@phimuemue
I do not update handled_first field anymore, as it's consumed.
I avoid to peek entirely, the bench become even faster. Control flow is quite updated but remains nice, might be even nicer.
And rebased on master.

[5.4031 µs 5.4195 µs 5.4353 µs] // without specialization
[2.6974 µs 2.7042 µs 2.7132 µs] // previous specialization with peek+next
[563.64 ns 565.92 ns 568.61 ns] // with new specialization

It bothered me a bit too to peek before calling next but I didn't think it would matter that much, thanks!

@Philippe-Cholet
Copy link
Member Author

Just adding that I initially tried to avoid std::mem::swap with (init, head) = self.peekable.fold((init, head), ...); which worked for me but not for the msrv CI test. It did not like assigning to a tuple. The performance was not affected.

@phimuemue
Copy link
Member

Nicely done, thanks.

As an aside: Does the mail address from your commits work?

@phimuemue phimuemue added this pull request to the merge queue Oct 6, 2023
Merged via the queue into rust-itertools:master with commit b8a77c4 Oct 6, 2023
8 checks passed
@Philippe-Cholet Philippe-Cholet deleted the fold-with-position branch October 6, 2023 11:44
@Philippe-Cholet
Copy link
Member Author

@phimuemue I did not check the mail address for quite some time, but I definitely did this time.

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