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

Precompile fails when multiple methods fit the supplied signature #38149

Closed
NHDaly opened this issue Oct 23, 2020 · 5 comments · Fixed by #41958
Closed

Precompile fails when multiple methods fit the supplied signature #38149

NHDaly opened this issue Oct 23, 2020 · 5 comments · Fixed by #41958
Labels
bug Indicates an unexpected problem or unintended behavior compiler:latency Compiler latency compiler:precompilation Precompilation of modules

Comments

@NHDaly
Copy link
Member

NHDaly commented Oct 23, 2020

This is a more in-depth explanation of the precompile failures seen in Case 4 in #28808 (comment).

precompile(f, (Any,...)) fails incorrectly for methods marked @nospecialize when the function in question has other, more specific methods defined.

An explanation follows.


As you can see, here, precompile works correctly for @nospecialize functions, when there is only one method:

julia> using MethodAnalysis

julia> module M
       @nospecialize
       foo(x, y) = x+y
       end
Main.M

julia> precompile(M.foo, (Any,Any))  # returns `true`, as expected.
true

julia> MethodAnalysis.methodinstances(M.foo)  # the new MethodInstance has been specialized and compiled:
1-element Array{Core.MethodInstance,1}:
 MethodInstance for foo(::Any, ::Any)

julia> M.foo(2.0, 3)  # you can call it with some arguments, and it works, and...
5.0

julia> MethodAnalysis.methodinstances(M.foo)   # ... doesn't create a new specialization
1-element Array{Core.MethodInstance,1}:
 MethodInstance for foo(::Any, ::Any)

However, when the function contains multiple methods, with one more specific than the other, precompile(foo, (Any,Any)) fails, incorrectly:

julia> using MethodAnalysis

julia> module M
       @nospecialize
       foo(x, y) = x+y         # (::Any, ::Any)
       foo(a::Int, b) = a+b    # (::Int, ::Any)
       end
Main.M

julia> precompile(M.foo, (Int,Any))  # This one specializes correctly ✔️
true

julia> precompile(M.foo, (Any,Any))  # 😱 But this one fails incorrectly! 😱 
false

julia> MethodAnalysis.methodinstances(M.foo)  # And we can see it failed to create a new MethodInstance for (Any,Any)
1-element Vector{Core.MethodInstance}:
 MethodInstance for foo(::Int64, ::Any)

julia> M.foo(2.0, 3)  # Even though when we actually _call_ it, it of course must create the MethodInstance:
5.0

julia> MethodAnalysis.methodinstances(M.foo)  # Now it's there, indeed.
2-element Vector{Core.MethodInstance}:
 MethodInstance for foo(::Int64, ::Any)
 MethodInstance for foo(::Any, ::Any)

This causes several static compilation failures in our production binary, meaning we end up compiling them again in production.

I tried stepping through the function in GDB to identify what's going on, and precompile(M.foo, (Any,Any)) ends up returning false on 2928, here:

julia/src/gf.c

Lines 2924 to 2931 in 12328e2

if (disjoint && lim >= 0) {
ndisjoint += 1;
if (ndisjoint > lim) {
JL_GC_POP();
return jl_false;
}
}
}

Hopefully this helps someone make sense of the failure? I couldn't quite make sense of it myself.

Thanks!

@NHDaly NHDaly added bug Indicates an unexpected problem or unintended behavior compiler:latency Compiler latency compiler:precompilation Precompilation of modules labels Dec 20, 2020
timholy added a commit that referenced this issue Aug 22, 2021
No code change is needed, but the use-case in that issue doesn't
appear to be tested directly.

Closes #38149
@timholy
Copy link
Member

timholy commented Aug 22, 2021

You can force both to be cached using the pattern in #41958. Just pick a signature that isn't covered by one of the more specific methods.

timholy added a commit that referenced this issue Aug 22, 2021
No code change is needed, but the use-case in that issue doesn't
appear to be tested directly.

Closes #38149
timholy added a commit that referenced this issue Aug 23, 2021
No code change is needed, but the use-case in that issue doesn't
appear to be tested directly.

Closes #38149
@NHDaly
Copy link
Member Author

NHDaly commented Sep 26, 2021

@timholy, I guess this doesn't exactly solve the issue here, because currently the trace-compile Precompile statement emitter emits precompile statements that look like the one I described in the issue.

So I guess we could either:

  • Change the precompile statement emitter to emit statements that use a non-matching type, as you've done in Add test for #38149 #41958. But this seems fragile, because the other methods could be defined later in the package compilation process, and it might end up colliding with one of those. I guess we could make up a type internal to the precompile statement emitter and use that?
  • Fix the implementation of precompile() so that it works for the case currently emitted.

Do you have a sense for which one of those would be easier? I'm going to re-open the issue for now, since I think the suggestion you've provided won't help until we do one of the above.

@NHDaly NHDaly reopened this Sep 26, 2021
@timholy timholy changed the title Precompile fails for nospecialize functions with multiple methods Precompile fails when multiple methods fit the supplied signature Sep 27, 2021
@timholy
Copy link
Member

timholy commented Sep 27, 2021

Probably the most robust approach would be to pick the least-specific method. Start checking here:

mi = jl_get_specialization1((jl_tupletype_t*)mi->specTypes, jl_world_counter, &min_world, &max_world, 0);

@NHDaly
Copy link
Member Author

NHDaly commented Jan 2, 2022

Thanks Tim. Just to confirm my understanding: that would be the approach to fix it in precompile() itself?

@vtjnash
Copy link
Member

vtjnash commented Jan 7, 2022

In principle, we should probably just iterate all matches, rather than getting exactly one match from jl_get_specialization1

LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Feb 22, 2022
No code change is needed, but the use-case in that issue doesn't
appear to be tested directly.

Closes JuliaLang#38149
LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Mar 8, 2022
No code change is needed, but the use-case in that issue doesn't
appear to be tested directly.

Closes JuliaLang#38149
@vtjnash vtjnash closed this as completed Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior compiler:latency Compiler latency compiler:precompilation Precompilation of modules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants