Fix serialization of code instances. #46373
Conversation
maleadt
added
compiler:precompilation
There was a problem hiding this comment.
This seems like a valid idea, but I'm wondering if we need to be more general about tracking how we got to jl_serialize_code_instance. I've not seen these before so I'm not sure whether MethodInstances could also be part of Julia objects, but presumably a package like
module MyPackage
const some_mis = [first(methods(sum)).specializations[1]]
endwould serve as a test case.
| jl_typeis(codeinst->rettype_const, jl_partial_opaque_type)) { | ||
| if (skip_partial_opaque) { | ||
| jl_serialize_code_instance(s, codeinst->next, skip_partial_opaque, internal); | ||
| jl_serialize_code_instance(s, codeinst->next, skip_partial_opaque, internal, 0); |
There was a problem hiding this comment.
I'm just preserving how this function worked before. Presumably there was a reason that jl_serialize_code_instance didn't serialize CIs with backref entries on recursive calls to jl_serialize_code_instance, so I'm just preserving that behavior. Instead, I'm only forcing the CIs to be serialized when I know that the backref table initialization doesn't mean that the object has been serialized, i.e., from the jl_is_code_instance branch in jl_serialize_value.
| jl_serialize_value(s, (jl_value_t*)backedges); | ||
| jl_serialize_value(s, (jl_value_t*)NULL); //callbacks | ||
| jl_serialize_code_instance(s, mi->cache, 1, internal); | ||
| jl_serialize_code_instance(s, mi->cache, 1, internal, 0); |
There was a problem hiding this comment.
What if a MethodInstance is a code-object? Would you want to serialize the CodeInstances that it points to? If so...we might have to keep track at a higher level of how we got here: if by traversing a module, then don't force, but if we're caching code, then force.
There was a problem hiding this comment.
Not setting force=0 doesn't mean that the CI won't be serialized, it only means that it can't have a backref table entry. Since we're calling jl_serialize_code_instance here on a field that wasn't the direct input to jl_serialize_value (which populates the backref table), IIUC that means we will either serialize it for the first time, or the backref table will have an entry point to a successfully serialized CI.
And this does indeed seem to work as expected. With your example above, I can successfully precompile the array of MIs and inspect the CI in the cache:
module CodeInstancePrecompile
const some_mis = [first(methods(identity)).specializations[1]]
__init__() = @show some_mis[].cache
end❯ JULIA_LOAD_PATH=. jl --startup-file=no -e 'using CodeInstancePrecompile'
(some_mis[]).cache = Core.CodeInstance(MethodInstance for identity(::Base.BottomRF{typeof(Base.add_sum)}), #undef, 0x0000000000000001, 0xffffffffffffffff, Base.BottomRF{typeof(Base.add_sum)}, Base.BottomRF{typeof(Base.add_sum)}(Base.add_sum), nothing, 0x00000155, 0x00000155, nothing, false, false, Ptr{Nothing} @0x00000001034d3a08, Ptr{Nothing} @0x0000000000000000, 0x00)
maleadt
force-pushed
the
tb/precompile_codeinstance
branch
from
57ede8f to
0930304
Compare
If they are encountered in a Julia object directly, as opposed to e.g. during serialization of a method instance, we'll have first populated the backref table as part of jl_serialize_value, so we need to skip that check during code instance serialization.
maleadt
force-pushed
the
tb/precompile_codeinstance
branch
from
0930304 to
31c0187
Compare
|
It is a confusing bit of code needing to track if we came via |
This reverts commit ff4f86d.
|
If a code instance is encountered in a Julia object directly, as opposed to e.g. during serialization of a method instance, we'll already have populated the backref table at the start of
jl_serialize_value. That meansjl_serialize_code_instancecan't just bail out, even though I presume that's still useful for nested calls. It would be cleaner to remove the check and calljl_serialize_valuefor nested code instances, but then the undocumentedinternalflag gets lost, so cc @Keno @timholy for thoughts on this.Fixes #46296
Introduced by #39512, so we could backport this all the way back to 1.7 if we want.