A few changes to the analysis of abstract signatures demo (#17)

demos/abstract.jl

@@ -1,20 +1,23 @@
 using MethodAnalysis
 
-
 """
-    atrisktyp(tt)
+    is_atrisk_type(tt)
 
 Given a Tuple-type signature (e.g., `Tuple{typeof(sum),Vector{Int}}`), determine whether this signature
 is "at risk" for invalidation. Essentially it returns `true` if one or more arguments are of abstract type,
 although there are prominent exceptions:
 
-- `Function` is allowed
-- any constructor call is allowed
-- `convert(X, x)` where `isa(x, X)` is true
-- `setindex!` and `push!` methods where the valtype is a subtype of the eltype (likewise keytype for AbstractDicts)
+- Constructor calls with arbitrary argument types
+- `convert(X, x)` where `isa(x, X)`
+- `setindex!` and `push!` methods where the valtype is a subtype of the eltype (for AbstractDicts, likewise for the keytype)
 - `getindex`, `length`, `isempty`, and `iterate` on any tuple
+
+All of these are "allowed," meaning that they return `false`.
+Moreover, some specific non-concrete argument types---like `Union`s of concrete types and `Function`---
+do not trigger a return of `true`, although other at-risk argument types can lead to an overall `true` return
+for the signature.
 """
-function atrisktype(@nospecialize(typ))
+function is_atrisk_type(@nospecialize(typ))
     # signatures like `convert(Vector, a)`, `foo(::Vararg{Synbol,N}) where N` do not seem to pose a problem
     isa(typ, TypeVar) && return false
     # isbits parameters are not a problem
@@ -24,7 +27,7 @@ function atrisktype(@nospecialize(typ))
     end
     # Exclude signatures with Union{}
     typ === Union{} && return false
-    isa(typ, Union) && return atrisktype(typ.a) | atrisktype(typ.b)
+    isa(typ, Union) && return is_atrisk_type(typ.a) | is_atrisk_type(typ.b)
     # Type{T}: signatures like `convert(::Type{AbstractString}, ::String)` are not problematic
     typ <: Type && return false
     if typ <: Tuple && length(typ.parameters) >= 1
@@ -38,6 +41,9 @@ function atrisktype(@nospecialize(typ))
                 p2 = Base.unwrap_unionall(p2)
                 if isa(p2, DataType) && length(p2.parameters) === 1
                     T = p2.parameters[1]
+                    if isa(T, TypeVar)
+                        T = T.ub
+                    end
                     isa(p3, Type) && isa(T, Type) && p3 <: T && return false
                 end
             end
@@ -53,9 +59,9 @@ function atrisktype(@nospecialize(typ))
             end
         # show(io::IO, x) is OK as long as typeof(x) is safe
         elseif p1 === typeof(Base.show) || p1 === typeof(Base.print) || p1 === typeof(Base.println)
-            # atrisktype(typ.parameters[2]) && return true
+            # is_atrisk_type(typ.parameters[2]) && return true
             for i = 3:length(typ.parameters)
-                atrisktype(typ.parameters[i]) && return true
+                is_atrisk_type(typ.parameters[i]) && return true
             end
             return false
         # setindex!(a, x, idx) and push!(a, x) are safe if typeof(x) <: eltype(a)
@@ -75,30 +81,46 @@ function atrisktype(@nospecialize(typ))
     isconcretetype(typ) && return false
     # ::Function args are excluded
     typ === Function && return false
-    !isempty(typ.parameters) && (any(atrisktype, typ.parameters) || return false)
+    !isempty(typ.parameters) && (any(is_atrisk_type, typ.parameters) || return false)
     return true
 end
 
-@assert  atrisktype(Tuple{typeof(==),Any,Any})
-@assert  atrisktype(Tuple{typeof(==),Symbol,Any})
-@assert  atrisktype(Tuple{typeof(==),Any,Symbol})
-@assert !atrisktype(Tuple{typeof(==),Symbol,Symbol})
-@assert !atrisktype(Tuple{typeof(convert),Type{Any},Any})
-@assert !atrisktype(Tuple{typeof(convert),Type{AbstractString},AbstractString})
-@assert !atrisktype(Tuple{typeof(convert),Type{AbstractString},String})
-@assert  atrisktype(Tuple{typeof(convert),Type{String},AbstractString})
-@assert !atrisktype(Tuple{typeof(map),Function,Vector{Any}})
-@assert !atrisktype(Tuple{typeof(getindex),Dict{Union{String,Int},Any},Union{String,Int}})
-@assert  atrisktype(Tuple{typeof(getindex),Dict{Union{String,Int},Any},Any})
-@assert !atrisktype(Tuple{Type{BoundsError},Any,Any})
-@assert  atrisktype(Tuple{typeof(sin),Any})
-@assert !atrisktype(Tuple{typeof(length),Tuple{Any,Any}})
-@assert  atrisktype(Tuple{typeof(setindex!),Vector{Int},Any,Int})
-@assert !atrisktype(Tuple{typeof(setindex!),Vector{Any},Any,Int})
-@assert  atrisktype(Tuple{typeof(push!),Vector{Int},Any})
-@assert !atrisktype(Tuple{typeof(push!),Vector{Any},Any})
+@assert  is_atrisk_type(Tuple{typeof(==),Any,Any})
+@assert  is_atrisk_type(Tuple{typeof(==),Symbol,Any})
+@assert  is_atrisk_type(Tuple{typeof(==),Any,Symbol})
+@assert !is_atrisk_type(Tuple{typeof(==),Symbol,Symbol})
+@assert !is_atrisk_type(Tuple{typeof(convert),Type{Any},Any})
+@assert !is_atrisk_type(Tuple{typeof(convert),Type{AbstractString},AbstractString})
+@assert !is_atrisk_type(Tuple{typeof(convert),Type{AbstractString},String})
+@assert  is_atrisk_type(Tuple{typeof(convert),Type{String},AbstractString})
+@assert !is_atrisk_type(Tuple{typeof(convert),Type{Union{Int,Float32}},Int})
+@assert !is_atrisk_type(Tuple{typeof(convert),Type{Union{Int,Float32}},Int32})
+@assert  is_atrisk_type(Tuple{typeof(convert),Type{Union{Int,Float32}},Integer})
+@assert !is_atrisk_type(Tuple{typeof(convert),Type{T} where T<:Union{Int,Float32},Int})
+@assert !is_atrisk_type(Tuple{typeof(map),Function,Vector{Any}})
+@assert !is_atrisk_type(Tuple{typeof(getindex),Dict{Union{String,Int},Any},Union{String,Int}})
+@assert  is_atrisk_type(Tuple{typeof(getindex),Dict{Union{String,Int},Any},Any})
+@assert !is_atrisk_type(Tuple{Type{BoundsError},Any,Any})
+@assert  is_atrisk_type(Tuple{typeof(sin),Any})
+@assert !is_atrisk_type(Tuple{typeof(length),Tuple{Any,Any}})
+@assert  is_atrisk_type(Tuple{typeof(setindex!),Vector{Int},Any,Int})
+@assert !is_atrisk_type(Tuple{typeof(setindex!),Vector{Any},Any,Int})
+@assert  is_atrisk_type(Tuple{typeof(push!),Vector{Int},Any})
+@assert !is_atrisk_type(Tuple{typeof(push!),Vector{Any},Any})
+
+# Get the name of a method as written in the code. This strips keyword-method mangling.
+function codename(sym::Symbol)
+    symstr = String(sym)
+    # Body methods
+    m = match(r"^#(.*?)#\d+$", symstr)
+    m !== nothing && return Symbol(only(m.captures))
+    # kw methods
+    m = match(r"^(.*?)##kw$", symstr)
+    m !== nothing && return Symbol(only(m.captures))
+    return sym
+end
 
-isexported(mi::Core.MethodInstance) = isdefined(Main, mi.def.name)
+isexported(mi::Core.MethodInstance) = isdefined(Main, codename(mi.def.name))
 getfunc(mi::Core.MethodInstance) = getfunc(mi.def)
 getfunc(m::Method) = getfield(m.module, m.name)
 nmethods(mi::Core.MethodInstance) = length(methods(getfunc(mi)))
@@ -124,7 +146,7 @@ end
 const becounter = Dict{Core.MethodInstance,Int}()
 visit() do item
     if item isa Core.MethodInstance && !fromcc(item.def.module)
-        if atrisktype(item.specTypes)
+        if is_atrisk_type(item.specTypes)
             becounter[item] = length(all_backedges(item))
         end
         return false
@@ -141,15 +163,12 @@ open("/tmp/methdata_$VERSION.log", "w") do io
 end
 
 # Split into exported & private functions
-mtup = (nmethods = 0, nbackedges = 0)
-miexp = Pair{Core.MethodInstance,typeof(mtup)}[]
+miexp = Pair{Core.MethodInstance,Int}[]
 mipriv = similar(miexp)
 for (mi, c) in prs
-    n = nmethods(mi)
-    pr = mi=>(nmethods=n, nbackedges=c)
     if isexported(mi)
-        push!(miexp, pr)
+        push!(miexp, mi=>c)
     else
-        push!(mipriv, pr)
+        push!(mipriv, mi=>c)
     end
 end

demos/abstract_tests.jl

@@ -0,0 +1,165 @@
+atrisk_backedges = with_all_backedges(keys(becounter))
+
+function atrisk_method(m::Method, atrisk_backedges)
+    for mi in methodinstances(m)
+        mi ∈ atrisk_backedges && return true
+    end
+    return false
+end
+
+function atrisk_triggers(m::Method, atrisk_instances)
+    triggers = Set{Core.MethodInstance}()
+    for mi in atrisk_instances
+        if atrisk_method(m, all_backedges(mi))
+            push!(triggers, mi)
+        end
+    end
+    return triggers
+end
+
+# This removes MethodInstances that no one in their right mind should ever invalidate by specialization.
+function remove_unlikely_methodinstances(list)
+    out = Core.MethodInstance[]
+    for mi in list
+        mi = mi::Core.MethodInstance   # must have MethodInstance elements
+        # All `continue` statements below omit the MethodInstance
+        name = codename(mi.def.name)
+        name ∈ (:invokelatest, :unwrap_unionall, :rewrap_unionall) && continue
+        name ∈ (:print, :sprint) && length(mi.specTypes.parameters) - mi.def.nkw > 3 && continue
+        # No one should ever specialize on notify or schedule's `val` argument
+        name === :notify && !is_atrisk_type(mi.specTypes.parameters[2]) &&
+            !any(is_atrisk_type, mi.specTypes.parameters[4:end]) && continue
+        name === :schedule && !any(is_atrisk_type, mi.specTypes.parameters[2:end-1]) && continue
+        # Add more removal-filters here
+
+        # We've decided to keep it
+        push!(out, mi)
+    end
+    return out
+end
+
+using Test
+
+# Invalidating the code that loads packages leads to major slowdowns, especially if it happens repeatedly
+# in a dependent chain of package loads. Ideally, we'd make this code-path "bulletproof".
+for m in methods(Base.require)
+    @test_broken isempty(remove_unlikely_methodinstances(atrisk_triggers(m, first.(miexp))))
+    @test_broken isempty(remove_unlikely_methodinstances(atrisk_triggers(m, first.(mipriv))))
+end
+
+# Test overall number of atrisk MethodInstances and their average number of backedges
+badexp = Set(remove_unlikely_methodinstances(first.(miexp)))
+badcounts = filter(pr->pr.first ∈ badexp, miexp)
+@test length(badcounts) < 1000
+if length(badcounts) < 800
+    @info "There are now only $(length(badcounts)) at-risk specializations of exported methods, consider dropping the threshold"
+end
+meancounts = sum(last.(badcounts))/length(badcounts)
+@test meancounts < 32
+if meancounts < 24
+    @info "The mean number of at-risk backedges is now only $meancounts, consider dropping the threshold"
+end
+
+# Check for inference quality in specific functions.
+# This is valid only for functions that should always return a particular type for any valid call of their methods.
+function function_returns(@nospecialize(f), @nospecialize(typ); allow_missing_for_missing=true, minargs=0)
+    for m in methods(f)
+        sig = Base.unwrap_unionall(m.sig)
+        for rt in Base.return_types(call_type(Base.unwrap_unionall(m.sig))...)
+            rt <: typ && continue
+            if allow_missing_for_missing && any(T->T===Missing, sig.parameters[2:end]) && rt === Missing
+                continue
+            end
+            length(sig.parameters) - 1 < minargs && continue
+            return false
+        end
+    end
+    return true
+end
+
+# All the is* functions
+# Not all of the broken cases have been checked carefully; it's possible some of these should return `Union{Bool,Missing}`
+# or something.
+@test_broken function_returns(isabspath, Bool)
+@test function_returns(isabstracttype, Bool)
+@test_broken function_returns(isapprox, Bool)
+@test_broken function_returns(isascii, Bool)
+# @test function_returns(isassigned, Bool)
+@test function_returns(isbits, Bool)
+@test function_returns(isbitstype, Bool)
+@test function_returns(isblockdev, Bool)
+@test function_returns(ischardev, Bool)
+@test function_returns(iscntrl, Bool)
+@test function_returns(isconcretetype, Bool)
+@test function_returns(isconst, Bool)
+@test function_returns(isdefined, Bool)
+@test function_returns(isdigit, Bool)
+@test function_returns(isdir, Bool)
+@test function_returns(isdirpath, Bool)
+@test_broken function_returns(isdisjoint, Bool)
+@test function_returns(isdispatchtuple, Bool)
+@test_broken function_returns(isempty, Bool)
+@test_broken function_returns(isequal, Bool; minargs=2)
+@test_broken function_returns(iseven, Bool)
+@test function_returns(isexported, Bool)
+@test function_returns(isfifo, Bool)
+@test function_returns(isfile, Bool)
+@test_broken function_returns(isfinite, Bool)
+@test_broken function_returns(isinf, Bool)
+@test_broken function_returns(isinteger, Bool)
+@test function_returns(isinteractive, Bool)
+@test_broken function_returns(isless, Bool)
+@test function_returns(isletter, Bool)
+@test function_returns(islink, Bool)
+@test function_returns(islocked, Bool)
+@test function_returns(islowercase, Bool)
+@test_broken function_returns(ismarked, Bool)
+@test function_returns(ismissing, Bool)
+@test function_returns(ismount, Bool)
+@test function_returns(ismutable, Bool)
+@test_broken function_returns(isnan, Bool)
+@test function_returns(isnothing, Bool)
+@test function_returns(isnumeric, Bool)
+@test_broken function_returns(isodd, Bool)
+@test_broken function_returns(isone, Bool)
+@test_broken function_returns(isopen, Bool)
+@test function_returns(ispath, Bool)
+@test_broken function_returns(isperm, Bool)
+@test function_returns(ispow2, Bool)
+@test function_returns(isprimitivetype, Bool)
+@test function_returns(isprint, Bool)
+@test function_returns(ispunct, Bool)
+@test_broken function_returns(isreadable, Bool)
+@test_broken function_returns(isreadonly, Bool)
+@test_broken function_returns(isready, Bool)
+@test_broken function_returns(isreal, Bool)
+@test function_returns(issetequal, Bool)
+@test function_returns(issetgid, Bool)
+@test function_returns(issetuid, Bool)
+@test function_returns(issocket, Bool)
+@test_broken function_returns(issorted, Bool)
+@test function_returns(isspace, Bool)
+@test function_returns(issticky, Bool)
+@test function_returns(isstructtype, Bool)
+@test_broken function_returns(issubnormal, Bool)
+@test_broken function_returns(issubset, Bool)
+@test function_returns(istaskdone, Bool)
+@test function_returns(istaskfailed, Bool)
+@test function_returns(istaskstarted, Bool)
+@test_broken function_returns(istextmime, Bool)
+@test function_returns(isuppercase, Bool)
+@test_broken function_returns(isvalid, Bool)
+@test_broken function_returns(iswritable, Bool)
+@test function_returns(isxdigit, Bool)
+@test_broken function_returns(iszero, Bool)
+
+@test_broken function_returns(eof, Bool)  # just the Pkg.TOML one is broken
+
+# Check that we never infer certain methodinstances
+for f in (==, isequal, <, <=)
+    for mi in methodinstances(==)
+        if any(T->T<:Real, mi.specTypes.parameters)
+            @test !is_atrisk_type(mi.specTypes)
+        end
+    end
+end

src/findcallers.jl

@@ -1,14 +1,13 @@
 export findcallers
 
-function get_typed_instances!(srcs, mi::MethodInstance, world, interp)
-    # This is essentially take from code_typed_by_type
-    tt = mi.specTypes
+function get_typed_instances!(srcs, @nospecialize(tt), method::Method, world, interp)
+    # This is essentially taken from code_typed_by_type
     matches = Base._methods_by_ftype(tt, -1, world)
     if matches === false
         error("signature $(item.specTypes) does not correspond to a generic function")
     end
     for match in matches
-        match.method == mi.def || continue
+        match.method == method || continue
         meth = Base.func_for_method_checked(match.method, tt, match.sparams)
         (src, ty) = isdefined(Core.Compiler, :NativeInterpreter) ?
             Core.Compiler.typeinf_code(interp, meth, match.spec_types, match.sparams, false) :
@@ -17,6 +16,7 @@ function get_typed_instances!(srcs, mi::MethodInstance, world, interp)
     end
     return srcs
 end
+get_typed_instances!(srcs, mi::MethodInstance, world, interp) = get_typed_instances!(srcs, mi.specTypes, mi.def, world, interp)
 
 defaultinterp(world) = isdefined(Core.Compiler, :NativeInterpreter) ?
                        Core.Compiler.NativeInterpreter(world) :
@@ -25,6 +25,9 @@ defaultinterp(world) = isdefined(Core.Compiler, :NativeInterpreter) ?
 function get_typed_instances(mi::MethodInstance; world=typemax(UInt), interp=defaultinterp(world))
     return get_typed_instances!(Tuple{CodeInfo,Core.SimpleVector}[], mi, world, interp)
 end
+function get_typed_instances(@nospecialize(tt), method::Method; world=typemax(UInt), interp=defaultinterp(world))
+    return get_typed_instances!(Tuple{CodeInfo,Core.SimpleVector}[], tt, method, world, interp)
+end
 
 struct CallMatch
     mi::MethodInstance