From df89440d7608f93ec32363a6e21db6396dbe1da9 Mon Sep 17 00:00:00 2001 From: Kristoffer Carlsson Date: Thu, 9 May 2024 12:43:03 +0200 Subject: [PATCH] account for extensions indirectly depending on each other in parallel package precompilation (#53972) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the parallel package precompilation code we are mapping what packages depend on what other packages so that we precompile things in the correct order ("bottom up") and so what we can also detect cycles and avoid precompiling packages in those cycles. However, we fail to detect one type of dependency which is that an extension can "indirectly" depend on another extension. This happens when the transitive dependencies of the extension (it's parent + triggers) are a superset of the dependencies of another extension. In other words, this means that the other extension will get loaded into the first extension once it gets loaded, effectively being a dependency. The failure to model this leads to some issues, for example using one of the examples in our own tests: ```julia julia> Base.active_project() "/home/kc/julia/test/project/Extensions/HasDepWithExtensions.jl/Project.toml" (HasDepWithExtensions) pkg> status --extensions Project HasDepWithExtensions v0.1.0 Status `~/julia/test/project/Extensions/HasDepWithExtensions.jl/Project.toml` [4d3288b3] HasExtensions v0.1.0 `../HasExtensions.jl` ├─ ExtensionFolder [ExtDep, ExtDep2] ├─ Extension [ExtDep] └─ LinearAlgebraExt [LinearAlgebra] julia> Base.Precompilation.precompilepkgs(; timing=true) Precompiling all packages... 196.1 ms ✓ HasExtensions 244.4 ms ✓ ExtDep2 207.9 ms ✓ SomePackage 201.6 ms ✓ ExtDep 462.5 ms ✓ HasExtensions → ExtensionFolder 200.1 ms ✓ HasExtensions → Extension 173.1 ms ✓ HasExtensions → LinearAlgebraExt 222.2 ms ✓ HasDepWithExtensions 8 dependencies successfully precompiled in 2 seconds julia> Base.Precompilation.precompilepkgs(; timing=true) Precompiling all packages... 213.4 ms ✓ HasExtensions → ExtensionFolder 1 dependency successfully precompiled in 0 seconds. 7 already precompiled. julia> Base.Precompilation.precompilepkgs(; timing=true) julia> ``` We can see here that `ExtensionFolder` gets precompiled twice which is due to `Extension` actually being an "indirect dependency" of `ExtensionFolder` and therefore should be precompiled before it. With this PR we instead get: ```julia julia> Precompilation.precompilepkgs(; timing=true) Precompiling all packages... 347.5 ms ✓ ExtDep2 294.0 ms ✓ SomePackage 293.2 ms ✓ HasExtensions 216.5 ms ✓ HasExtensions → LinearAlgebraExt 554.9 ms ✓ ExtDep 580.9 ms ✓ HasExtensions → Extension 593.8 ms ✓ HasExtensions → ExtensionFolder 261.3 ms ✓ HasDepWithExtensions 8 dependencies successfully precompiled in 2 seconds julia> Precompilation.precompilepkgs(; timing=true) julia> ``` `Extension` is precompiled after `ExtensionFolder` and nothing happens on the second call. Also, with this PR we get for the issue in https://github.com/JuliaLang/julia/issues/53081#issue-2103686391: ```julia (jl_zuuRGt) pkg> st Status `/private/var/folders/tp/2p4x9ygx48sgsdl1ccg1mp_40000gn/T/jl_zuuRGt/Project.toml` ⌃ [d236fae5] PreallocationTools v0.4.17 ⌃ [0c5d862f] Symbolics v5.16.1 Info Packages marked with ⌃ have new versions available and may be upgradable. julia> Precompilation.precompilepkgs(; timing=true) ┌ Warning: Circular dependency detected. Precompilation will be skipped for: │ SymbolicsPreallocationToolsExt │ SymbolicsForwardDiffExt ``` and we avoid precompiling the problematic extensions. This should also allow extensions to precompile in parallel which I think was prevented before (from the code that is removed in the beginning of the diff). --- base/precompilation.jl | 56 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 49 insertions(+), 7 deletions(-) diff --git a/base/precompilation.jl b/base/precompilation.jl index 9acbd2a11f202..8e95939bc1008 100644 --- a/base/precompilation.jl +++ b/base/precompilation.jl @@ -402,7 +402,6 @@ function precompilepkgs(pkgs::Vector{String}=String[]; depsmap[pkg] = filter!(!Base.in_sysimage, deps) # add any extensions pkg_exts = Dict{Base.PkgId, Vector{Base.PkgId}}() - prev_ext = nothing for (ext_name, extdep_uuids) in env.extensions[dep] ext_deps = Base.PkgId[] push!(ext_deps, pkg) # depends on parent package @@ -417,13 +416,8 @@ function precompilepkgs(pkgs::Vector{String}=String[]; end end all_extdeps_available || continue - if prev_ext isa Base.PkgId - # also make the exts depend on eachother sequentially to avoid race - push!(ext_deps, prev_ext) - end ext_uuid = Base.uuid5(pkg.uuid, ext_name) ext = Base.PkgId(ext_uuid, ext_name) - prev_ext = ext filter!(!Base.in_sysimage, ext_deps) depsmap[ext] = ext_deps exts[ext] = pkg.name @@ -434,6 +428,51 @@ function precompilepkgs(pkgs::Vector{String}=String[]; end end + # An extension effectively depends on another extension if it has all the the + # dependencies of that other extension + function expand_dependencies(depsmap) + function visit!(visited, node, all_deps) + if node in visited + return + end + push!(visited, node) + for dep in get(Set{Base.PkgId}, depsmap, node) + if !(dep in all_deps) + push!(all_deps, dep) + visit!(visited, dep, all_deps) + end + end + end + + depsmap_transitive = Dict{Base.PkgId, Set{Base.PkgId}}() + for package in keys(depsmap) + # Initialize a set to keep track of all dependencies for 'package' + all_deps = Set{Base.PkgId}() + visited = Set{Base.PkgId}() + visit!(visited, package, all_deps) + # Update depsmap with the complete set of dependencies for 'package' + depsmap_transitive[package] = all_deps + end + return depsmap_transitive + end + + depsmap_transitive = expand_dependencies(depsmap) + + for (_, extensions_1) in pkg_exts_map + for extension_1 in extensions_1 + deps_ext_1 = depsmap_transitive[extension_1] + for (_, extensions_2) in pkg_exts_map + for extension_2 in extensions_2 + extension_1 == extension_2 && continue + deps_ext_2 = depsmap_transitive[extension_2] + if issubset(deps_ext_2, deps_ext_1) + push!(depsmap[extension_1], extension_2) + end + end + end + end + end + @debug "precompile: deps collected" # this loop must be run after the full depsmap has been populated for (pkg, pkg_exts) in pkg_exts_map @@ -748,6 +787,7 @@ function precompilepkgs(pkgs::Vector{String}=String[]; end @debug "precompile: starting precompilation loop" depsmap direct_deps ## precompilation loop + for (pkg, deps) in depsmap cachepaths = Base.find_all_in_cache_path(pkg) sourcepath = Base.locate_package(pkg) @@ -820,6 +860,7 @@ function precompilepkgs(pkgs::Vector{String}=String[]; end loaded && (n_loaded += 1) catch err + # @show err close(std_pipe.in) # close pipe to end the std output monitor wait(t_monitor) if err isa ErrorException || (err isa ArgumentError && startswith(err.msg, "Invalid header in cache file")) @@ -843,7 +884,8 @@ function precompilepkgs(pkgs::Vector{String}=String[]; notify(was_processed[pkg_config]) catch err_outer # For debugging: - # println("Task failed $err_outer") # logging doesn't show here + # println("Task failed $err_outer") + # Base.display_error(ErrorException(""), Base.catch_backtrace())# logging doesn't show here handle_interrupt(err_outer) || rethrow() notify(was_processed[pkg_config]) finally