-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Don't perform extra inference during incremental image creation #48054
Conversation
src/precompile_utils.c
Outdated
@@ -188,7 +188,7 @@ static int precompile_enq_specialization_(jl_method_instance_t *mi, void *closur | |||
(jl_ir_inlining_cost((jl_array_t*)inferred) == UINT16_MAX)) { | |||
do_compile = 1; | |||
} | |||
else if (jl_atomic_load_relaxed(&codeinst->invoke) != NULL || jl_atomic_load_relaxed(&codeinst->precompile)) { | |||
else if (jl_atomic_load_relaxed(&codeinst->precompile)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead we can check that invoke
is not set to jl_fptr_interpret_call
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would work for this case, but I don't really understand the purpose and semantics of the ->precompile
field then if we just basically ignore it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timholy wrote this particular section of code.
I'm planning to merge this today so Revise starts working again. Obviously if @timholy has alternative ideas for the correct criterion here, so should do that as a followup. |
8c71279
to
756db23
Compare
Looks like the condition here isn't quite enough.
|
Looks like @vtjnash was right and we just can't have that case at all (if we want it, we need to have it do re-inference earlier). |
Sigh, we have a test that fails when some of these get removed from the sysimage. I'm just gonna turn off the warning I added here, put back the logic that adds them to the list and then rely on the check I added at the inference entry to avoid having it actually do the inference. Not pretty, but should fix the bug and hopefully @vtjnash or @timholy will fix this properly soon. |
As noted in #48047, we're currently attempting to infer extra methods during incremental image saving, which causes us to miss edges in the image. In particular, in the case of #48047, Cthulhu had the `compile=min` option set, which caused the code instance for `do_typeinf!` to not be infered. However, later it was nevertheless queued for precompilation, causing inference to occur at an inopportune time. This PR simply prevents code instances that don't explicitly have the `->precompile` flag set (e.g. the guard instance created for the interpreter) from being enqueued for precompilation. It is not clear that this is necessarily the correct behavior - we may in fact want to infer these method instances, just before we set up the serializer state, but for now this fixes #48047 for me. I also included an appropriate test and a warning message if we attempt to enter inference when this is not legal, so any revisit of what should be happening here can hopefully make use of those.
Sorry about this. Until today, for the last few months I've been ignoring my gmail (GitHub notifications), and only just saw this. I think the code in question is old, for example here it is in 1.8: Lines 288 to 311 in 0efc765
It seems to be identical to what you started with before this PR. I think the big difference is that we're calling it from more paths now than we used to be. And probably at a later stage: I moved the time-of-call to after we'd done the system traversal to make sure that the native code for external methods (methods we add to pre-existing modules) and external specializations (new specializations of methods we don't own) also got written, so I was just trying to re-use the work we had already done on system traversal. In rough terms, that moved the call location from somewhere around here: Lines 108 to 116 in dc2b4d9
to now being called from here: Line 2628 in 0913cbc
The intent is simply to cache "what we've got." I think anything that has Does that provide enough of a roadmap to know how to fix this? |
After that system traversal, any code execution will invalidate all of that result, since the system has changed |
I think we're agreeing, but I'm not certain I know what you mean. What execution are you referring to? To clarify, my point is that we shouldn't do any additional inference; if we discover anything that needs inference before we can pass it to LLVM, we shouldn't infer and we shouldn't pass to LLVM. The only reason to pass anything through LLVM (again) is because we need to generate a clean LLVM module for caching, but if we didn't need to do that then I'd say "no more compilation at all." The flag on the |
That is fine, but you have to re-run |
So are you proposing basically that we call Understand that because of my LLVM-ignorance I am a little vague about what the various code/opaque-pointer fields of a |
Ah shoot, I need to correct one thing: https://github.com/JuliaLang/julia/blame/db00cc1a8455ceb4a8dc3cb8dd6ded3d62e46dcb/src/julia.h#L372 vs https://github.com/JuliaLang/julia/blame/f8d3bd22f68f10bd2dc3f1be3b74bfa7cb125bc5/src/julia.h#L387 The one I added (to |
As noted in #48047, we're currently attempting to infer extra methods during incremental image saving, which causes us to miss edges in the image. In particular, in the case of #48047, Cthulhu had the `compile=min` option set, which caused the code instance for `do_typeinf!` to not be infered. However, later it was nevertheless queued for precompilation, causing inference to occur at an inopportune time. This PR simply prevents code instances that don't explicitly have the `->precompile` flag set (e.g. the guard instance created for the interpreter) from being enqueued for precompilation. It is not clear that this is necessarily the correct behavior - we may in fact want to infer these method instances, just before we set up the serializer state, but for now this fixes #48047 for me. I also included an appropriate test and a warning message if we attempt to enter inference when this is not legal, so any revisit of what should be happening here can hopefully make use of those. (cherry picked from commit 80aeebe)
As noted in #48047, we're currently attempting to infer extra methods during incremental image saving, which causes us to miss edges in the image. In particular, in the case of #48047, Cthulhu had the
compile=min
option set, which caused the code instance fordo_typeinf!
to not be infered. However, later it was nevertheless queued for precompilation, causing inference to occur at an inopportune time.This PR simply prevents code instances that don't explicitly have the
->precompile
flag set (e.g. the guard instance created for the interpreter) from being enqueued for precompilation. It is not clear that this is necessarily the correct behavior - we may in fact want to infer these method instances, just before we set up the serializer state, but for now this fixes #48047 for me.I also included an appropriate test and a warning message if we attempt to enter inference when this is not legal, so any revisit of what should be happening here can hopefully make use of those.