Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

precompilepkgs: respect loaded dependencies when precompiling for load #56901

Merged
merged 3 commits into from
Dec 30, 2024
Merged

precompilepkgs: respect loaded dependencies when precompiling for load #56901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
310a989
precompilepkgs: respect loaded dependencies when precompiling for load
IanButterworth Dec 23, 2024
a7ca209
explain that further precompilation may be hit due to loaded deps
IanButterworth Dec 28, 2024
9a10434
improve wording
IanButterworth Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions base/loading.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1828,7 +1828,7 @@ function compilecache_path(pkg::PkgId;
path = nothing
isnothing(sourcepath) && error("Cannot locate source for $(repr("text/plain", pkg))")
for path_to_try in cachepaths
staledeps = stale_cachefile(sourcepath, path_to_try, ignore_loaded = true, requested_flags=flags)
staledeps = stale_cachefile(sourcepath, path_to_try; ignore_loaded, requested_flags=flags)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this was to allow the user to compute the best pkg to load for compilecache_path, even if it is currently loaded at a different version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe it was just missed originally? c24136d

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could've been. It's not mentioned in the PR #50218

if staledeps === true
continue
end
Expand Down Expand Up @@ -2649,7 +2649,7 @@ function __require_prelocked(pkg::PkgId, env)
parallel_precompile_attempted = true
unlock(require_lock)
try
Precompilation.precompilepkgs([pkg.name]; _from_loading=true)
Precompilation.precompilepkgs([pkg.name]; _from_loading=true, ignore_loaded=false)
finally
lock(require_lock)
end
Expand Down
24 changes: 14 additions & 10 deletions base/precompilation.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,11 +418,12 @@ function precompilepkgs(pkgs::Vector{String}=String[];
io::IO=stderr,
# asking for timing disables fancy mode, as timing is shown in non-fancy mode
fancyprint::Bool = can_fancyprint(io) && !timing,
manifest::Bool=false,)
manifest::Bool=false,
ignore_loaded::Bool=true)
# monomorphize this to avoid latency problems
_precompilepkgs(pkgs, internal_call, strict, warn_loaded, timing, _from_loading,
configs isa Vector{Config} ? configs : [configs],
IOContext{IO}(io), fancyprint, manifest)
IOContext{IO}(io), fancyprint, manifest, ignore_loaded)
end

function _precompilepkgs(pkgs::Vector{String},
Expand All @@ -434,7 +435,8 @@ function _precompilepkgs(pkgs::Vector{String},
configs::Vector{Config},
io::IOContext{IO},
fancyprint::Bool,
manifest::Bool)
manifest::Bool,
ignore_loaded::Bool)
requested_pkgs = copy(pkgs) # for understanding user intent

time_start = time_ns()
Expand Down Expand Up @@ -918,7 +920,7 @@ function _precompilepkgs(pkgs::Vector{String},
wait(was_processed[(dep,config)])
end
circular = pkg in circular_deps
is_stale = !Base.isprecompiled(pkg; ignore_loaded=true, stale_cache, cachepath_cache, cachepaths, sourcepath, flags=cacheflags)
is_stale = !Base.isprecompiled(pkg; ignore_loaded, stale_cache, cachepath_cache, cachepaths, sourcepath, flags=cacheflags)
if !circular && is_stale
Base.acquire(parallel_limiter)
is_project_dep = pkg in project_deps
Expand All @@ -944,10 +946,10 @@ function _precompilepkgs(pkgs::Vector{String},
try
# allows processes to wait if another process is precompiling a given package to
# a functionally identical package cache (except for preferences, which may differ)
t = @elapsed ret = precompile_pkgs_maybe_cachefile_lock(io, print_lock, fancyprint, pkg_config, pkgspidlocked, hascolor, parallel_limiter) do
t = @elapsed ret = precompile_pkgs_maybe_cachefile_lock(io, print_lock, fancyprint, pkg_config, pkgspidlocked, hascolor, parallel_limiter, ignore_loaded) do
Base.with_logger(Base.NullLogger()) do
# The false here means we ignore loaded modules, so precompile for a fresh session
keep_loaded_modules = false
# whether to respect already loaded dependency versions
keep_loaded_modules = !ignore_loaded
# for extensions, any extension in our direct dependencies is one we have a right to load
# for packages, we may load any extension (all possible triggers are accounted for above)
loadable_exts = haskey(ext_to_parent, pkg) ? filter((dep)->haskey(ext_to_parent, dep), direct_deps[pkg]) : nothing
Expand Down Expand Up @@ -1037,9 +1039,11 @@ function _precompilepkgs(pkgs::Vector{String},
plural1 = length(configs) > 1 ? "dependency configurations" : n_loaded == 1 ? "dependency" : "dependencies"
plural2 = n_loaded == 1 ? "a different version is" : "different versions are"
plural3 = n_loaded == 1 ? "" : "s"
plural4 = n_loaded == 1 ? "this package" : "these packages"
print(iostr, "\n ",
color_string(string(n_loaded), Base.warn_color()),
" $(plural1) precompiled but $(plural2) currently loaded. Restart julia to access the new version$(plural3)"
" $(plural1) precompiled but $(plural2) currently loaded. Restart julia to access the new version$(plural3). \
Otherwise, loading dependents of $(plural4) may trigger further precompilation to work with the unexpected version$(plural3)."
)
end
if !isempty(precomperr_deps)
Expand Down Expand Up @@ -1130,7 +1134,7 @@ function _color_string(cstr::String, col::Union{Int64, Symbol}, hascolor)
end

# Can be merged with `maybe_cachefile_lock` in loading?
function precompile_pkgs_maybe_cachefile_lock(f, io::IO, print_lock::ReentrantLock, fancyprint::Bool, pkg_config, pkgspidlocked, hascolor, parallel_limiter::Base.Semaphore)
function precompile_pkgs_maybe_cachefile_lock(f, io::IO, print_lock::ReentrantLock, fancyprint::Bool, pkg_config, pkgspidlocked, hascolor, parallel_limiter::Base.Semaphore, ignore_loaded::Bool)
if !(isdefined(Base, :mkpidlock_hook) && isdefined(Base, :trymkpidlock_hook) && Base.isdefined(Base, :parse_pidfile_hook))
return f()
end
Expand Down Expand Up @@ -1158,7 +1162,7 @@ function precompile_pkgs_maybe_cachefile_lock(f, io::IO, print_lock::ReentrantLo
# wait until the lock is available
@invokelatest Base.mkpidlock_hook(() -> begin
# double-check in case the other process crashed or the lock expired
if Base.isprecompiled(pkg; ignore_loaded=true, flags=cacheflags) # don't use caches for this as the env state will have changed
if Base.isprecompiled(pkg; ignore_loaded, flags=cacheflags) # don't use caches for this as the env state will have changed
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall a reason for this to be forced false. I'm assuming it was just missed.

return nothing # returning nothing indicates a process waited for another
else
delete!(pkgspidlocked, pkg_config)
Expand Down
Loading