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

Artifacts: pull out a recursive function from a closure to a stand alone function #49755

Merged
merged 1 commit into from
May 12, 2023

Conversation

KristofferC
Copy link
Member

Recursive closures have to be boxed and type inference gives up. This improves the load time of LLVMExtra.jl that calls into Artifacts at runtime.

@KristofferC KristofferC added compiler:latency Compiler latency backport 1.9 Change should be backported to release-1.9 labels May 11, 2023
@KristofferC KristofferC requested a review from staticfloat May 11, 2023 08:24
@staticfloat
Copy link
Member

Just to check, it’s enough to add the override file parameter without moving the function definition to be top level, right?

@KristofferC
Copy link
Member Author

I dont actually know. I'll try it out.

@KristofferC
Copy link
Member Author

KristofferC commented May 11, 2023

This suggests that it is not enough:

julia> function f(x)
    g(x) = x == 0 ? 1 : x * g(x-1)
    return g(x)
end;

julia> @code_warntype f(5)
MethodInstance for f(::Int64)
  from f(x) @ Main REPL[2]:1
Arguments
  #self#::Core.Const(f)
  x::Int64
Locals
  g@_3::Core.Box
  g@_4::Union{}
  g@_5::Union{}
Body::Any

@staticfloat
Copy link
Member

I did not know that! I’ll have to avoid using interior functions then.

@KristofferC KristofferC merged commit 6733197 into master May 12, 2023
@KristofferC KristofferC deleted the kc/art_clos branch May 12, 2023 09:13
@KristofferC
Copy link
Member Author

I’ll have to avoid using interior functions then.

At least when they are recursive I guess.

KristofferC added a commit that referenced this pull request May 15, 2023
@KristofferC KristofferC mentioned this pull request May 19, 2023
51 tasks
KristofferC added a commit that referenced this pull request May 27, 2023
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label May 28, 2023
kpamnany pushed a commit that referenced this pull request Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:latency Compiler latency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants