improve precompilation coverage (#33006)

- move the place where --trace-compile outputs precompile statement to a location that catches more cases
- tweak the REPL code to be more amenable to precompilation in light of
- instead of trying to encode all the rules where the precompile emitter
  fails (#28808) just try to precompile and do nothing if it fails.

Author: KristofferC

contrib/generate_precompile.jl

@@ -142,12 +142,6 @@ function generate_precompile_statements()
             push!(statements, statement)
         end
 
-        if have_repl
-            # Seems like a reasonable number right now, adjust as needed
-            # comment out if debugging script
-            @assert length(statements) > 700
-        end
-
         # Create a staging area where all the loaded packages are available
         PrecompileStagingArea = Module()
         for (_pkgid, _mod) in Base.loaded_modules
@@ -157,22 +151,23 @@ function generate_precompile_statements()
         end
 
         # Execute the collected precompile statements
+        n_succeeded = 0
         include_time = @elapsed for statement in sort(collect(statements))
             # println(statement)
-            # Workarounds for #28808
-            occursin("\"YYYY-mm-dd\\THH:MM:SS\"", statement) && continue
-            statement == "precompile(Tuple{typeof(Base.show), Base.IOContext{Base.TTY}, Type{Vararg{Any, N} where N}})" && continue
-            # check for `#x##s66` style variable names not in quotes
-            occursin(r"#\w", statement) &&
-                count(r"(?:#+\w+)+", statement) !=
-                count(r"\"(?:#+\w+)+\"", statement) && continue
             try
                 Base.include_string(PrecompileStagingArea, statement)
+                n_succeeded += 1
             catch
-                @error "Failed to precompile $statement"
-                rethrow()
+                # See #28808
+                # @error "Failed to precompile $statement"
             end
         end
+        if have_repl
+            # Seems like a reasonable number right now, adjust as needed
+            # comment out if debugging script
+            @assert n_succeeded > 3500
+        end
+
         print(" $(length(statements)) generated in ")
         tot_time = time() - start_time
         Base.time_print(tot_time * 10^9)

src/codegen.cpp

@@ -1042,6 +1042,10 @@ const char *name_from_method_instance(jl_method_instance_t *mi)
 extern "C"
 jl_code_instance_t *jl_compile_linfo(jl_method_instance_t *mi, jl_code_info_t *src, size_t world, const jl_cgparams_t *params)
 {
+    // TODO: Merge with jl_dump_compiles?
+    static ios_t f_precompile;
+    static JL_STREAM* s_precompile = NULL;
+
     // N.B.: `src` may have not been rooted by the caller.
     JL_TIMING(CODEGEN);
     assert(jl_is_method_instance(mi));
@@ -1266,12 +1270,34 @@ jl_code_instance_t *jl_compile_linfo(jl_method_instance_t *mi, jl_code_info_t *s
     // ... unless mi->def isn't defined here meaning the function is a toplevel thunk and
     // would have its CodeInfo printed in the stream, which might contain double-quotes that
     // would not be properly escaped given the double-quotes added to the stream below.
-    if (dump_compiles_stream != NULL && jl_is_method(mi->def.method)) {
-        uint64_t this_time = jl_hrtime();
-        jl_printf(dump_compiles_stream, "%" PRIu64 "\t\"", this_time - last_time);
-        jl_static_show(dump_compiles_stream, mi->specTypes);
-        jl_printf(dump_compiles_stream, "\"\n");
-        last_time = this_time;
+    if (jl_is_method(mi->def.method)) {
+        if (jl_options.trace_compile != NULL) {
+            if (s_precompile == NULL) {
+                const char* t = jl_options.trace_compile;
+                if (!strncmp(t, "stderr", 6))
+                    s_precompile = JL_STDERR;
+                else {
+                    if (ios_file(&f_precompile, t, 1, 1, 1, 1) == NULL)
+                        jl_errorf("cannot open precompile statement file \"%s\" for writing", t);
+                    s_precompile = (JL_STREAM*) &f_precompile;
+                }
+            }
+            if (!jl_has_free_typevars(mi->specTypes)) {
+                jl_printf(s_precompile, "precompile(");
+                jl_static_show(s_precompile, mi->specTypes);
+                jl_printf(s_precompile, ")\n");
+
+                if (s_precompile != JL_STDERR)
+                    ios_flush(&f_precompile);
+            }
+        }
+        if (dump_compiles_stream != NULL) {
+            uint64_t this_time = jl_hrtime();
+            jl_printf(dump_compiles_stream, "%" PRIu64 "\t\"", this_time - last_time);
+            jl_static_show(dump_compiles_stream, mi->specTypes);
+            jl_printf(dump_compiles_stream, "\"\n");
+            last_time = this_time;
+        }
     }
     JL_GC_POP();
     return codeinst;

src/gf.c

@@ -1079,10 +1079,6 @@ static jl_typemap_entry_t *jl_typemap_morespecific_by_type(jl_typemap_entry_t *f
 
 static jl_method_instance_t *jl_mt_assoc_by_type(jl_methtable_t *mt, jl_datatype_t *tt, int mt_cache, size_t world)
 {
-    // TODO: Merge with jl_dump_compiles?
-    static ios_t f_precompile;
-    static JL_STREAM* s_precompile = NULL;
-
     // caller must hold the mt->writelock
     jl_typemap_entry_t *entry = NULL;
     entry = jl_typemap_assoc_by_type(mt->cache, (jl_value_t*)tt, NULL, /*subtype*/1, jl_cachearg_offset(mt), world, /*max_world_mask*/0);
@@ -1099,26 +1095,6 @@ static jl_method_instance_t *jl_mt_assoc_by_type(jl_methtable_t *mt, jl_datatype
         entry = jl_typemap_morespecific_by_type(entry, (jl_value_t*)tt, &env, world);
         if (entry != NULL) {
             jl_method_t *m = entry->func.method;
-            if (jl_options.trace_compile != NULL) {
-                if (s_precompile == NULL) {
-                    const char* t = jl_options.trace_compile;
-                    if (!strncmp(t, "stderr", 6))
-                        s_precompile = JL_STDERR;
-                    else {
-                        if (ios_file(&f_precompile, t, 1, 1, 1, 1) == NULL)
-                            jl_errorf("cannot open precompile statement file \"%s\" for writing", t);
-                        s_precompile = (JL_STREAM*) &f_precompile;
-                    }
-                }
-                if (!jl_has_free_typevars((jl_value_t*)tt)) {
-                    jl_printf(s_precompile, "precompile(");
-                    jl_static_show(s_precompile, (jl_value_t*)tt);
-                    jl_printf(s_precompile, ")\n");
-
-                    if (s_precompile != JL_STDERR)
-                        ios_flush(&f_precompile);
-                }
-            }
             if (!mt_cache) {
                 intptr_t nspec = (mt == jl_type_type_mt ? m->nargs + 1 : mt->max_args + 2);
                 jl_compilation_sig(tt, env, m, nspec, &newparams);

stdlib/REPL/src/REPL.jl

@@ -765,15 +765,21 @@ setup_interface(
     repl::LineEditREPL;
     # those keyword arguments may be deprecated eventually in favor of the Options mechanism
     hascolor::Bool = repl.options.hascolor,
-    extra_repl_keymap::Union{Dict,Vector{<:Dict}} = repl.options.extra_keymap
+    extra_repl_keymap::Any = repl.options.extra_keymap
 ) = setup_interface(repl, hascolor, extra_repl_keymap)
 
 # This non keyword method can be precompiled which is important
 function setup_interface(
     repl::LineEditREPL,
     hascolor::Bool,
-    extra_repl_keymap::Union{Dict,Vector{<:Dict}},
+    extra_repl_keymap::Any, # Union{Dict,Vector{<:Dict}},
 )
+    # The precompile statement emitter has problem outputting valid syntax for the
+    # type of `Union{Dict,Vector{<:Dict}}` (see #28808).
+    # This function is however important to precompile for REPL startup time, therefore,
+    # make the type Any and just assert that we have the correct type below.
+    @assert extra_repl_keymap isa Union{Dict,Vector{<:Dict}}
+
     ###
     #
     # This function returns the main interface that describes the REPL