Skip to content

Commit

Permalink
Don't perform extra inference during incremental image creation (#48054)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Keno authored Jan 5, 2023
1 parent a9506f5 commit 80aeebe
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 2 deletions.
4 changes: 4 additions & 0 deletions base/Base.jl
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,10 @@ for m in methods(include)
delete_method(m)
end

# This method is here only to be overwritten during the test suite to test
# various sysimg related invalidation scenarios.
a_method_to_overwrite_in_test() = inferencebarrier(1)

# These functions are duplicated in client.jl/include(::String) for
# nicer stacktraces. Modifications here have to be backported there
include(mod::Module, _path::AbstractString) = _include(identity, mod, _path)
Expand Down
7 changes: 7 additions & 0 deletions src/gf.c
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,13 @@ jl_code_info_t *jl_type_infer(jl_method_instance_t *mi, size_t world, int force)
if (jl_typeinf_func == NULL)
return NULL;
jl_task_t *ct = jl_current_task;
if (ct->reentrant_inference == (uint16_t)-1) {
// TODO: We should avoid attempting to re-inter inference here at all
// and turn on this warning, but that requires further refactoring
// of the precompile code, so for now just catch that case here.
//jl_printf(JL_STDERR, "ERROR: Attempted to enter inference while writing out image.");
return NULL;
}
if (ct->reentrant_inference > 2)
return NULL;

Expand Down
11 changes: 9 additions & 2 deletions src/staticdata.c
Original file line number Diff line number Diff line change
Expand Up @@ -2614,7 +2614,12 @@ JL_DLLEXPORT void jl_create_system_image(void **_native_data, jl_array_t *workli
} else {
checksumpos_ff = checksumpos;
}
jl_gc_enable_finalizers(ct, 0); // make sure we don't run any Julia code concurrently after this point
{
// make sure we don't run any Julia code concurrently after this point
jl_gc_enable_finalizers(ct, 0);
assert(ct->reentrant_inference == 0);
ct->reentrant_inference = (uint16_t)-1;
}
jl_prepare_serialization_data(mod_array, newly_inferred, jl_worklist_key(worklist), &extext_methods, &new_specializations, &method_roots_list, &ext_targets, &edges);

// Generate _native_data`
Expand All @@ -2638,7 +2643,9 @@ JL_DLLEXPORT void jl_create_system_image(void **_native_data, jl_array_t *workli
jl_save_system_image_to_stream(ff, worklist, extext_methods, new_specializations, method_roots_list, ext_targets, edges);
native_functions = NULL;
if (worklist) {
jl_gc_enable_finalizers(ct, 1); // make sure we don't run any Julia code concurrently before this point
// Re-enable running julia code for postoutput hooks, atexit, etc.
jl_gc_enable_finalizers(ct, 1);
ct->reentrant_inference = 0;
jl_precompile_toplevel_module = NULL;
}

Expand Down
17 changes: 17 additions & 0 deletions test/precompile.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1691,6 +1691,23 @@ precompile_test_harness("DynamicExpressions") do load_path
end
end

precompile_test_harness("BadInvalidations") do load_path
write(joinpath(load_path, "BadInvalidations.jl"),
"""
module BadInvalidations
Base.Experimental.@compiler_options compile=min optimize=1
getval() = Base.a_method_to_overwrite_in_test()
getval()
end # module BadInvalidations
""")
Base.compilecache(Base.PkgId("BadInvalidations"))
(@eval Base a_method_to_overwrite_in_test() = inferencebarrier(2))
(@eval (using BadInvalidations))
Base.invokelatest() do
@test BadInvalidations.getval() === 2
end
end

empty!(Base.DEPOT_PATH)
append!(Base.DEPOT_PATH, original_depot_path)
empty!(Base.LOAD_PATH)
Expand Down

0 comments on commit 80aeebe

Please sign in to comment.