Skip to content

Commit

Permalink
account for extensions indirectly depending on each other in parallel…
Browse files Browse the repository at this point in the history
… package precompilation (#53972)

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
#53081 (comment):

```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).
  • Loading branch information
KristofferC committed May 9, 2024
1 parent 3920694 commit df89440
Showing 1 changed file with 49 additions and 7 deletions.
56 changes: 49 additions & 7 deletions base/precompilation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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"))
Expand All @@ -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
Expand Down

0 comments on commit df89440

Please sign in to comment.