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

Improving broadcasting performance by working around recursion limits of inlining #41139

Closed

Conversation

mateuszbaran
Copy link
Contributor

This PR implements the changes proposed in #41090 . All it does is duplicating the two methods that are marked as deletions in this PR. Benchmarks that motivate this change are available here: #41090 (comment) .

@mbauman
Copy link
Member

mbauman commented Jun 8, 2021

I must confess it's always a challenge for me to understand the existing recursive magic whenever I look at this file. I can often get there, but it makes my head hurt a bit.

I understand the motivation here, but the indirection makes this intractable for me. 🤯

@mateuszbaran
Copy link
Contributor Author

I still don't understand how make_makeargs works, I just noticed that these specific methods weren't inlined sometimes and found a way to fix that 🙂 .

@mateuszbaran
Copy link
Contributor Author

Bump?

@Pramodh-G
Copy link
Contributor

Bump

@oscardssmith oscardssmith added the triage This should be discussed on a triage call label Jun 30, 2021
@oscardssmith
Copy link
Member

I think we need triage to decide whether the performance gain is worth the added complexity.

end

for UB in [Any, RecursiveInliningEnforcerA]
@eval @inline function (bb::RecursiveInliningEnforcerB{TMT,TMH,TT,TH,TF})(args::Vararg{Any,N}) where {N,TMT,TMH<:$UB,TT,TH,TF}
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need two methods? Doesn't the Any bound cover everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC the compiler currently has problems with inlining recursive method calls, so by having two different methods doing the same thing there is less recursion to handle. There is no other reason to having two methods there. I don't know how exactly this works so I may be wrong though -- but it works.

@JeffBezanson
Copy link
Member

Triage thinks this is a bit too dependent on current details of how the compiler works. It basically just names the types of the returned functions and adds unnecessary extra methods. From looking at the code, there is no good reason why that should have any effect. So we should try to improve the compiler instead.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jul 1, 2021
N5N3 added a commit to N5N3/julia that referenced this pull request Feb 11, 2022
Similar to JuliaLang#41139, but avoid unnecessary extra methods.
N5N3 added a commit to N5N3/julia that referenced this pull request Feb 11, 2022
Similar to JuliaLang#41139, but avoid unnecessary extra methods.
N5N3 added a commit to N5N3/julia that referenced this pull request Mar 12, 2022
Similar to JuliaLang#41139, but avoid unnecessary extra methods.
N5N3 added a commit to N5N3/julia that referenced this pull request May 1, 2022
Similar to JuliaLang#41139, but avoid unnecessary extra methods.
N5N3 added a commit to N5N3/julia that referenced this pull request May 10, 2022
Similar to JuliaLang#41139, but avoid unnecessary extra methods.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants