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

much faster code-coverage for packages #57988

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+85 −64
Open

much faster code-coverage for packages #57988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 13 additions & 11 deletions Compiler/src/inferencestate.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,21 +575,23 @@ function (::ComputeTryCatch{Handler})(code::Vector{Any}, bbs::Union{Vector{Basic
end

# check if coverage mode is enabled
function should_insert_coverage(mod::Module, debuginfo::DebugInfo)
coverage_enabled(mod) && return true
JLOptions().code_coverage == 3 || return false
should_insert_coverage(mod::Module, debuginfo::DebugInfo) = should_instrument(mod, debuginfo, true)

function should_instrument(mod::Module, debuginfo::DebugInfo, only_if_affects_optimizer::Bool=false)
instrumentation_enabled(mod, only_if_affects_optimizer) && return true
JLOptions().code_coverage == 3 || JLOptions().malloc_log == 3 || return false
# path-specific coverage mode: if any line falls in a tracked file enable coverage for all
return _should_insert_coverage(debuginfo)
return _should_instrument(debuginfo)
end

_should_insert_coverage(mod::Symbol) = is_file_tracked(mod)
_should_insert_coverage(mod::Method) = _should_insert_coverage(mod.file)
_should_insert_coverage(mod::MethodInstance) = _should_insert_coverage(mod.def)
_should_insert_coverage(mod::Module) = false
function _should_insert_coverage(info::DebugInfo)
_should_instrument(loc::Symbol) = is_file_tracked(loc)
_should_instrument(loc::Method) = _should_instrument(loc.file)
_should_instrument(loc::MethodInstance) = _should_instrument(loc.def)
_should_instrument(loc::Module) = false
function _should_instrument(info::DebugInfo)
linetable = info.linetable
linetable === nothing || (_should_insert_coverage(linetable) && return true)
_should_insert_coverage(info.def) && return true
linetable === nothing || (_should_instrument(linetable) && return true)
_should_instrument(info.def) && return true
return false
end

Expand Down
13 changes: 12 additions & 1 deletion Compiler/src/utilities.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ end

inlining_enabled() = (JLOptions().can_inline == 1)

function coverage_enabled(m::Module)
function instrumentation_enabled(m::Module, only_if_affects_optimizer::Bool)
Copy link
Member

Choose a reason for hiding this comment

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

The name here sounds like it means "check whether instrumentation is enabled for m" but really it seems to determine whether m needs to have instrumentation. Maybe requires_instrumentation?

generating_output() && return false # don't alter caches
cov = JLOptions().code_coverage
if cov == 1 # user
Expand All @@ -340,6 +340,17 @@ function coverage_enabled(m::Module)
elseif cov == 2 # all
return true
end
if !only_if_affects_optimizer
log = JLOptions().malloc_log
if log == 1 # user
m = moduleroot(m)
m === Core && return false
isdefined(Main, :Base) && m === Main.Base && return false
return true
elseif log == 2 # all
return true
end
end
return false
end

Expand Down
47 changes: 3 additions & 44 deletions base/loading.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1243,21 +1243,7 @@ const TIMING_IMPORTS = Threads.Atomic{Int}(0)
# these return either the array of modules loaded from the path / content given
# or an Exception that describes why it couldn't be loaded
# and it reconnects the Base.Docs.META
function _include_from_serialized(pkg::PkgId, path::String, ocachepath::Union{Nothing, String}, depmods::Vector{Any}, ignore_native::Union{Nothing,Bool}=nothing; register::Bool=true)
if isnothing(ignore_native)
if JLOptions().code_coverage == 0 && JLOptions().malloc_log == 0
ignore_native = false
else
io = open(path, "r")
try
iszero(isvalid_cache_header(io)) && return ArgumentError("Incompatible header in cache file $path.")
_, (includes, _, _), _, _, _, _, _, _ = parse_cache_header(io, path)
ignore_native = pkg_tracked(includes)
finally
close(io)
end
end
end
function _include_from_serialized(pkg::PkgId, path::String, ocachepath::Union{Nothing, String}, depmods::Vector{Any}; register::Bool=true)
assert_havelock(require_lock)
timing_imports = TIMING_IMPORTS[] > 0
try
Expand All @@ -1276,6 +1262,7 @@ function _include_from_serialized(pkg::PkgId, path::String, ocachepath::Union{No
depmods[i] = dep
end

ignore_native = false
unlock(require_lock) # temporarily _unlock_ during these operations
sv = try
if ocachepath !== nothing
Expand Down Expand Up @@ -1949,44 +1936,16 @@ function _tryrequire_from_serialized(modkey::PkgId, build_id::UInt128)
return ErrorException("Required dependency $modkey failed to load from a cache file.")
end

# returns whether the package is tracked in coverage or malloc tracking based on
# JLOptions and includes
function pkg_tracked(includes)
if JLOptions().code_coverage == 0 && JLOptions().malloc_log == 0
return false
elseif JLOptions().code_coverage == 1 || JLOptions().malloc_log == 1 # user
# Just say true. Pkgimages aren't in Base
return true
elseif JLOptions().code_coverage == 2 || JLOptions().malloc_log == 2 # all
return true
elseif JLOptions().code_coverage == 3 || JLOptions().malloc_log == 3 # tracked path
if JLOptions().tracked_path == C_NULL
return false
else
tracked_path = unsafe_string(JLOptions().tracked_path)
if isempty(tracked_path)
return false
else
return any(includes) do inc
startswith(inc.filename, tracked_path)
end
end
end
end
end

# loads a precompile cache file, ignoring stale_cachefile tests
# load all dependent modules first
function _tryrequire_from_serialized(pkg::PkgId, path::String, ocachepath::Union{Nothing, String})
assert_havelock(require_lock)
local depmodnames
io = open(path, "r")
ignore_native = false
try
iszero(isvalid_cache_header(io)) && return ArgumentError("Incompatible header in cache file $path.")
_, (includes, _, _), depmodnames, _, _, _, clone_targets, _ = parse_cache_header(io, path)

ignore_native = pkg_tracked(includes)

pkgimage = !isempty(clone_targets)
if pkgimage
Expand All @@ -2013,7 +1972,7 @@ function _tryrequire_from_serialized(pkg::PkgId, path::String, ocachepath::Union
depmods[i] = dep
end
# then load the file
loaded = _include_from_serialized(pkg, path, ocachepath, depmods, ignore_native; register = true)
loaded = _include_from_serialized(pkg, path, ocachepath, depmods; register = true)
return loaded
end

Expand Down
65 changes: 57 additions & 8 deletions base/staticdata.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

module StaticData

using Core: CodeInstance, MethodInstance
using Base: get_world_counter
using .Core: CodeInstance, MethodInstance
using .Base: JLOptions, Compiler, get_world_counter, _methods_by_ftype, get_methodtable

const WORLD_AGE_REVALIDATION_SENTINEL::UInt = 1
const _jl_debug_method_invalidation = Ref{Union{Nothing,Vector{Any}}}(nothing)
Expand Down Expand Up @@ -73,6 +73,51 @@ end

get_require_world() = unsafe_load(cglobal(:jl_require_world, UInt))

function gen_staged_sig(def::Method, mi::MethodInstance)
Copy link
Member

Choose a reason for hiding this comment

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

This file feels like a slightly strange place to define this, but obviously not really a problem

isdefined(def, :generator) || return nothing
isdispatchtuple(mi.specTypes) || return nothing
gen = Core.Typeof(def.generator)
return Tuple{gen, UInt, Method, Vararg}
## more precise method lookup, but more costly and likely not actually better?
#tts = (mi.specTypes::DataType).parameters
#sps = Any[Core.Typeof(mi.sparam_vals[i]) for i in 1:length(mi.sparam_vals)]
#if def.isva
# return Tuple{gen, UInt, Method, sps..., tts[1:def.nargs - 1]..., Tuple{tts[def.nargs - 1:end]...}}
#else
# return Tuple{gen, UInt, Method, sps..., tts...}
#end
end

function needs_instrumentation(codeinst::CodeInstance, mi::MethodInstance, def::Method)
if JLOptions().code_coverage != 0 || JLOptions().malloc_log != 0
# test if the code needs to run with instrumentation, in which case we cannot use existing generated code
if isdefined(def, :debuginfo) ? # generated_only functions do not have debuginfo, so fall back to considering their codeinst debuginfo though this may be slower (and less accurate?)
Compiler.should_instrument(def.module, def.debuginfo) :
Compiler.should_instrument(def.module, codeinst.debuginfo)
return true
end
gensig = gen_staged_sig(def, mi)
if gensig !== nothing
# if this is defined by a generator, try to consider forcing re-running the generators too, to add coverage for them
minworld = Ref{UInt}(1)
maxworld = Ref{UInt}(typemax(UInt))
has_ambig = Ref{Int32}(0)
result = _methods_by_ftype(gensig, nothing, -1, validation_world, #=ambig=#false, minworld, maxworld, has_ambig)
if result !== nothing
for k = 1:length(result)
match = result[k]::Core.MethodMatch
genmethod = match.method
# no, I refuse to refuse to recurse into your cursed generated function generators and will only test one level deep here
Copy link
Member

Choose a reason for hiding this comment

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

😆

if isdefined(genmethod, :debuginfo) && Compiler.should_instrument(genmethod.module, genmethod.debuginfo)
return true
end
end
end
end
end
return false
end

# Test all edges relevant to a method:
# - Visit the entire call graph, starting from edges[idx] to determine if that method is valid
# - Implements Tarjan's SCC (strongly connected components) algorithm, simplified to remove the count variable
Expand All @@ -84,6 +129,12 @@ function verify_method(codeinst::CodeInstance, stack::Vector{CodeInstance}, visi
return 0, world, max_valid2
end
end
mi = get_ci_mi(codeinst)
def = mi.def::Method
if needs_instrumentation(codeinst, mi, def)
return 0, world, UInt(0)
end

# Implicitly referenced bindings in the current module do not get explicit edges.
# If they were invalidated, they'll be in `mwis`. If they weren't, they imply a minworld
# of `get_require_world`. In principle, this is only required for methods that do reference
Expand All @@ -92,8 +143,6 @@ function verify_method(codeinst::CodeInstance, stack::Vector{CodeInstance}, visi
# but no implicit edges) is rare and there would be little benefit to lower the minworld for it
# in any case, so we just always use `get_require_world` here.
local minworld::UInt, maxworld::UInt = get_require_world(), validation_world
def = get_ci_mi(codeinst).def
@assert def isa Method
if haskey(visiting, codeinst)
return visiting[codeinst], minworld, maxworld
end
Expand Down Expand Up @@ -226,7 +275,7 @@ function verify_method(codeinst::CodeInstance, stack::Vector{CodeInstance}, visi
end
@atomic :monotonic child.max_world = maxworld
if maxworld == validation_world && validation_world == get_world_counter()
Base.Compiler.store_backedges(child, child.edges)
Compiler.store_backedges(child, child.edges)
end
@assert visiting[child] == length(stack) + 1
delete!(visiting, child)
Expand All @@ -244,7 +293,7 @@ function verify_call(@nospecialize(sig), expecteds::Core.SimpleVector, i::Int, n
minworld = Ref{UInt}(1)
maxworld = Ref{UInt}(typemax(UInt))
has_ambig = Ref{Int32}(0)
result = Base._methods_by_ftype(sig, nothing, lim, world, #=ambig=#false, minworld, maxworld, has_ambig)
result = _methods_by_ftype(sig, nothing, lim, world, #=ambig=#false, minworld, maxworld, has_ambig)
if result === nothing
maxworld[] = 0
else
Expand Down Expand Up @@ -306,11 +355,11 @@ function verify_invokesig(@nospecialize(invokesig), expected::Method, world::UIn
else
minworld = 1
maxworld = typemax(UInt)
mt = Base.get_methodtable(expected)
mt = get_methodtable(expected)
if mt === nothing
maxworld = 0
else
matched, valid_worlds = Base.Compiler._findsup(invokesig, mt, world)
matched, valid_worlds = Compiler._findsup(invokesig, mt, world)
minworld, maxworld = valid_worlds.min_world, valid_worlds.max_world
if matched === nothing
maxworld = 0
Expand Down