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

Force specialization on operator arguments for mapfoldl_impl #33917

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Nov 22, 2019

Eliminates dynamic dispatch for a more than 10x gain in this benchmark.

@jebej
Copy link
Contributor

jebej commented Nov 22, 2019

Is the second change, on L54, actually necessary? There the arguments are just passed through.

I also asked on discourse, but might as well do it here: the f and op arguments are used in the method of L39, shouldn't specialization kick in there?

@timholy
Copy link
Sponsor Member Author

timholy commented Nov 22, 2019

Maybe the first one isn't necessary, but I don't think it hurts either. The one on L54 is clearly important, as profiling will show you.

@timholy timholy merged commit 9bac3a9 into master Nov 22, 2019
@timholy timholy deleted the teh/spec_mapfoldl_impl branch November 22, 2019 18:15
@@ -36,7 +36,7 @@ mul_prod(x::Real, y::Real)::Real = x * y

## foldl && mapfoldl

function mapfoldl_impl(f, op, nt::NamedTuple{(:init,)}, itr, i...)
function mapfoldl_impl(f::F, op::OP, nt::NamedTuple{(:init,)}, itr, i...) where {F,OP}
Copy link
Member

Choose a reason for hiding this comment

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

Migth be worth a comment, as the where {F,OP} is here purely for this unusual reason of forcing specialization, which may not be evident to the next person touching these lines (who might as well, with good intentions, revert this PR unknowingly :) )

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

We might want to do that broadly across Julia's code base. Grep string in #32761 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Ah right! Would it stretch too much the meaning of @specialize to extend this macro to formalize this technique? E.g. @specialize(f), @specialize(op) in this case, even if no @nospecialize was active?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I think it would be great if @specialize did this. I didn't even realize we had it! There's a discourse post from a while back making this exact suggestion.

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.

4 participants