Skip to content

Conversation

timholy
Copy link
Member

@timholy timholy commented Jan 11, 2022

During incremental compilation (aka, package precompilation),
wait to call jl_compress_ir until the moment of method
serialization. This increases the efficiency of potential
root-order transformations (e.g., #42016). Aside from
possible memory constraints, there appears to be little downside
to waiting.

This is a small piece of #42016, split out to test whether it can be safely done on its own. If folks want, we can disable the defensive decompression portion of this PR, but defensive decompression willmay be necessary for #42016. So I wanted to start with the full shebang.

Here's where root-reordering is potentially useful:

module PkgA
f(args...) = ...
end

module PkgB
using PkgA
# code that add more roots to PkgA.f ("B roots")
end

module PkgC
using PkgA
# run some code that adds roots to PkgA.f   ("C roots")
using PkgB   # this adds more roots to PkgA.f  ("B roots")
# run yet more code that adds yet more roots to PkgA.f   (more "C roots")
end

If we want to encode the package-of-origin for each root, root-reordering improves the efficiency of run-length encoding by enabling one to group the new roots added by PkgC into a single block. It may not be strictly necessary: we could have a run of PkgC roots, followed by a run of PkgB roots, followed by more PkgC roots. As long as we tag the roots by both module id and within-module count, then we can disambiguate them. But to me it seems to make sense to do a bit of work during serialization to make the code that runs during deserialization faster and simpler.

Since most roots seem to arise from compression (rather than codegen), just this one change already goes much of the way---we may not need additional reorderings.

During incremental compilation (aka, package precompilation),
wait to call `jl_compress_ir` until the moment of method
serialization. This increases the efficiency of potential
root-order transformations (e.g., #42016).  Aside from
possible memory constraints, there appears to be little downside
to waiting.
@timholy
Copy link
Member Author

timholy commented Jan 11, 2022

Out of curiosity, I added this:

diff --git a/src/dump.c b/src/dump.c
index 3999702ed0..d2aca57140 100644
--- a/src/dump.c
+++ b/src/dump.c
@@ -490,8 +490,10 @@ static void uncompress_code(jl_method_instance_t *mi)
     assert(jl_is_method(m));
     jl_code_instance_t *codeinst = mi->cache;
     while (codeinst) {
-        if (codeinst->inferred && jl_is_array(codeinst->inferred))
+        if (codeinst->inferred && jl_is_array(codeinst->inferred)) {
+            jl_(mi);
             codeinst->inferred = (jl_value_t*)jl_uncompress_ir(m, codeinst, (jl_array_t*)codeinst->inferred);
+        }
         codeinst = codeinst->next;
     }
 }
@@ -691,8 +693,10 @@ static void jl_serialize_value_(jl_serializer_state *s, jl_value_t *v, int as_li
             // Compression must occur before roots are serialized.
             // However, because jl_compress_ir is externally callable, first check
             // whether we need to decompress. This enables root-reordering transformations.
-            if (m->source && jl_is_array(m->source))
+            if (m->source && jl_is_array(m->source)) {
+                jl_(m);
                 m->source = (jl_value_t*)jl_uncompress_ir(m, NULL, (jl_array_t*)m->source);
+            }
             size_t l = jl_svec_len(m->specializations);
             for (i = 0; i < l; i++) {
                 jl_method_instance_t *mi = (jl_method_instance_t*)jl_svecref(m->specializations, i);

and was able to get through both Revise and Makie precompilation without any warnings.

@timholy
Copy link
Member Author

timholy commented Jan 11, 2022

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@timholy
Copy link
Member Author

timholy commented Jan 11, 2022

@nanosoldier runtests(["AdvancedHMC", "Andes", "AprilTags", "BlobTracking", "CopEnt", "CropRootBox", "Cropbox", "Crystallography", "DelayEmbeddings", "FSInteraction", "Garlic", "Hecke", "IntensityScans", "LeafGasExchange", "Manifolds", "MarkableIntegers", "MatrixPencils", "NumericalAlgorithms", "OutlierDetectionData", "QuantumPropagators", "RemoveLFS", "ReservoirComputing", "SimpleCrop", "SnowyOwl", "SpeechFeatures", "UpROOT", "UpdateJulia"], vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@timholy
Copy link
Member Author

timholy commented Jan 12, 2022

All failures except one are due to JuliaData/SplitApplyCombine.jl#47. It's possible this PR produces more dire outcomes in cases of "incremental compilation may be fatally broken" warnings, is that a concern?

Likewise, the MarkableIntegers failure is due to https://github.com/JeffreySarnoff/MarkableIntegers.jl/blob/9f147cfa7c4c55d5604fa071d50d61ddd7534054/src/promote.jl#L163. Interestingly,

diff --git a/src/gf.c b/src/gf.c
index 27b19e50c8..1a62caaebe 100644
--- a/src/gf.c
+++ b/src/gf.c
@@ -333,6 +333,9 @@ JL_DLLEXPORT jl_value_t *jl_rettype_inferred(jl_method_instance_t *mi, size_t mi
     while (codeinst) {
         if (codeinst->min_world <= min_world && max_world <= codeinst->max_world) {
             jl_value_t *code = codeinst->inferred;
+            if (code && code != jl_nothing && !jl_is_code_info(code) && !jl_typeis(code, jl_array_uint8_type)) {
+                jl_(mi);
+            }
             if (code && (code == jl_nothing || jl_ir_flag_inferred((jl_array_t*)code)))
                 return (jl_value_t*)codeinst;
         }
diff --git a/src/ircode.c b/src/ircode.c
index 279e458728..84410559f4 100644
--- a/src/ircode.c
+++ b/src/ircode.c
@@ -850,6 +850,8 @@ JL_DLLEXPORT uint8_t jl_ir_flag_inferred(jl_array_t *data)
 {
     if (jl_is_code_info(data))
         return ((jl_code_info_t*)data)->inferred;
+    if (!jl_typeis(data, jl_array_uint8_type))
+        jl_(data);
     assert(jl_typeis(data, jl_array_uint8_type));
     jl_code_info_flags_t flags;
     flags.packed = ((uint8_t*)data->data)[0];

yields

julia> using MarkableIntegers
[ Info: Precompiling MarkableIntegers [0913cafa-90c8-523e-aa68-da9dd2ac225d]
convert(Type{UInt64}, UInt64) from convert(Type{UInt64}, UInt64)
<?#0x7feb287b0130::0>
julia-debug: /home/tim/src/julia-branch/src/ircode.c:855: ijl_ir_flag_inferred: Assertion `jl_typeis(data, jl_array_uint8_type)' failed.

signal (6): Aborted
in expression starting at none:0
gsignal at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
abort at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x7feb30a99489)
__assert_fail at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
ijl_ir_flag_inferred at /home/tim/src/julia-branch/src/ircode.c:855
ijl_rettype_inferred at /home/tim/src/julia-branch/src/gf.c:339
get at ./compiler/cicache.jl:56 [inlined]
typeinf_edge at ./compiler/typeinfer.jl:776
...

in a julia debug build. I'm not sure I've seen a type 0 before, not entirely sure what that suggests.

@rikhuijzer
Copy link
Contributor

rikhuijzer commented Jan 12, 2022

uncompress_code and compress_code contain a lot of code duplication. That part would be more readable if the duplication can be reduced

@IanButterworth
Copy link
Member

I gave this a spin (with a rebase) vs. master on a slow loading package with many deps.
Happy to report it loaded and looked to be passing package tests.

Master 8997a90

julia> @time Pkg.precompile()
Precompiling project...
  264 dependencies successfully precompiled in 80 seconds
 80.559884 seconds ...

julia> @time using Foo
 18.791561 seconds (56.26 M allocations: 3.370 GiB, 6.73% gc time, 44.37% compilation time)

This PR (rebased on the same master)

julia> @time Pkg.precompile()
Precompiling project...
  264 dependencies successfully precompiled in 80 seconds
 80.129397 seconds ...

julia> @time using Foo
 18.022423 seconds (56.07 M allocations: 3.366 GiB, 6.00% gc time, 45.92% compilation time)

Seems like some benefit at both precompile time and load time? I just expected the former?

@timholy
Copy link
Member Author

timholy commented Feb 15, 2022

This isn't needed anymore (#43990).

@timholy timholy closed this Feb 15, 2022
@timholy timholy deleted the teh/defer_compression_incremental branch February 15, 2022 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants