Fix OncePerThread issue when building a sysimage using the current sysimage #57656
Conversation
src/staticdata.c
Outdated
| @@ -3143,6 +3144,22 @@ static void jl_save_system_image_to_stream(ios_t *f, jl_array_t *mod_array, | |||
| } | |||
| jl_queue_for_serialization(&s, global_roots_list); | |||
| jl_queue_for_serialization(&s, global_roots_keyset); | |||
| precompile_field_replace_list = jl_alloc_svec(3); | |||
There was a problem hiding this comment.
Please add some comments explaining why this is necessary
| @@ -3129,7 +3130,7 @@ static void jl_save_system_image_to_stream(ios_t *f, jl_array_t *mod_array, | |||
| jl_queue_for_serialization(&s, s.method_roots_list); | |||
| jl_serialize_reachable(&s); | |||
| } | |||
| // step 1.4: prune (garbage collect) special weak references from the jl_global_roots_list | |||
| // step 1.4: prune (garbage collect) special weak references from the jl_global_roots_list and field replace list | |||
| if (worklist == NULL) { | |||
There was a problem hiding this comment.
Why doesn't this issue apply to pkgimages?
There was a problem hiding this comment.
pkgimages seal their own world so they their globals don't get compiled twice. The issue here is that on a normal sysimage you do inherit the world.
There was a problem hiding this comment.
Is it not possible to load a pkgimage (and its globals) when building a sysimage?
There was a problem hiding this comment.
Yes, but those changes don't get saved
There was a problem hiding this comment.
You mean that we copy the data from the pkgimage, instead of the in-memory objects?
I'm not sure that's true (or something else is going wrong). If you compile this package:
module OncePerFoo
const f = OncePerThread{Nothing}() do
println(Core.stdout, "Running thread init...")
end
__init__() = (f(); println(Core.stdout, "Hello World!"))
f() # Executed during pre-compilation
end # module OncePerFooand then compile with juliac.jl --trim=no the following script:
module MyApp
using OncePerFoo
Base.@ccallable function main()::Cint
OncePerFoo.f()
return 0
end
OncePerFoo.f() # fire init during compilation time
endthe OncePerThread does not fire at runtime, even with this PR applied:
$ ./onceperfoo
Hello World!There was a problem hiding this comment.
Oh this is an interesting case and is very tricky. I didn't think of the case where we use packageimages to build a system image. This means we have to merge these from different package images
There was a problem hiding this comment.
Yeah, either that or the the fields need to be marked as "sticky" in the sysimage (i.e. they are auto-replaced with their original value from whatever image they were loaded from)
It seems a bit laborious given that you have to compile multiple images, but the behavior seems 100% deterministic to me and very easy to trigger. Is the main problem that we don't have an incremental sysimage build in CI right now? |
|
Yeah, to test it you have to build an image with a once per thread in it. |
Both `--trim=no` and `--trim=safe` are supposed to be safe options by default, so let's not get overzealous and throw out all the IR even though we're not trimming Allows `juliac.jl` to use/test the sysimage creation pipeline unrelated to `--trim` support, as in #57656 (comment)
This is quite tricky to test unfortunately, but #57544 caught this and this fixes that