From 39e1365e301e3c9d354958a42e3b580bdd7b2946 Mon Sep 17 00:00:00 2001 From: Lilith Orion Hafner Date: Tue, 16 Jul 2024 06:35:52 -0500 Subject: [PATCH 1/9] Replace some occurrences of iteration over 1:length with more idiomatic structures (mostly eachindex) (#55137) Base should be a model for the ecosystem, and `eachindex(x)` is better than `1:length(x)` in almost all cases. I've updated many, but certainly not all examples. This is mostly a NFC, but also fixes #55136. (cherry picked from commit 0945b9d7740855c82a09fed42fbf6bc561e02c77) --- base/abstractarray.jl | 8 ++++---- base/errorshow.jl | 10 +++++----- base/expr.jl | 2 +- base/intfuncs.jl | 2 +- base/loading.jl | 9 ++++----- base/multidimensional.jl | 2 +- base/reflection.jl | 2 +- base/show.jl | 10 +++++----- base/tuple.jl | 6 +++--- 9 files changed, 25 insertions(+), 26 deletions(-) diff --git a/base/abstractarray.jl b/base/abstractarray.jl index bb4aff0f6a411..45fff8dae0d24 100644 --- a/base/abstractarray.jl +++ b/base/abstractarray.jl @@ -1634,10 +1634,10 @@ typed_vcat(::Type{T}) where {T} = Vector{T}() typed_hcat(::Type{T}) where {T} = Vector{T}() ## cat: special cases -vcat(X::T...) where {T} = T[ X[i] for i=1:length(X) ] -vcat(X::T...) where {T<:Number} = T[ X[i] for i=1:length(X) ] -hcat(X::T...) where {T} = T[ X[j] for i=1:1, j=1:length(X) ] -hcat(X::T...) where {T<:Number} = T[ X[j] for i=1:1, j=1:length(X) ] +vcat(X::T...) where {T} = T[ X[i] for i=eachindex(X) ] +vcat(X::T...) where {T<:Number} = T[ X[i] for i=eachindex(X) ] +hcat(X::T...) where {T} = T[ X[j] for i=1:1, j=eachindex(X) ] +hcat(X::T...) where {T<:Number} = T[ X[j] for i=1:1, j=eachindex(X) ] vcat(X::Number...) = hvcat_fill!(Vector{promote_typeof(X...)}(undef, length(X)), X) hcat(X::Number...) = hvcat_fill!(Matrix{promote_typeof(X...)}(undef, 1,length(X)), X) diff --git a/base/errorshow.jl b/base/errorshow.jl index f63c150336689..b9a9a473cce96 100644 --- a/base/errorshow.jl +++ b/base/errorshow.jl @@ -196,7 +196,7 @@ function showerror(io::IO, ex::CanonicalIndexError) print(io, "CanonicalIndexError: ", ex.func, " not defined for ", ex.type) end -typesof(@nospecialize args...) = Tuple{Any[ Core.Typeof(args[i]) for i in 1:length(args) ]...} +typesof(@nospecialize args...) = Tuple{Any[Core.Typeof(arg) for arg in args]...} function print_with_compare(io::IO, @nospecialize(a::DataType), @nospecialize(b::DataType), color::Symbol) if a.name === b.name @@ -273,7 +273,7 @@ function showerror(io::IO, ex::MethodError) arg_types_param = arg_types_param[3:end] san_arg_types_param = san_arg_types_param[3:end] keys = kwt.parameters[1]::Tuple - kwargs = Any[(keys[i], fieldtype(kwt, i)) for i in 1:length(keys)] + kwargs = Any[(keys[i], fieldtype(kwt, i)) for i in eachindex(keys)] arg_types = rewrap_unionall(Tuple{arg_types_param...}, arg_types) end if f === Base.convert && length(arg_types_param) == 2 && !is_arg_types @@ -687,7 +687,7 @@ function show_reduced_backtrace(io::IO, t::Vector) push!(repeated_cycle, (0,0,0)) # repeated_cycle is never empty frame_counter = 1 - for i in 1:length(displayed_stackframes) + for i in eachindex(displayed_stackframes) (frame, n) = displayed_stackframes[i] print_stackframe(io, frame_counter, frame, n, ndigits_max, STACKTRACE_FIXEDCOLORS, STACKTRACE_MODULECOLORS) @@ -864,7 +864,7 @@ end function _collapse_repeated_frames(trace) kept_frames = trues(length(trace)) last_frame = nothing - for i in 1:length(trace) + for i in eachindex(trace) frame::StackFrame, _ = trace[i] if last_frame !== nothing && frame.file == last_frame.file && frame.line == last_frame.line #= @@ -909,7 +909,7 @@ function _collapse_repeated_frames(trace) end if length(last_params) > length(params) issame = true - for i = 1:length(params) + for i = eachindex(params) issame &= params[i] == last_params[i] end if issame diff --git a/base/expr.jl b/base/expr.jl index c4ed916fe34aa..58435135c99ae 100644 --- a/base/expr.jl +++ b/base/expr.jl @@ -68,7 +68,7 @@ function copy_exprs(@nospecialize(x)) end return x end -copy_exprargs(x::Array{Any,1}) = Any[copy_exprs(@inbounds x[i]) for i in 1:length(x)] +copy_exprargs(x::Array{Any,1}) = Any[copy_exprs(@inbounds x[i]) for i in eachindex(x)] @eval exprarray(head::Symbol, arg::Array{Any,1}) = $(Expr(:new, :Expr, :head, :arg)) diff --git a/base/intfuncs.jl b/base/intfuncs.jl index 74b450990bc12..f2de94db5589e 100644 --- a/base/intfuncs.jl +++ b/base/intfuncs.jl @@ -976,7 +976,7 @@ end Return an array with element type `T` (default `Int`) of the digits of `n` in the given base, optionally padded with zeros to a specified size. More significant digits are at -higher indices, such that `n == sum(digits[k]*base^(k-1) for k=1:length(digits))`. +higher indices, such that `n == sum(digits[k]*base^(k-1) for k in eachindex(digits))`. See also [`ndigits`](@ref), [`digits!`](@ref), and for base 2 also [`bitstring`](@ref), [`count_ones`](@ref). diff --git a/base/loading.jl b/base/loading.jl index 558eb76d51e96..7f24ae48a3e33 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -1228,7 +1228,7 @@ function _include_from_serialized(pkg::PkgId, path::String, ocachepath::Union{No t_comp_before = cumulative_compile_time_ns() end - for i in 1:length(depmods) + for i in eachindex(depmods) dep = depmods[i] dep isa Module && continue _, depkey, depbuild_id = dep::Tuple{String, PkgId, UInt128} @@ -1781,8 +1781,7 @@ function compilecache_path(pkg::PkgId; end staledeps, _, _ = staledeps::Tuple{Vector{Any}, Union{Nothing, String}, UInt128} # finish checking staledeps module graph - for i in 1:length(staledeps) - dep = staledeps[i] + for dep in staledeps dep isa Module && continue modpath, modkey, modbuild_id = dep::Tuple{String, PkgId, UInt128} modpaths = find_all_in_cache_path(modkey) @@ -1971,7 +1970,7 @@ end try staledeps, ocachefile, newbuild_id = staledeps::Tuple{Vector{Any}, Union{Nothing, String}, UInt128} # finish checking staledeps module graph - for i in 1:length(staledeps) + for i in eachindex(staledeps) dep = staledeps[i] dep isa Module && continue modpath, modkey, modbuild_id = dep::Tuple{String, PkgId, UInt128} @@ -2003,7 +2002,7 @@ end end # finish loading module graph into staledeps # TODO: call all start_loading calls (in reverse order) before calling any _include_from_serialized, since start_loading will drop the loading lock - for i in 1:length(staledeps) + for i in eachindex(staledeps) dep = staledeps[i] dep isa Module && continue modpath, modkey, modbuild_id, modcachepath, modstaledeps, modocachepath = dep::Tuple{String, PkgId, UInt128, String, Vector{Any}, Union{Nothing, String}} diff --git a/base/multidimensional.jl b/base/multidimensional.jl index b6c93fa318d78..730a165cde18f 100644 --- a/base/multidimensional.jl +++ b/base/multidimensional.jl @@ -1639,7 +1639,7 @@ function checkdims_perm(P::AbstractArray{TP,N}, B::AbstractArray{TB,N}, perm) wh length(perm) == N || throw(ArgumentError("expected permutation of size $N, but length(perm)=$(length(perm))")) isperm(perm) || throw(ArgumentError("input is not a permutation")) indsP = axes(P) - for i = 1:length(perm) + for i in eachindex(perm) indsP[i] == indsB[perm[i]] || throw(DimensionMismatch("destination tensor of incorrect size")) end nothing diff --git a/base/reflection.jl b/base/reflection.jl index fd3e7931b2ce8..d70683feaaae7 100644 --- a/base/reflection.jl +++ b/base/reflection.jl @@ -2367,7 +2367,7 @@ function hasmethod(f, t, kwnames::Tuple{Vararg{Symbol}}; world::UInt=get_world_c for kw in kws endswith(String(kw), "...") && return true end - kwnames = Symbol[kwnames[i] for i in 1:length(kwnames)] + kwnames = collect(kwnames) return issubset(kwnames, kws) end diff --git a/base/show.jl b/base/show.jl index 3bec8a5c885a2..66628d60f21bf 100644 --- a/base/show.jl +++ b/base/show.jl @@ -676,7 +676,7 @@ function show_can_elide(p::TypeVar, wheres::Vector, elide::Int, env::SimpleVecto has_typevar(v.lb, p) && return false has_typevar(v.ub, p) && return false end - for i = 1:length(env) + for i = eachindex(env) i == skip && continue has_typevar(env[i], p) && return false end @@ -1183,7 +1183,7 @@ end function show_at_namedtuple(io::IO, syms::Tuple, types::DataType) first = true - for i in 1:length(syms) + for i in eachindex(syms) if !first print(io, ", ") end @@ -2372,7 +2372,7 @@ function show_unquoted(io::IO, ex::Expr, indent::Int, prec::Int, quote_level::In if get(io, beginsym, false) print(io, '(') ind = indent + indent_width - for i = 1:length(ex.args) + for i = eachindex(ex.args) if i > 1 # if there was only a comment before the first semicolon, the expression would get parsed as a NamedTuple if !(i == 2 && ex.args[1] isa LineNumberNode) @@ -2890,7 +2890,7 @@ function dump(io::IOContext, x::SimpleVector, n::Int, indent) end print(io, "SimpleVector") if n > 0 - for i = 1:length(x) + for i in eachindex(x) println(io) print(io, indent, " ", i, ": ") if isassigned(x,i) @@ -3000,7 +3000,7 @@ function dump(io::IOContext, x::DataType, n::Int, indent) end fields = fieldnames(x) fieldtypes = datatype_fieldtypes(x) - for idx in 1:length(fields) + for idx in eachindex(fields) println(io) print(io, indent, " ", fields[idx]) if isassigned(fieldtypes, idx) diff --git a/base/tuple.jl b/base/tuple.jl index 5ab5b4b1c7a26..4d0791e3303d6 100644 --- a/base/tuple.jl +++ b/base/tuple.jl @@ -502,7 +502,7 @@ end _findfirst_rec(f, i::Int, ::Tuple{}) = nothing _findfirst_rec(f, i::Int, t::Tuple) = (@inline; f(first(t)) ? i : _findfirst_rec(f, i+1, tail(t))) function _findfirst_loop(f::Function, t) - for i in 1:length(t) + for i in eachindex(t) f(t[i]) && return i end return nothing @@ -536,7 +536,7 @@ function _isequal(t1::Tuple{Any,Vararg{Any}}, t2::Tuple{Any,Vararg{Any}}) return isequal(t1[1], t2[1]) && _isequal(tail(t1), tail(t2)) end function _isequal(t1::Any32, t2::Any32) - for i = 1:length(t1) + for i in eachindex(t1, t2) if !isequal(t1[i], t2[i]) return false end @@ -567,7 +567,7 @@ function _eq_missing(t1::Tuple, t2::Tuple) end function _eq(t1::Any32, t2::Any32) anymissing = false - for i = 1:length(t1) + for i in eachindex(t1, t2) eq = (t1[i] == t2[i]) if ismissing(eq) anymissing = true From 03d23d0eb92d1d2d7db15fa7f6548d5c1c4ef0b5 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Fri, 4 Oct 2024 12:03:53 -0400 Subject: [PATCH 2/9] add logic to prefer loading modules that are already loaded (#55908) Iterate over the list of existing loaded modules for PkgId whenever loading a new module for PkgId, so that we will use that existing build_id content if it otherwise passes the other stale_checks. (cherry picked from commit 7e2d8035990e67810ccbbc9fb5da810b1b83b774) --- base/Base.jl | 2 +- base/loading.jl | 184 +++++++++++++++++++++++++++--------------------- test/loading.jl | 36 +++++++++- 3 files changed, 138 insertions(+), 84 deletions(-) diff --git a/base/Base.jl b/base/Base.jl index 881e9f258dd5d..8e6a3ec17bdaf 100644 --- a/base/Base.jl +++ b/base/Base.jl @@ -605,7 +605,7 @@ function __init__() empty!(explicit_loaded_modules) empty!(loaded_precompiles) # If we load a packageimage when building the image this might not be empty for (mod, key) in module_keys - loaded_precompiles[key => module_build_id(mod)] = mod + push!(get!(Vector{Module}, loaded_precompiles, key), mod) end if haskey(ENV, "JULIA_MAX_NUM_PRECOMPILE_FILES") MAX_NUM_PRECOMPILE_FILES[] = parse(Int, ENV["JULIA_MAX_NUM_PRECOMPILE_FILES"]) diff --git a/base/loading.jl b/base/loading.jl index 7f24ae48a3e33..cbbd74e86f211 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -1232,7 +1232,7 @@ function _include_from_serialized(pkg::PkgId, path::String, ocachepath::Union{No dep = depmods[i] dep isa Module && continue _, depkey, depbuild_id = dep::Tuple{String, PkgId, UInt128} - dep = loaded_precompiles[depkey => depbuild_id] + dep = something(maybe_loaded_precompile(depkey, depbuild_id)) @assert PkgId(dep) == depkey && module_build_id(dep) === depbuild_id depmods[i] = dep end @@ -1338,6 +1338,7 @@ end function register_restored_modules(sv::SimpleVector, pkg::PkgId, path::String) # This function is also used by PkgCacheInspector.jl + assert_havelock(require_lock) restored = sv[1]::Vector{Any} for M in restored M = M::Module @@ -1346,7 +1347,7 @@ function register_restored_modules(sv::SimpleVector, pkg::PkgId, path::String) end if parentmodule(M) === M push!(loaded_modules_order, M) - loaded_precompiles[pkg => module_build_id(M)] = M + push!(get!(Vector{Module}, loaded_precompiles, pkg), M) end end @@ -1962,90 +1963,102 @@ end assert_havelock(require_lock) paths = find_all_in_cache_path(pkg, DEPOT_PATH) newdeps = PkgId[] - for path_to_try in paths::Vector{String} - staledeps = stale_cachefile(pkg, build_id, sourcepath, path_to_try; reasons, stalecheck) - if staledeps === true - continue - end - try - staledeps, ocachefile, newbuild_id = staledeps::Tuple{Vector{Any}, Union{Nothing, String}, UInt128} - # finish checking staledeps module graph - for i in eachindex(staledeps) - dep = staledeps[i] - dep isa Module && continue - modpath, modkey, modbuild_id = dep::Tuple{String, PkgId, UInt128} - modpaths = find_all_in_cache_path(modkey, DEPOT_PATH) - for modpath_to_try in modpaths - modstaledeps = stale_cachefile(modkey, modbuild_id, modpath, modpath_to_try; stalecheck) - if modstaledeps === true - continue - end - modstaledeps, modocachepath, _ = modstaledeps::Tuple{Vector{Any}, Union{Nothing, String}, UInt128} - staledeps[i] = (modpath, modkey, modbuild_id, modpath_to_try, modstaledeps, modocachepath) - @goto check_next_dep + try_build_ids = UInt128[build_id] + if build_id == UInt128(0) + let loaded = get(loaded_precompiles, pkg, nothing) + if loaded !== nothing + for mod in loaded # try these in reverse original load order to see if one is already valid + pushfirst!(try_build_ids, module_build_id(mod)) end - @debug "Rejecting cache file $path_to_try because required dependency $modkey with build ID $(UUID(modbuild_id)) is missing from the cache." - @goto check_next_path - @label check_next_dep end - M = get(loaded_precompiles, pkg => newbuild_id, nothing) - if isa(M, Module) - stalecheck && register_root_module(M) - return M - end - if stalecheck - try - touch(path_to_try) # update timestamp of precompilation file - catch ex # file might be read-only and then we fail to update timestamp, which is fine - ex isa IOError || rethrow() - end + end + end + for build_id in try_build_ids + for path_to_try in paths::Vector{String} + staledeps = stale_cachefile(pkg, build_id, sourcepath, path_to_try; reasons, stalecheck) + if staledeps === true + continue end - # finish loading module graph into staledeps - # TODO: call all start_loading calls (in reverse order) before calling any _include_from_serialized, since start_loading will drop the loading lock - for i in eachindex(staledeps) - dep = staledeps[i] - dep isa Module && continue - modpath, modkey, modbuild_id, modcachepath, modstaledeps, modocachepath = dep::Tuple{String, PkgId, UInt128, String, Vector{Any}, Union{Nothing, String}} - dep = start_loading(modkey, modbuild_id, stalecheck) - while true - if dep isa Module - if PkgId(dep) == modkey && module_build_id(dep) === modbuild_id - break - else - @debug "Rejecting cache file $path_to_try because module $modkey got loaded at a different version than expected." - @goto check_next_path + try + staledeps, ocachefile, newbuild_id = staledeps::Tuple{Vector{Any}, Union{Nothing, String}, UInt128} + # finish checking staledeps module graph + for i in eachindex(staledeps) + dep = staledeps[i] + dep isa Module && continue + modpath, modkey, modbuild_id = dep::Tuple{String, PkgId, UInt128} + modpaths = find_all_in_cache_path(modkey, DEPOT_PATH) + for modpath_to_try in modpaths + modstaledeps = stale_cachefile(modkey, modbuild_id, modpath, modpath_to_try; stalecheck) + if modstaledeps === true + continue end + modstaledeps, modocachepath, _ = modstaledeps::Tuple{Vector{Any}, Union{Nothing, String}, UInt128} + staledeps[i] = (modpath, modkey, modbuild_id, modpath_to_try, modstaledeps, modocachepath) + @goto check_next_dep end - if dep === nothing - try - set_pkgorigin_version_path(modkey, modpath) - dep = _include_from_serialized(modkey, modcachepath, modocachepath, modstaledeps; register = stalecheck) - finally - end_loading(modkey, dep) + @debug "Rejecting cache file $path_to_try because required dependency $modkey with build ID $(UUID(modbuild_id)) is missing from the cache." + @goto check_next_path + @label check_next_dep + end + M = maybe_loaded_precompile(pkg, newbuild_id) + if isa(M, Module) + stalecheck && register_root_module(M) + return M + end + if stalecheck + try + touch(path_to_try) # update timestamp of precompilation file + catch ex # file might be read-only and then we fail to update timestamp, which is fine + ex isa IOError || rethrow() + end + end + # finish loading module graph into staledeps + # TODO: call all start_loading calls (in reverse order) before calling any _include_from_serialized, since start_loading will drop the loading lock + for i in eachindex(staledeps) + dep = staledeps[i] + dep isa Module && continue + modpath, modkey, modbuild_id, modcachepath, modstaledeps, modocachepath = dep::Tuple{String, PkgId, UInt128, String, Vector{Any}, Union{Nothing, String}} + dep = start_loading(modkey, modbuild_id, stalecheck) + while true + if dep isa Module + if PkgId(dep) == modkey && module_build_id(dep) === modbuild_id + break + else + @debug "Rejecting cache file $path_to_try because module $modkey got loaded at a different version than expected." + @goto check_next_path + end end - if !isa(dep, Module) - @debug "Rejecting cache file $path_to_try because required dependency $modkey failed to load from cache file for $modcachepath." exception=dep - @goto check_next_path - else - push!(newdeps, modkey) + if dep === nothing + try + set_pkgorigin_version_path(modkey, modpath) + dep = _include_from_serialized(modkey, modcachepath, modocachepath, modstaledeps; register = stalecheck) + finally + end_loading(modkey, dep) + end + if !isa(dep, Module) + @debug "Rejecting cache file $path_to_try because required dependency $modkey failed to load from cache file for $modcachepath." exception=dep + @goto check_next_path + else + push!(newdeps, modkey) + end end end + staledeps[i] = dep end - staledeps[i] = dep - end - restored = get(loaded_precompiles, pkg => newbuild_id, nothing) - if !isa(restored, Module) - restored = _include_from_serialized(pkg, path_to_try, ocachefile, staledeps; register = stalecheck) - end - isa(restored, Module) && return restored - @debug "Deserialization checks failed while attempting to load cache from $path_to_try" exception=restored - @label check_next_path - finally - for modkey in newdeps - insert_extension_triggers(modkey) - stalecheck && run_package_callbacks(modkey) + restored = maybe_loaded_precompile(pkg, newbuild_id) + if !isa(restored, Module) + restored = _include_from_serialized(pkg, path_to_try, ocachefile, staledeps; register = stalecheck) + end + isa(restored, Module) && return restored + @debug "Deserialization checks failed while attempting to load cache from $path_to_try" exception=restored + @label check_next_path + finally + for modkey in newdeps + insert_extension_triggers(modkey) + stalecheck && run_package_callbacks(modkey) + end + empty!(newdeps) end - empty!(newdeps) end end return nothing @@ -2065,7 +2078,7 @@ function start_loading(modkey::PkgId, build_id::UInt128, stalecheck::Bool) loaded = stalecheck ? maybe_root_module(modkey) : nothing loaded isa Module && return loaded if build_id != UInt128(0) - loaded = get(loaded_precompiles, modkey => build_id, nothing) + loaded = maybe_loaded_precompile(modkey, build_id) loaded isa Module && return loaded end loading = get(package_locks, modkey, nothing) @@ -2394,13 +2407,22 @@ const pkgorigins = Dict{PkgId,PkgOrigin}() const explicit_loaded_modules = Dict{PkgId,Module}() # Emptied on Julia start const loaded_modules = Dict{PkgId,Module}() # available to be explicitly loaded -const loaded_precompiles = Dict{Pair{PkgId,UInt128},Module}() # extended (complete) list of modules, available to be loaded +const loaded_precompiles = Dict{PkgId,Vector{Module}}() # extended (complete) list of modules, available to be loaded const loaded_modules_order = Vector{Module}() const module_keys = IdDict{Module,PkgId}() # the reverse of loaded_modules is_root_module(m::Module) = @lock require_lock haskey(module_keys, m) root_module_key(m::Module) = @lock require_lock module_keys[m] +function maybe_loaded_precompile(key::PkgId, buildid::UInt128) + assert_havelock(require_lock) + mods = get(loaded_precompiles, key, nothing) + mods === nothing && return + for mod in mods + module_build_id(mod) == buildid && return mod + end +end + function module_build_id(m::Module) hi, lo = ccall(:jl_module_build_id, NTuple{2,UInt64}, (Any,), m) return (UInt128(hi) << 64) | lo @@ -2421,7 +2443,7 @@ end end end end - haskey(loaded_precompiles, key => module_build_id(m)) || push!(loaded_modules_order, m) + maybe_loaded_precompile(key, module_build_id(m)) === nothing && push!(loaded_modules_order, m) loaded_modules[key] = m explicit_loaded_modules[key] = m module_keys[m] = key @@ -3785,8 +3807,8 @@ end for i in 1:ndeps req_key, req_build_id = required_modules[i] # Check if module is already loaded - if !stalecheck && haskey(loaded_precompiles, req_key => req_build_id) - M = loaded_precompiles[req_key => req_build_id] + M = stalecheck ? nothing : maybe_loaded_precompile(req_key, req_build_id) + if M !== nothing @assert PkgId(M) == req_key && module_build_id(M) === req_build_id depmods[i] = M elseif root_module_exists(req_key) diff --git a/test/loading.jl b/test/loading.jl index 8f0c4897e24eb..9cfd421a596cb 100644 --- a/test/loading.jl +++ b/test/loading.jl @@ -1,10 +1,10 @@ # This file is a part of Julia. License is MIT: https://julialang.org/license -original_depot_path = copy(Base.DEPOT_PATH) - using Test # Tests for @__LINE__ inside and outside of macros +# NOTE: the __LINE__ numbers for these first couple tests are significant, so +# adding any lines here will make those tests fail @test (@__LINE__) == 8 macro macro_caller_lineno() @@ -33,6 +33,9 @@ end @test @nested_LINE_expansion() == ((@__LINE__() - 4, @__LINE__() - 12), @__LINE__()) @test @nested_LINE_expansion2() == ((@__LINE__() - 5, @__LINE__() - 9), @__LINE__()) +original_depot_path = copy(Base.DEPOT_PATH) +include("precompile_utils.jl") + loaded_files = String[] push!(Base.include_callbacks, (mod::Module, fn::String) -> push!(loaded_files, fn)) include("test_sourcepath.jl") @@ -1590,3 +1593,32 @@ end copy!(LOAD_PATH, old_load_path) end end + +@testset "require_stdlib loading duplication" begin + depot_path = mktempdir() + oldBase64 = nothing + try + push!(empty!(DEPOT_PATH), depot_path) + Base64_key = Base.PkgId(Base.UUID("2a0f44e3-6c83-55bd-87e4-b1978d98bd5f"), "Base64") + oldBase64 = Base.unreference_module(Base64_key) + cc = Base.compilecache(Base64_key) + @test Base.isprecompiled(Base64_key, cachepaths=String[cc[1]]) + empty!(DEPOT_PATH) + Base.require_stdlib(Base64_key) + push!(DEPOT_PATH, depot_path) + append!(DEPOT_PATH, original_depot_path) + oldloaded = @lock(Base.require_lock, length(get(Base.loaded_precompiles, Base64_key, Module[]))) + Base.require(Base64_key) + @test @lock(Base.require_lock, length(get(Base.loaded_precompiles, Base64_key, Module[]))) == oldloaded + Base.unreference_module(Base64_key) + empty!(DEPOT_PATH) + push!(DEPOT_PATH, depot_path) + Base.require(Base64_key) + @test @lock(Base.require_lock, length(get(Base.loaded_precompiles, Base64_key, Module[]))) == oldloaded + 1 + Base.unreference_module(Base64_key) + finally + oldBase64 === nothing || Base.register_root_module(oldBase64) + copy!(DEPOT_PATH, original_depot_path) + rm(depot_path, force=true, recursive=true) + end +end From 493bec08ac03da86723db0a75954cf3d1c7ca254 Mon Sep 17 00:00:00 2001 From: Kristoffer Carlsson Date: Mon, 21 Oct 2024 14:40:37 +0200 Subject: [PATCH 3/9] remove new references to explicit_loaded_modules --- base/loading.jl | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/base/loading.jl b/base/loading.jl index cbbd74e86f211..322565d661ac3 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -2477,9 +2477,6 @@ loaded_modules_array() = @lock require_lock copy(loaded_modules_order) # after unreference_module, a subsequent require call will try to load a new copy of it, if stale # reload(m) = (unreference_module(m); require(m)) function unreference_module(key::PkgId) - if haskey(explicit_loaded_modules, key) - m = pop!(explicit_loaded_modules, key) - end if haskey(loaded_modules, key) m = pop!(loaded_modules, key) # need to ensure all modules are GC rooted; will still be referenced @@ -3043,7 +3040,7 @@ function compilecache(pkg::PkgId, path::String, internal_stderr::IO = stderr, in # build up the list of modules that we want the precompile process to preserve if keep_loaded_modules concrete_deps = copy(_concrete_dependencies) - for (pkgreq, modreq) in loaded_modules # TODO: convert all relevant staleness heuristics to use explicit_loaded_modules instead + for (pkgreq, modreq) in loaded_modules if !(pkgreq === Main || pkgreq === Core || pkgreq === Base) push!(concrete_deps, pkgreq => module_build_id(modreq)) end From 694d00a528b88bb1def12483b449999040ed6831 Mon Sep 17 00:00:00 2001 From: Kristoffer Carlsson Date: Fri, 18 Oct 2024 14:42:23 +0200 Subject: [PATCH 4/9] Revert "Extensions: make loading of extensions independent of what packages are in the sysimage (#52841)" This reverts commit 08d229f4a7cb0c3ef5becddd1b3bc4f8f178b8e4. (cherry picked from commit e3f2f6b9c69293da68c1019366f8397d2049c6d8) --- base/Base.jl | 1 - base/loading.jl | 9 +++------ test/loading.jl | 19 ------------------- .../HasDepWithExtensions.jl/Manifest.toml | 7 +------ .../Extensions/HasExtensions.jl/Project.toml | 2 -- .../HasExtensions.jl/ext/LinearAlgebraExt.jl | 3 --- 6 files changed, 4 insertions(+), 37 deletions(-) delete mode 100644 test/project/Extensions/HasExtensions.jl/ext/LinearAlgebraExt.jl diff --git a/base/Base.jl b/base/Base.jl index 8e6a3ec17bdaf..ab8493cd6fd32 100644 --- a/base/Base.jl +++ b/base/Base.jl @@ -602,7 +602,6 @@ function __init__() init_load_path() init_active_project() append!(empty!(_sysimage_modules), keys(loaded_modules)) - empty!(explicit_loaded_modules) empty!(loaded_precompiles) # If we load a packageimage when building the image this might not be empty for (mod, key) in module_keys push!(get!(Vector{Module}, loaded_precompiles, key), mod) diff --git a/base/loading.jl b/base/loading.jl index 322565d661ac3..018475b3604ca 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -1518,7 +1518,7 @@ function _insert_extension_triggers(parent::PkgId, extensions::Dict{String, Any} uuid_trigger = UUID(totaldeps[trigger]::String) trigger_id = PkgId(uuid_trigger, trigger) push!(trigger_ids, trigger_id) - if !haskey(explicit_loaded_modules, trigger_id) || haskey(package_locks, trigger_id) + if !haskey(Base.loaded_modules, trigger_id) || haskey(package_locks, trigger_id) trigger1 = get!(Vector{ExtensionId}, EXT_DORMITORY, trigger_id) push!(trigger1, gid) else @@ -2390,9 +2390,8 @@ function __require_prelocked(uuidkey::PkgId, env=nothing) insert_extension_triggers(uuidkey) # After successfully loading, notify downstream consumers run_package_callbacks(uuidkey) - elseif !haskey(explicit_loaded_modules, uuidkey) - explicit_loaded_modules[uuidkey] = m - run_package_callbacks(uuidkey) + else + newm = root_module(uuidkey) end return m end @@ -2405,7 +2404,6 @@ end PkgOrigin() = PkgOrigin(nothing, nothing, nothing) const pkgorigins = Dict{PkgId,PkgOrigin}() -const explicit_loaded_modules = Dict{PkgId,Module}() # Emptied on Julia start const loaded_modules = Dict{PkgId,Module}() # available to be explicitly loaded const loaded_precompiles = Dict{PkgId,Vector{Module}}() # extended (complete) list of modules, available to be loaded const loaded_modules_order = Vector{Module}() @@ -2445,7 +2443,6 @@ end end maybe_loaded_precompile(key, module_build_id(m)) === nothing && push!(loaded_modules_order, m) loaded_modules[key] = m - explicit_loaded_modules[key] = m module_keys[m] = key end nothing diff --git a/test/loading.jl b/test/loading.jl index 9cfd421a596cb..fa90739b74fc9 100644 --- a/test/loading.jl +++ b/test/loading.jl @@ -1119,25 +1119,6 @@ end run(cmd_proj_ext) end - # Sysimage extensions - # The test below requires that LinearAlgebra is in the sysimage and that it has not been loaded yet. - # if it gets moved out, this test will need to be updated. - # We run this test in a new process so we are not vulnerable to a previous test having loaded LinearAlgebra - sysimg_ext_test_code = """ - uuid_key = Base.PkgId(Base.UUID("37e2e46d-f89d-539d-b4ee-838fcccc9c8e"), "LinearAlgebra") - Base.in_sysimage(uuid_key) || error("LinearAlgebra not in sysimage") - haskey(Base.explicit_loaded_modules, uuid_key) && error("LinearAlgebra already loaded") - using HasExtensions - Base.get_extension(HasExtensions, :LinearAlgebraExt) === nothing || error("unexpectedly got an extension") - using LinearAlgebra - haskey(Base.explicit_loaded_modules, uuid_key) || error("LinearAlgebra not loaded") - Base.get_extension(HasExtensions, :LinearAlgebraExt) isa Module || error("expected extension to load") - """ - cmd = `$(Base.julia_cmd()) --startup-file=no -e $sysimg_ext_test_code` - cmd = addenv(cmd, "JULIA_LOAD_PATH" => join([proj, "@stdlib"], sep)) - run(cmd) - - # Extensions in implicit environments old_load_path = copy(LOAD_PATH) try diff --git a/test/project/Extensions/HasDepWithExtensions.jl/Manifest.toml b/test/project/Extensions/HasDepWithExtensions.jl/Manifest.toml index 59bd802c9710a..98510dcb27733 100644 --- a/test/project/Extensions/HasDepWithExtensions.jl/Manifest.toml +++ b/test/project/Extensions/HasDepWithExtensions.jl/Manifest.toml @@ -25,17 +25,12 @@ deps = ["ExtDep3"] path = "../HasExtensions.jl" uuid = "4d3288b3-3afc-4bb6-85f3-489fffe514c8" version = "0.1.0" +weakdeps = ["ExtDep", "ExtDep2"] [deps.HasExtensions.extensions] Extension = "ExtDep" ExtensionDep = "ExtDep3" ExtensionFolder = ["ExtDep", "ExtDep2"] - LinearAlgebraExt = "LinearAlgebra" - - [deps.HasExtensions.weakdeps] - ExtDep = "fa069be4-f60b-4d4c-8b95-f8008775090c" - ExtDep2 = "55982ee5-2ad5-4c40-8cfe-5e9e1b01500d" - LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" [[deps.SomeOtherPackage]] path = "../SomeOtherPackage" diff --git a/test/project/Extensions/HasExtensions.jl/Project.toml b/test/project/Extensions/HasExtensions.jl/Project.toml index fe21a1423f543..a02f5662d602d 100644 --- a/test/project/Extensions/HasExtensions.jl/Project.toml +++ b/test/project/Extensions/HasExtensions.jl/Project.toml @@ -8,10 +8,8 @@ ExtDep3 = "a5541f1e-a556-4fdc-af15-097880d743a1" [weakdeps] ExtDep = "fa069be4-f60b-4d4c-8b95-f8008775090c" ExtDep2 = "55982ee5-2ad5-4c40-8cfe-5e9e1b01500d" -LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" [extensions] Extension = "ExtDep" ExtensionDep = "ExtDep3" ExtensionFolder = ["ExtDep", "ExtDep2"] -LinearAlgebraExt = "LinearAlgebra" diff --git a/test/project/Extensions/HasExtensions.jl/ext/LinearAlgebraExt.jl b/test/project/Extensions/HasExtensions.jl/ext/LinearAlgebraExt.jl deleted file mode 100644 index 19f87cb849417..0000000000000 --- a/test/project/Extensions/HasExtensions.jl/ext/LinearAlgebraExt.jl +++ /dev/null @@ -1,3 +0,0 @@ -module LinearAlgebraExt - -end From d9db8f8649db4e944c7dfbe6b9562596429ac440 Mon Sep 17 00:00:00 2001 From: KristofferC Date: Mon, 21 Oct 2024 14:18:48 +0200 Subject: [PATCH 5/9] fix lookup when extension is in `[deps]` (cherry picked from commit ad1dc390e3123b65433ee06a651ca6de88c29914) --- base/loading.jl | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/base/loading.jl b/base/loading.jl index 018475b3604ca..324ff8f657145 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -934,14 +934,14 @@ function explicit_manifest_deps_get(project_file::String, where::PkgId, name::St entry = entry::Dict{String, Any} uuid = get(entry, "uuid", nothing)::Union{String, Nothing} uuid === nothing && continue + # deps is either a list of names (deps = ["DepA", "DepB"]) or + # a table of entries (deps = {"DepA" = "6ea...", "DepB" = "55d..."} + deps = get(entry, "deps", nothing)::Union{Vector{String}, Dict{String, Any}, Nothing} if UUID(uuid) === where.uuid found_where = true - # deps is either a list of names (deps = ["DepA", "DepB"]) or - # a table of entries (deps = {"DepA" = "6ea...", "DepB" = "55d..."} - deps = get(entry, "deps", nothing)::Union{Vector{String}, Dict{String, Any}, Nothing} if deps isa Vector{String} found_name = name in deps - break + found_name && @goto done elseif deps isa Dict{String, Any} deps = deps::Dict{String, Any} for (dep, uuid) in deps @@ -960,23 +960,25 @@ function explicit_manifest_deps_get(project_file::String, where::PkgId, name::St return PkgId(UUID(uuid), name) end exts = extensions[where.name]::Union{String, Vector{String}} + weakdeps = get(entry, "weakdeps", nothing)::Union{Vector{String}, Dict{String, Any}, Nothing} if (exts isa String && name == exts) || (exts isa Vector{String} && name in exts) - weakdeps = get(entry, "weakdeps", nothing)::Union{Vector{String}, Dict{String, Any}, Nothing} - if weakdeps !== nothing - if weakdeps isa Vector{String} - found_name = name in weakdeps - break - elseif weakdeps isa Dict{String, Any} - weakdeps = weakdeps::Dict{String, Any} - for (dep, uuid) in weakdeps - uuid::String - if dep === name - return PkgId(UUID(uuid), name) + for deps′ in [weakdeps, deps] + if deps′ !== nothing + if deps′ isa Vector{String} + found_name = name in deps′ + found_name && @goto done + elseif deps′ isa Dict{String, Any} + deps′ = deps′::Dict{String, Any} + for (dep, uuid) in deps′ + uuid::String + if dep === name + return PkgId(UUID(uuid), name) + end + end end end end end - end # `name` is not an ext, do standard lookup as if this was the parent return identify_package(PkgId(UUID(uuid), dep_name), name) end @@ -984,6 +986,7 @@ function explicit_manifest_deps_get(project_file::String, where::PkgId, name::St end end end + @label done found_where || return nothing found_name || return PkgId(name) # Only reach here if deps was not a dict which mean we have a unique name for the dep From 7a1aaa905073b37e9b24af6c2e4e84e332e5245c Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Fri, 25 Oct 2024 10:30:28 -0400 Subject: [PATCH 6/9] drop require lock when not needed during loading to allow parallel precompile loading (#56291) Fixes `_require_search_from_serialized` to first acquire all start_loading locks (using a deadlock-free batch-locking algorithm) before doing stalechecks and the rest, so that all the global computations happen behind the require_lock, then the rest can happen behind module-specific locks, then (as before) extensions can be loaded in parallel eventually after `require` returns. (cherry picked from commit db3d816985cf307520e7e86ebedba2d674af1353) --- base/loading.jl | 272 ++++++++++++++++++++++++++++-------------------- 1 file changed, 158 insertions(+), 114 deletions(-) diff --git a/base/loading.jl b/base/loading.jl index 324ff8f657145..55873313ed9c5 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -1225,47 +1225,52 @@ function _include_from_serialized(pkg::PkgId, path::String, ocachepath::Union{No assert_havelock(require_lock) timing_imports = TIMING_IMPORTS[] > 0 try - if timing_imports - t_before = time_ns() - cumulative_compile_timing(true) - t_comp_before = cumulative_compile_time_ns() - end + if timing_imports + t_before = time_ns() + cumulative_compile_timing(true) + t_comp_before = cumulative_compile_time_ns() + end - for i in eachindex(depmods) - dep = depmods[i] - dep isa Module && continue - _, depkey, depbuild_id = dep::Tuple{String, PkgId, UInt128} - dep = something(maybe_loaded_precompile(depkey, depbuild_id)) - @assert PkgId(dep) == depkey && module_build_id(dep) === depbuild_id - depmods[i] = dep - end + for i in eachindex(depmods) + dep = depmods[i] + dep isa Module && continue + _, depkey, depbuild_id = dep::Tuple{String, PkgId, UInt128} + dep = something(maybe_loaded_precompile(depkey, depbuild_id)) + @assert PkgId(dep) == depkey && module_build_id(dep) === depbuild_id + depmods[i] = dep + end - if ocachepath !== nothing - @debug "Loading object cache file $ocachepath for $(repr("text/plain", pkg))" - sv = ccall(:jl_restore_package_image_from_file, Any, (Cstring, Any, Cint, Cstring, Cint), ocachepath, depmods, false, pkg.name, ignore_native) - else - @debug "Loading cache file $path for $(repr("text/plain", pkg))" - sv = ccall(:jl_restore_incremental, Any, (Cstring, Any, Cint, Cstring), path, depmods, false, pkg.name) - end - if isa(sv, Exception) - return sv - end + unlock(require_lock) # temporarily _unlock_ during these operations + sv = try + if ocachepath !== nothing + @debug "Loading object cache file $ocachepath for $(repr("text/plain", pkg))" + ccall(:jl_restore_package_image_from_file, Any, (Cstring, Any, Cint, Cstring, Cint), ocachepath, depmods, false, pkg.name, ignore_native) + else + @debug "Loading cache file $path for $(repr("text/plain", pkg))" + ccall(:jl_restore_incremental, Any, (Cstring, Any, Cint, Cstring), path, depmods, false, pkg.name) + end + finally + lock(require_lock) + end + if isa(sv, Exception) + return sv + end - restored = register_restored_modules(sv, pkg, path) + restored = register_restored_modules(sv, pkg, path) - for M in restored - M = M::Module - if parentmodule(M) === M && PkgId(M) == pkg - register && register_root_module(M) - if timing_imports - elapsed_time = time_ns() - t_before - comp_time, recomp_time = cumulative_compile_time_ns() .- t_comp_before - print_time_imports_report(M, elapsed_time, comp_time, recomp_time) + for M in restored + M = M::Module + if parentmodule(M) === M && PkgId(M) == pkg + register && register_root_module(M) + if timing_imports + elapsed_time = time_ns() - t_before + comp_time, recomp_time = cumulative_compile_time_ns() .- t_comp_before + print_time_imports_report(M, elapsed_time, comp_time, recomp_time) + end + return M end - return M end - end - return ErrorException("Required dependency $(repr("text/plain", pkg)) failed to load from a cache file.") + return ErrorException("Required dependency $(repr("text/plain", pkg)) failed to load from a cache file.") finally timing_imports && cumulative_compile_timing(false) @@ -1982,13 +1987,46 @@ end if staledeps === true continue end - try - staledeps, ocachefile, newbuild_id = staledeps::Tuple{Vector{Any}, Union{Nothing, String}, UInt128} - # finish checking staledeps module graph - for i in eachindex(staledeps) + staledeps, ocachefile, newbuild_id = staledeps::Tuple{Vector{Any}, Union{Nothing, String}, UInt128} + startedloading = length(staledeps) + 1 + try # any exit from here (goto, break, continue, return) will end_loading + # finish checking staledeps module graph, while acquiring all start_loading locks + # so that concurrent require calls won't make any different decisions that might conflict with the decisions here + # note that start_loading will drop the loading lock if necessary + let i = 0 + # start_loading here has a deadlock problem if we try to load `A,B,C` and `B,A,D` at the same time: + # it will claim A,B have a cycle, but really they just have an ambiguous order and need to be batch-acquired rather than singly + # solve that by making sure we can start_loading everything before allocating each of those and doing all the stale checks + while i < length(staledeps) + i += 1 + dep = staledeps[i] + dep isa Module && continue + _, modkey, modbuild_id = dep::Tuple{String, PkgId, UInt128} + dep = canstart_loading(modkey, modbuild_id, stalecheck) + if dep isa Module + if PkgId(dep) == modkey && module_build_id(dep) === modbuild_id + staledeps[i] = dep + continue + else + @debug "Rejecting cache file $path_to_try because module $modkey got loaded at a different version than expected." + @goto check_next_path + end + continue + elseif dep === nothing + continue + end + wait(dep) # releases require_lock, so requires restarting this loop + i = 0 + end + end + for i in reverse(eachindex(staledeps)) dep = staledeps[i] dep isa Module && continue modpath, modkey, modbuild_id = dep::Tuple{String, PkgId, UInt128} + # inline a call to start_loading here + @assert canstart_loading(modkey, modbuild_id, stalecheck) === nothing + package_locks[modkey] = current_task() => Threads.Condition(require_lock) + startedloading = i modpaths = find_all_in_cache_path(modkey, DEPOT_PATH) for modpath_to_try in modpaths modstaledeps = stale_cachefile(modkey, modbuild_id, modpath, modpath_to_try; stalecheck) @@ -2016,37 +2054,22 @@ end end end # finish loading module graph into staledeps - # TODO: call all start_loading calls (in reverse order) before calling any _include_from_serialized, since start_loading will drop the loading lock + # n.b. this runs __init__ methods too early, so it is very unwise to have those, as they may see inconsistent loading state, causing them to fail unpredictably here for i in eachindex(staledeps) dep = staledeps[i] dep isa Module && continue modpath, modkey, modbuild_id, modcachepath, modstaledeps, modocachepath = dep::Tuple{String, PkgId, UInt128, String, Vector{Any}, Union{Nothing, String}} - dep = start_loading(modkey, modbuild_id, stalecheck) - while true - if dep isa Module - if PkgId(dep) == modkey && module_build_id(dep) === modbuild_id - break - else - @debug "Rejecting cache file $path_to_try because module $modkey got loaded at a different version than expected." - @goto check_next_path - end - end - if dep === nothing - try - set_pkgorigin_version_path(modkey, modpath) - dep = _include_from_serialized(modkey, modcachepath, modocachepath, modstaledeps; register = stalecheck) - finally - end_loading(modkey, dep) - end - if !isa(dep, Module) - @debug "Rejecting cache file $path_to_try because required dependency $modkey failed to load from cache file for $modcachepath." exception=dep - @goto check_next_path - else - push!(newdeps, modkey) - end - end + set_pkgorigin_version_path(modkey, modpath) + dep = _include_from_serialized(modkey, modcachepath, modocachepath, modstaledeps; register = stalecheck) + if !isa(dep, Module) + @debug "Rejecting cache file $path_to_try because required dependency $modkey failed to load from cache file for $modcachepath." exception=dep + @goto check_next_path + else + startedloading = i + 1 + end_loading(modkey, dep) + staledeps[i] = dep + push!(newdeps, modkey) end - staledeps[i] = dep end restored = maybe_loaded_precompile(pkg, newbuild_id) if !isa(restored, Module) @@ -2056,11 +2079,21 @@ end @debug "Deserialization checks failed while attempting to load cache from $path_to_try" exception=restored @label check_next_path finally + # cancel all start_loading locks that were taken but not fulfilled before failing + for i in startedloading:length(staledeps) + dep = staledeps[i] + dep isa Module && continue + if dep isa Tuple{String, PkgId, UInt128} + _, modkey, _ = dep + else + _, modkey, _ = dep::Tuple{String, PkgId, UInt128, String, Vector{Any}, Union{Nothing, String}} + end + end_loading(modkey, nothing) + end for modkey in newdeps insert_extension_triggers(modkey) stalecheck && run_package_callbacks(modkey) end - empty!(newdeps) end end end @@ -2073,66 +2106,76 @@ const package_locks = Dict{PkgId,Pair{Task,Threads.Condition}}() debug_loading_deadlocks::Bool = true # Enable a slightly more expensive, but more complete algorithm that can handle simultaneous tasks. # This only triggers if you have multiple tasks trying to load the same package at the same time, # so it is unlikely to make a performance difference normally. -function start_loading(modkey::PkgId, build_id::UInt128, stalecheck::Bool) - # handle recursive and concurrent calls to require + +function canstart_loading(modkey::PkgId, build_id::UInt128, stalecheck::Bool) assert_havelock(require_lock) require_lock.reentrancy_cnt == 1 || throw(ConcurrencyViolationError("recursive call to start_loading")) - while true - loaded = stalecheck ? maybe_root_module(modkey) : nothing + loaded = stalecheck ? maybe_root_module(modkey) : nothing + loaded isa Module && return loaded + if build_id != UInt128(0) + loaded = maybe_loaded_precompile(modkey, build_id) loaded isa Module && return loaded - if build_id != UInt128(0) - loaded = maybe_loaded_precompile(modkey, build_id) - loaded isa Module && return loaded + end + loading = get(package_locks, modkey, nothing) + loading === nothing && return nothing + # load already in progress for this module on the task + task, cond = loading + deps = String[modkey.name] + pkgid = modkey + assert_havelock(cond.lock) + if debug_loading_deadlocks && current_task() !== task + waiters = Dict{Task,Pair{Task,PkgId}}() # invert to track waiting tasks => loading tasks + for each in package_locks + cond2 = each[2][2] + assert_havelock(cond2.lock) + for waiting in cond2.waitq + push!(waiters, waiting => (each[2][1] => each[1])) + end end - loading = get(package_locks, modkey, nothing) - if loading === nothing - package_locks[modkey] = current_task() => Threads.Condition(require_lock) - return nothing + while true + running = get(waiters, task, nothing) + running === nothing && break + task, pkgid = running + push!(deps, pkgid.name) + task === current_task() && break end - # load already in progress for this module on the task - task, cond = loading - deps = String[modkey.name] - pkgid = modkey - assert_havelock(cond.lock) - if debug_loading_deadlocks && current_task() !== task - waiters = Dict{Task,Pair{Task,PkgId}}() # invert to track waiting tasks => loading tasks - for each in package_locks - cond2 = each[2][2] - assert_havelock(cond2.lock) - for waiting in cond2.waitq - push!(waiters, waiting => (each[2][1] => each[1])) - end - end - while true - running = get(waiters, task, nothing) - running === nothing && break - task, pkgid = running - push!(deps, pkgid.name) - task === current_task() && break - end - end - if current_task() === task - others = String[modkey.name] # repeat this to emphasize the cycle here - for each in package_locks # list the rest of the packages being loaded too - if each[2][1] === task - other = each[1].name - other == modkey.name || other == pkgid.name || push!(others, other) - end - end - msg = sprint(deps, others) do io, deps, others - print(io, "deadlock detected in loading ") - join(io, deps, " -> ") - print(io, " -> ") - join(io, others, " && ") + end + if current_task() === task + others = String[modkey.name] # repeat this to emphasize the cycle here + for each in package_locks # list the rest of the packages being loaded too + if each[2][1] === task + other = each[1].name + other == modkey.name || other == pkgid.name || push!(others, other) end - throw(ConcurrencyViolationError(msg)) end - loaded = wait(cond) + msg = sprint(deps, others) do io, deps, others + print(io, "deadlock detected in loading ") + join(io, deps, " -> ") + print(io, " -> ") + join(io, others, " && ") + end + throw(ConcurrencyViolationError(msg)) + end + return cond +end + +function start_loading(modkey::PkgId, build_id::UInt128, stalecheck::Bool) + # handle recursive and concurrent calls to require + while true + loaded = canstart_loading(modkey, build_id, stalecheck) + if loaded === nothing + package_locks[modkey] = current_task() => Threads.Condition(require_lock) + return nothing + elseif loaded isa Module + return loaded + end + loaded = wait(loaded) loaded isa Module && return loaded end end function end_loading(modkey::PkgId, @nospecialize loaded) + assert_havelock(require_lock) loading = pop!(package_locks, modkey) notify(loading[2], loaded, all=true) nothing @@ -2622,6 +2665,7 @@ function _require(pkg::PkgId, env=nothing) end # load a serialized file directly, including dependencies (without checking staleness except for immediate conflicts) +# this does not call start_loading / end_loading, so can lead to some odd behaviors function _require_from_serialized(uuidkey::PkgId, path::String, ocachepath::Union{String, Nothing}, sourcepath::String) @lock require_lock begin set_pkgorigin_version_path(uuidkey, sourcepath) From 501b263daed031c2504115d06b5ec93eededb39d Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Mon, 28 Oct 2024 12:51:55 -0400 Subject: [PATCH 7/9] loading: clean up more concurrency issues (#56329) Guarantee that `__init__` runs before `using` returns. Could be slightly breaking for people that do crazy things inside `__init__`, but just don't do that. Since extensions then probably load after `__init__` (or at least, run their `__init__` after), this is a partial step towards changing things so that extensions are guaranteed to load if using all of their triggers before the corresponding `using` returns Fixes #55556 (cherry picked from commit 9dbdeb4fb323dfc78fde96a82bbb8fdefeb61a70) --- base/Base.jl | 4 +- base/loading.jl | 137 +++++++++++++----------------- base/methodshow.jl | 12 +-- contrib/generate_precompile.jl | 4 +- stdlib/REPL/src/Pkg_beforeload.jl | 12 +-- 5 files changed, 71 insertions(+), 98 deletions(-) diff --git a/base/Base.jl b/base/Base.jl index ab8493cd6fd32..768372d7a6ba4 100644 --- a/base/Base.jl +++ b/base/Base.jl @@ -603,8 +603,8 @@ function __init__() init_active_project() append!(empty!(_sysimage_modules), keys(loaded_modules)) empty!(loaded_precompiles) # If we load a packageimage when building the image this might not be empty - for (mod, key) in module_keys - push!(get!(Vector{Module}, loaded_precompiles, key), mod) + for mod in loaded_modules_order + push!(get!(Vector{Module}, loaded_precompiles, PkgId(mod)), mod) end if haskey(ENV, "JULIA_MAX_NUM_PRECOMPILE_FILES") MAX_NUM_PRECOMPILE_FILES[] = parse(Int, ENV["JULIA_MAX_NUM_PRECOMPILE_FILES"]) diff --git a/base/loading.jl b/base/loading.jl index 55873313ed9c5..56297cea917f0 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -523,8 +523,7 @@ See also [`pkgdir`](@ref). """ function pathof(m::Module) @lock require_lock begin - pkgid = get(module_keys, m, nothing) - pkgid === nothing && return nothing + pkgid = PkgId(m) origin = get(pkgorigins, pkgid, nothing) origin === nothing && return nothing path = origin.path @@ -1614,7 +1613,7 @@ get_extension(parent::Module, ext::Symbol) = get_extension(PkgId(parent), ext) function get_extension(parentid::PkgId, ext::Symbol) parentid.uuid === nothing && return nothing extid = PkgId(uuid5(parentid.uuid, string(ext)), string(ext)) - return get(loaded_modules, extid, nothing) + return maybe_root_module(extid) end # End extensions @@ -1773,7 +1772,7 @@ function show(io::IO, it::ImageTarget) end # should sync with the types of arguments of `stale_cachefile` -const StaleCacheKey = Tuple{Base.PkgId, UInt128, String, String} +const StaleCacheKey = Tuple{PkgId, UInt128, String, String} function compilecache_path(pkg::PkgId; ignore_loaded::Bool=false, @@ -2025,7 +2024,7 @@ end modpath, modkey, modbuild_id = dep::Tuple{String, PkgId, UInt128} # inline a call to start_loading here @assert canstart_loading(modkey, modbuild_id, stalecheck) === nothing - package_locks[modkey] = current_task() => Threads.Condition(require_lock) + package_locks[modkey] = (current_task(), Threads.Condition(require_lock), modbuild_id) startedloading = i modpaths = find_all_in_cache_path(modkey, DEPOT_PATH) for modpath_to_try in modpaths @@ -2101,7 +2100,7 @@ end end # to synchronize multiple tasks trying to import/using something -const package_locks = Dict{PkgId,Pair{Task,Threads.Condition}}() +const package_locks = Dict{PkgId,Tuple{Task,Threads.Condition,UInt128}}() debug_loading_deadlocks::Bool = true # Enable a slightly more expensive, but more complete algorithm that can handle simultaneous tasks. # This only triggers if you have multiple tasks trying to load the same package at the same time, @@ -2110,14 +2109,21 @@ debug_loading_deadlocks::Bool = true # Enable a slightly more expensive, but mor function canstart_loading(modkey::PkgId, build_id::UInt128, stalecheck::Bool) assert_havelock(require_lock) require_lock.reentrancy_cnt == 1 || throw(ConcurrencyViolationError("recursive call to start_loading")) - loaded = stalecheck ? maybe_root_module(modkey) : nothing - loaded isa Module && return loaded - if build_id != UInt128(0) + loading = get(package_locks, modkey, nothing) + if loading === nothing + loaded = stalecheck ? maybe_root_module(modkey) : nothing + loaded isa Module && return loaded + if build_id != UInt128(0) + loaded = maybe_loaded_precompile(modkey, build_id) + loaded isa Module && return loaded + end + return nothing + end + if !stalecheck && build_id != UInt128(0) && loading[3] != build_id + # don't block using an existing specific loaded module on needing a different concurrently loaded one loaded = maybe_loaded_precompile(modkey, build_id) loaded isa Module && return loaded end - loading = get(package_locks, modkey, nothing) - loading === nothing && return nothing # load already in progress for this module on the task task, cond = loading deps = String[modkey.name] @@ -2164,7 +2170,7 @@ function start_loading(modkey::PkgId, build_id::UInt128, stalecheck::Bool) while true loaded = canstart_loading(modkey, build_id, stalecheck) if loaded === nothing - package_locks[modkey] = current_task() => Threads.Condition(require_lock) + package_locks[modkey] = (current_task(), Threads.Condition(require_lock), build_id) return nothing elseif loaded isa Module return loaded @@ -2295,15 +2301,15 @@ For more details regarding code loading, see the manual sections on [modules](@r [parallel computing](@ref code-availability). """ function require(into::Module, mod::Symbol) - if _require_world_age[] != typemax(UInt) - Base.invoke_in_world(_require_world_age[], __require, into, mod) - else - @invokelatest __require(into, mod) + world = _require_world_age[] + if world == typemax(UInt) + world = get_world_counter() end + return invoke_in_world(world, __require, into, mod) end function __require(into::Module, mod::Symbol) - if into === Base.__toplevel__ && generating_output(#=incremental=#true) + if into === __toplevel__ && generating_output(#=incremental=#true) error("`using/import $mod` outside of a Module detected. Importing a package outside of a module \ is not allowed during package precompilation.") end @@ -2407,24 +2413,22 @@ function collect_manifest_warnings() return msg end -require(uuidkey::PkgId) = @lock require_lock _require_prelocked(uuidkey) - -function _require_prelocked(uuidkey::PkgId, env=nothing) - if _require_world_age[] != typemax(UInt) - Base.invoke_in_world(_require_world_age[], __require_prelocked, uuidkey, env) - else - @invokelatest __require_prelocked(uuidkey, env) +function require(uuidkey::PkgId) + world = _require_world_age[] + if world == typemax(UInt) + world = get_world_counter() end + return invoke_in_world(world, __require, uuidkey) end - -function __require_prelocked(uuidkey::PkgId, env=nothing) +__require(uuidkey::PkgId) = @lock require_lock _require_prelocked(uuidkey) +function _require_prelocked(uuidkey::PkgId, env=nothing) assert_havelock(require_lock) m = start_loading(uuidkey, UInt128(0), true) if m === nothing last = toplevel_load[] try toplevel_load[] = false - m = _require(uuidkey, env) + m = __require_prelocked(uuidkey, env) if m === nothing error("package `$(uuidkey.name)` did not define the expected \ module `$(uuidkey.name)`, check for typos in package module name") @@ -2436,8 +2440,6 @@ function __require_prelocked(uuidkey::PkgId, env=nothing) insert_extension_triggers(uuidkey) # After successfully loading, notify downstream consumers run_package_callbacks(uuidkey) - else - newm = root_module(uuidkey) end return m end @@ -2453,10 +2455,9 @@ const pkgorigins = Dict{PkgId,PkgOrigin}() const loaded_modules = Dict{PkgId,Module}() # available to be explicitly loaded const loaded_precompiles = Dict{PkgId,Vector{Module}}() # extended (complete) list of modules, available to be loaded const loaded_modules_order = Vector{Module}() -const module_keys = IdDict{Module,PkgId}() # the reverse of loaded_modules -is_root_module(m::Module) = @lock require_lock haskey(module_keys, m) -root_module_key(m::Module) = @lock require_lock module_keys[m] +is_root_module(m::Module) = parentmodule(m) === m || m === Base +root_module_key(m::Module) = PkgId(m) function maybe_loaded_precompile(key::PkgId, buildid::UInt128) assert_havelock(require_lock) @@ -2489,7 +2490,6 @@ end end maybe_loaded_precompile(key, module_build_id(m)) === nothing && push!(loaded_modules_order, m) loaded_modules[key] = m - module_keys[m] = key end nothing end @@ -2506,24 +2506,27 @@ using Base end # get a top-level Module from the given key +# this is similar to `require`, but worse in almost every possible way root_module(key::PkgId) = @lock require_lock loaded_modules[key] function root_module(where::Module, name::Symbol) key = identify_package(where, String(name)) key isa PkgId || throw(KeyError(name)) return root_module(key) end +root_module_exists(key::PkgId) = @lock require_lock haskey(loaded_modules, key) maybe_root_module(key::PkgId) = @lock require_lock get(loaded_modules, key, nothing) -root_module_exists(key::PkgId) = @lock require_lock haskey(loaded_modules, key) loaded_modules_array() = @lock require_lock copy(loaded_modules_order) # after unreference_module, a subsequent require call will try to load a new copy of it, if stale # reload(m) = (unreference_module(m); require(m)) function unreference_module(key::PkgId) + @lock require_lock begin if haskey(loaded_modules, key) m = pop!(loaded_modules, key) # need to ensure all modules are GC rooted; will still be referenced - # in module_keys + # in loaded_modules_order + end end end @@ -2544,7 +2547,7 @@ const PKG_PRECOMPILE_HOOK = Ref{Function}() disable_parallel_precompile::Bool = false # Returns `nothing` or the new(ish) module -function _require(pkg::PkgId, env=nothing) +function __require_prelocked(pkg::PkgId, env) assert_havelock(require_lock) # perform the search operation to select the module file require intends to load @@ -2654,7 +2657,7 @@ function _require(pkg::PkgId, env=nothing) unlock(require_lock) try include(__toplevel__, path) - loaded = get(loaded_modules, pkg, nothing) + loaded = maybe_root_module(pkg) finally lock(require_lock) if uuid !== old_uuid @@ -2687,38 +2690,18 @@ function require_stdlib(package_uuidkey::PkgId, ext::Union{Nothing, String}=noth @lock require_lock begin # the PkgId of the ext, or package if not an ext this_uuidkey = ext isa String ? PkgId(uuid5(package_uuidkey.uuid, ext), ext) : package_uuidkey - newm = maybe_root_module(this_uuidkey) - if newm isa Module - return newm - end - # first since this is a stdlib, try to look there directly first env = Sys.STDLIB - #sourcepath = "" - if ext === nothing - sourcepath = normpath(env, this_uuidkey.name, "src", this_uuidkey.name * ".jl") - else - sourcepath = find_ext_path(normpath(joinpath(env, package_uuidkey.name)), ext) - end - #mbypath = manifest_uuid_path(env, this_uuidkey) - #if mbypath isa String && isfile_casesensitive(mbypath) - # sourcepath = mbypath - #else - # # if the user deleted the stdlib folder, we next try using their environment - # sourcepath = locate_package_env(this_uuidkey) - # if sourcepath !== nothing - # sourcepath, env = sourcepath - # end - #end - #if sourcepath === nothing - # throw(ArgumentError(""" - # Package $(repr("text/plain", this_uuidkey)) is required but does not seem to be installed. - # """)) - #end - set_pkgorigin_version_path(this_uuidkey, sourcepath) - depot_path = append_bundled_depot_path!(empty(DEPOT_PATH)) newm = start_loading(this_uuidkey, UInt128(0), true) newm === nothing || return newm try + # first since this is a stdlib, try to look there directly first + if ext === nothing + sourcepath = normpath(env, this_uuidkey.name, "src", this_uuidkey.name * ".jl") + else + sourcepath = find_ext_path(normpath(joinpath(env, package_uuidkey.name)), ext) + end + depot_path = append_bundled_depot_path!(empty(DEPOT_PATH)) + set_pkgorigin_version_path(this_uuidkey, sourcepath) newm = _require_search_from_serialized(this_uuidkey, sourcepath, UInt128(0), false; DEPOT_PATH=depot_path) finally end_loading(this_uuidkey, newm) @@ -3852,32 +3835,32 @@ end if M !== nothing @assert PkgId(M) == req_key && module_build_id(M) === req_build_id depmods[i] = M - elseif root_module_exists(req_key) - M = root_module(req_key) + continue + end + M = maybe_root_module(req_key) + if M isa Module if PkgId(M) == req_key && module_build_id(M) === req_build_id depmods[i] = M + continue elseif M == Core @debug "Rejecting cache file $cachefile because it was made with a different julia version" record_reason(reasons, "wrong julia version") return true # Won't be able to fulfill dependency elseif ignore_loaded || !stalecheck # Used by Pkg.precompile given that there it's ok to precompile different versions of loaded packages - @goto locate_branch else @debug "Rejecting cache file $cachefile because module $req_key is already loaded and incompatible." record_reason(reasons, "wrong dep version loaded") return true # Won't be able to fulfill dependency end - else - @label locate_branch - path = locate_package(req_key) # TODO: add env and/or skip this when stalecheck is false - if path === nothing - @debug "Rejecting cache file $cachefile because dependency $req_key not found." - record_reason(reasons, "dep missing source") - return true # Won't be able to fulfill dependency - end - depmods[i] = (path, req_key, req_build_id) end + path = locate_package(req_key) # TODO: add env and/or skip this when stalecheck is false + if path === nothing + @debug "Rejecting cache file $cachefile because dependency $req_key not found." + record_reason(reasons, "dep missing source") + return true # Won't be able to fulfill dependency + end + depmods[i] = (path, req_key, req_build_id) end # check if this file is going to provide one of our concrete dependencies diff --git a/base/methodshow.jl b/base/methodshow.jl index 477acd2960c48..a2158cb9180e4 100644 --- a/base/methodshow.jl +++ b/base/methodshow.jl @@ -378,7 +378,6 @@ function url(m::Method) line = m.line line <= 0 || occursin(r"In\[[0-9]+\]"a, file) && return "" Sys.iswindows() && (file = replace(file, '\\' => '/')) - libgit2_id = PkgId(UUID((0x76f85450_5226_5b5a,0x8eaa_529ad045b433)), "LibGit2") if inbase(M) if isempty(Base.GIT_VERSION_INFO.commit) # this url will only work if we're on a tagged release @@ -386,8 +385,10 @@ function url(m::Method) else return "https://github.com/JuliaLang/julia/tree/$(Base.GIT_VERSION_INFO.commit)/base/$file#L$line" end - elseif root_module_exists(libgit2_id) - LibGit2 = root_module(libgit2_id) + end + libgit2_id = PkgId(UUID((0x76f85450_5226_5b5a,0x8eaa_529ad045b433)), "LibGit2") + LibGit2 = maybe_root_module(libgit2_id) + if LibGit2 isa Module try d = dirname(file) return LibGit2.with(LibGit2.GitRepoExt(d)) do repo @@ -404,11 +405,10 @@ function url(m::Method) end end catch - return fileurl(file) + # oops, this was a bad idea end - else - return fileurl(file) end + return fileurl(file) end function show(io::IO, ::MIME"text/html", m::Method) diff --git a/contrib/generate_precompile.jl b/contrib/generate_precompile.jl index 49bcf3f43ea41..8f7b3ee765de8 100644 --- a/contrib/generate_precompile.jl +++ b/contrib/generate_precompile.jl @@ -37,8 +37,8 @@ precompile(Base.unsafe_string, (Ptr{UInt8},)) precompile(Base.unsafe_string, (Ptr{Int8},)) # loading.jl -precompile(Base.__require_prelocked, (Base.PkgId, Nothing)) -precompile(Base._require, (Base.PkgId, Nothing)) +precompile(Base.__require, (Module, Symbol)) +precompile(Base.__require, (Base.PkgId,)) precompile(Base.indexed_iterate, (Pair{Symbol, Union{Nothing, String}}, Int)) precompile(Base.indexed_iterate, (Pair{Symbol, Union{Nothing, String}}, Int, Int)) precompile(Tuple{typeof(Base.Threads.atomic_add!), Base.Threads.Atomic{Int}, Int}) diff --git a/stdlib/REPL/src/Pkg_beforeload.jl b/stdlib/REPL/src/Pkg_beforeload.jl index 407ee5bf01e92..98df958ecb3ce 100644 --- a/stdlib/REPL/src/Pkg_beforeload.jl +++ b/stdlib/REPL/src/Pkg_beforeload.jl @@ -1,17 +1,7 @@ ## Pkg stuff needed before Pkg has loaded const Pkg_pkgid = Base.PkgId(Base.UUID("44cfe95a-1eb2-52ea-b672-e2afdf69b78f"), "Pkg") - -function load_pkg() - REPLExt = Base.require_stdlib(Pkg_pkgid, "REPLExt") - @lock Base.require_lock begin - # require_stdlib does not guarantee that the `__init__` of the package is done when loading is done async - # but we need to wait for the repl mode to be set up - lock = get(Base.package_locks, Base.PkgId(REPLExt), nothing) - lock !== nothing && wait(lock[2]) - end - return REPLExt -end +load_pkg() = Base.require_stdlib(Pkg_pkgid, "REPLExt") ## Below here copied/tweaked from Pkg Types.jl so that the dummy Pkg prompt # can populate the env correctly before Pkg loads From ee4120fb92a220f2158901c1c3aa0fd32e7ee957 Mon Sep 17 00:00:00 2001 From: Ian Butterworth Date: Thu, 7 Nov 2024 08:09:43 -0500 Subject: [PATCH 8/9] fixup #55908 backport --- test/loading.jl | 1 - 1 file changed, 1 deletion(-) diff --git a/test/loading.jl b/test/loading.jl index fa90739b74fc9..f643b14ed7e9a 100644 --- a/test/loading.jl +++ b/test/loading.jl @@ -34,7 +34,6 @@ end @test @nested_LINE_expansion2() == ((@__LINE__() - 5, @__LINE__() - 9), @__LINE__()) original_depot_path = copy(Base.DEPOT_PATH) -include("precompile_utils.jl") loaded_files = String[] push!(Base.include_callbacks, (mod::Module, fn::String) -> push!(loaded_files, fn)) From e71fddabd937d3d184817df78a3bafb5b996a976 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Mon, 7 Oct 2024 19:54:51 -0400 Subject: [PATCH 9/9] add require_lock call to maybe_loaded_precompile (#56027) If we expect this to be a public API (https://github.com/timholy/Revise.jl for some reason is trying to access this state), we should lock around it for consistency with the other similar functions. Needed for https://github.com/timholy/Revise.jl/pull/856 (cherry picked from commit 4cdd864e535b16e928c2eff43e6f1583bd77209d) --- base/loading.jl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/base/loading.jl b/base/loading.jl index 56297cea917f0..7ea022c135a0b 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -2460,12 +2460,13 @@ is_root_module(m::Module) = parentmodule(m) === m || m === Base root_module_key(m::Module) = PkgId(m) function maybe_loaded_precompile(key::PkgId, buildid::UInt128) - assert_havelock(require_lock) + @lock require_lock begin mods = get(loaded_precompiles, key, nothing) mods === nothing && return for mod in mods module_build_id(mod) == buildid && return mod end + end end function module_build_id(m::Module)