Avoid invalidations related to * string concat #2325

IanButterworth · 2021-01-11T00:19:23Z

I noticed that loading Flux was causing Pkg latency, which on investigation seems to be due to ChainRulesCore's necessarily broad * methods (JuliaDiff/ChainRulesCore.jl#273) and the use of * for string concat in Pkg

Before:

julia> using SnoopCompileCore

julia> invalidations = @snoopr using ChainRulesCore;

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations)
4-element Vector{SnoopCompile.MethodInvalidations}:
 inserting convert(::Type{var"#s4"} where var"#s4"<:Tuple, comp::Composite{var"#s3", var"#s2"} where var"#s2"<:Tuple where var"#s3") in ChainRulesCore at /Users/ian/.julia/packages/ChainRulesCore/cpHLu/src/differentials/composite.jl:68 invalidated:
   mt_backedges: 1: signature Tuple{typeof(convert), Type{Tuple{DataType, DataType, DataType}}, Any} triggered MethodInstance for Pair{DataType, Tuple{DataType, DataType, DataType}}(::Any, ::Any) (0 children)
                 2: signature Tuple{typeof(convert), Type{NTuple{8, DataType}}, Any} triggered MethodInstance for Pair{DataType, NTuple{8, DataType}}(::Any, ::Any) (0 children)
                 3: signature Tuple{typeof(convert), Type{NTuple{7, DataType}}, Any} triggered MethodInstance for Pair{DataType, NTuple{7, DataType}}(::Any, ::Any) (0 children)
                 4: signature Tuple{typeof(convert), Type{Tuple{Symbol, Any, Symbol, Symbol}}, Any} triggered MethodInstance for setindex!(::Vector{Tuple{Symbol, Any, Symbol, Symbol}}, ::Any, ::Int64) (0 children)
                 5: signature Tuple{typeof(convert), Type{Tuple{Base.UUID, String}}, Any} triggered MethodInstance for setindex!(::Vector{Tuple{Base.UUID, String}}, ::Any, ::Int64) (0 children)
                 6: signature Tuple{typeof(convert), Type{Tuple{Any, String}}, Any} triggered MethodInstance for setindex!(::Vector{Tuple{Any, String}}, ::Any, ::Int64) (0 children)

 inserting *(s, comp::Composite) in ChainRulesCore at /Users/ian/.julia/packages/ChainRulesCore/cpHLu/src/differential_arithmetic.jl:138 invalidated:
   mt_backedges: 1: signature Tuple{typeof(*), Union{Regex, String}, Any} triggered MethodInstance for *(::Any, ::Char, ::Any) (0 children)

 inserting convert(::Type{var"#s4"} where var"#s4"<:Dict, comp::Composite{var"#s3", var"#s2"} where var"#s2"<:Dict where var"#s3"<:Dict) in ChainRulesCore at /Users/ian/.julia/packages/ChainRulesCore/cpHLu/src/differentials/composite.jl:69 invalidated:
   mt_backedges: 1: signature Tuple{typeof(convert), Type{Dict{String, Any}}, Any} triggered MethodInstance for setindex!(::Dict{Base.BinaryPlatforms.AbstractPlatform, Dict{String, Any}}, ::Any, ::Base.BinaryPlatforms.Platform) (0 children)
                 2: signature Tuple{typeof(convert), Type{Dict{Symbol, Any}}, Any} triggered MethodInstance for setindex!(::IdDict{Function, Dict{Symbol, Any}}, ::Any, ::Any) (8 children)
                 3: signature Tuple{typeof(convert), Type{Dict{Char, Any}}, Any} triggered MethodInstance for REPL.LineEdit.Prompt(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any) (18 children)

 inserting *(::Zero, ::Any) in ChainRulesCore at /Users/ian/.julia/packages/ChainRulesCore/cpHLu/src/differential_arithmetic.jl:68 invalidated:
   mt_backedges: 1: signature Tuple{typeof(*), Any, String} triggered MethodInstance for Pkg.REPLMode.promptf() (0 children)
                 2: signature Tuple{typeof(*), Any, String} triggered MethodInstance for Pkg.API.var"#precompile#195"(::Bool, ::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, ::typeof(Pkg.API.precompile), ::Pkg.Types.Context) (15 children)
                 3: signature Tuple{typeof(*), Any, Char} triggered MethodInstance for *(::Any, ::Char, ::Any) (16 children)
   10 mt_cache

The Pkg.API.var"#precompile#195" invalidation seems most substantial

julia> show(last(root); minchildren=0, maxdepth=10)
MethodInstance for Pkg.API.var"#precompile#197"(::Bool, ::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, ::typeof(Pkg.API.precompile), ::Pkg.Types.Context) (15 children)
 MethodInstance for (::Pkg.API.var"#precompile##kw")(::NamedTuple{(:internal_call,), Tuple{Bool}}, ::typeof(Pkg.API.precompile), ::Pkg.Types.Context) (14 children)
  MethodInstance for Pkg.API._auto_precompile(::Pkg.Types.Context) (13 children)
   MethodInstance for Pkg.API.var"#instantiate#249"(::Nothing, ::Bool, ::Bool, ::Base.BinaryPlatforms.Platform, ::Bool, ::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, ::typeof(Pkg.API.instantiate), ::Pkg.Types.Context) (4 children)
    MethodInstance for (::Pkg.API.var"#instantiate##kw")(::NamedTuple{(:manifest, :update_registry, :verbose), Tuple{Nothing, Bool, Bool}}, ::typeof(Pkg.API.instantiate), ::Pkg.Types.Context) (1 children)
     MethodInstance for Pkg.API.var"#instantiate#249"(::Nothing, ::Bool, ::Bool, ::Base.BinaryPlatforms.Platform, ::Bool, ::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, ::typeof(Pkg.API.instantiate), ::Pkg.Types.Context) (0 children)
    MethodInstance for (::Pkg.API.var"#instantiate##kw")(::NamedTuple{(:allow_autoprecomp,), Tuple{Bool}}, ::typeof(Pkg.API.instantiate), ::Pkg.Types.Context) (1 children)
     MethodInstance for Pkg.API.var"#precompile#197"(::Bool, ::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, ::typeof(Pkg.API.precompile), ::Pkg.Types.Context) (0 children)
   MethodInstance for Pkg.API.var"#add#24"(::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, ::typeof(Pkg.API.add), ::Vector{Pkg.Types.PackageSpec}) (7 children)
    MethodInstance for Pkg.API.add(::Vector{Pkg.Types.PackageSpec}) (6 children)
     MethodInstance for Pkg.API.var"#add#22"(::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, ::typeof(Pkg.API.add), ::Pkg.Types.PackageSpec) (1 children)
      MethodInstance for Pkg.API.add(::Pkg.Types.PackageSpec) (0 children)
     MethodInstance for Pkg.API.var"#add#23"(::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, ::typeof(Pkg.API.add), ::Vector{String}) (3 children)
      MethodInstance for Pkg.API.add(::Vector{String}) (2 children)
       MethodInstance for Pkg.API.var"#add#22"(::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, ::typeof(Pkg.API.add), ::String) (1 children)
        MethodInstance for Pkg.API.add(::String) (0 children)

This PR eliminates the Pkg.REPLMode.promptf() and Pkg.API.var"#precompile#195" invalidations

4-element Vector{SnoopCompile.MethodInvalidations}:
 inserting convert(::Type{var"#s4"} where var"#s4"<:Tuple, comp::Composite{var"#s3", var"#s2"} where var"#s2"<:Tuple where var"#s3") in ChainRulesCore at /Users/ian/.julia/dev/ChainRulesCore/src/differentials/composite.jl:68 invalidated:
   mt_backedges: 1: signature Tuple{typeof(convert), Type{Tuple{DataType, DataType, DataType}}, Any} triggered MethodInstance for Pair{DataType, Tuple{DataType, DataType, DataType}}(::Any, ::Any) (0 children)
                 2: signature Tuple{typeof(convert), Type{NTuple{8, DataType}}, Any} triggered MethodInstance for Pair{DataType, NTuple{8, DataType}}(::Any, ::Any) (0 children)
                 3: signature Tuple{typeof(convert), Type{NTuple{7, DataType}}, Any} triggered MethodInstance for Pair{DataType, NTuple{7, DataType}}(::Any, ::Any) (0 children)
                 4: signature Tuple{typeof(convert), Type{Tuple{Symbol, Any, Symbol, Symbol}}, Any} triggered MethodInstance for setindex!(::Vector{Tuple{Symbol, Any, Symbol, Symbol}}, ::Any, ::Int64) (0 children)
                 5: signature Tuple{typeof(convert), Type{Tuple{Any, String}}, Any} triggered MethodInstance for setindex!(::Vector{Tuple{Any, String}}, ::Any, ::Int64) (0 children)

 inserting *(s, comp::Composite) in ChainRulesCore at /Users/ian/.julia/dev/ChainRulesCore/src/differential_arithmetic.jl:138 invalidated:
   mt_backedges: 1: signature Tuple{typeof(*), Union{Regex, String}, Any} triggered MethodInstance for *(::Any, ::Char, ::Any) (0 children)
                 2: signature Tuple{typeof(*), String, Any} triggered MethodInstance for Logging.default_metafmt(::Base.CoreLogging.LogLevel, ::Any, ::Any, ::Any, ::Any, ::Any) (0 children)

 inserting *(::DoesNotExist, ::Any) in ChainRulesCore at /Users/ian/.julia/dev/ChainRulesCore/src/differential_arithmetic.jl:28 invalidated:
   mt_backedges: 1: signature Tuple{typeof(*), Any, Char} triggered MethodInstance for *(::Any, ::Char, ::Any) (16 children)
   8 mt_cache

 inserting convert(::Type{var"#s4"} where var"#s4"<:Dict, comp::Composite{var"#s3", var"#s2"} where var"#s2"<:Dict where var"#s3"<:Dict) in ChainRulesCore at /Users/ian/.julia/dev/ChainRulesCore/src/differentials/composite.jl:69 invalidated:
   mt_backedges: 1: signature Tuple{typeof(convert), Type{Dict{String, Any}}, Any} triggered MethodInstance for setindex!(::Dict{Base.BinaryPlatforms.AbstractPlatform, Dict{String, Any}}, ::Any, ::Base.BinaryPlatforms.Platform) (0 children)
                 2: signature Tuple{typeof(convert), Type{Dict{Symbol, Any}}, Any} triggered MethodInstance for setindex!(::IdDict{Function, Dict{Symbol, Any}}, ::Any, ::Any) (8 children)
                 3: signature Tuple{typeof(convert), Type{Dict{Char, Any}}, Any} triggered MethodInstance for REPL.LineEdit.Prompt(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any) (18 children)

timholy

LGTM. I don't know the Pkg internals super-well, so take that FWIW, but overall this looks like a nice enhacement.

timholy · 2021-01-11T21:11:38Z

src/REPLMode/REPLMode.jl

@@ -509,7 +509,7 @@ function promptf()
    prefix = ""
    if project_file !== nothing
        if prev_project_file == project_file && prev_project_timestamp == mtime(project_file)
-            prefix = prev_prefix
+            prefix = prev_prefix::String
        else
            project_name = projname(project_file)
            if project_name !== nothing


Is project_name guaranteed to be a String when it's not nothing? Just wondering about the safety of the typeassert (presumably, you're fine).

From what I can tell it should be.. either way, wrt. what gets applied to prev_prefix the product of string() is always String, I believe??

To play it safe I've reverted the type assert

KristofferC · 2021-01-12T07:21:31Z

src/API.jl

@@ -1008,7 +1009,7 @@ function precompile(ctx::Context; internal_call::Bool=false, kwargs...)
    end
    depsmap = Dict{Base.PkgId, Vector{Base.PkgId}}(Iterators.filter(!isnothing, deps_pair_or_nothing)) #flat map of each dep and its deps

-    if ctx.env.pkg !== nothing && isfile( joinpath( dirname(ctx.env.project_file), "src", ctx.env.pkg.name * ".jl") )
+    if ctx.env.pkg !== nothing && isfile( joinpath( dirname(ctx.env.project_file), "src", string(ctx.env.pkg.name, ".jl")) )


I think the standard way of doing this is:

ctx_env_pkg = ctx.env.pkg if ctx_env_pkg !== nothing && isfile( joinpath( dirname(ctx.env.project_file), "src", ctx_env_pkg.name * ".jl") )

Basically, hoist the load of the thing we check for nothing so that Julia realizes it cannot have changed the next time we use it. Changing * to string is mostly a bandaid.

KristofferC · 2021-01-12T07:23:18Z

If things are well inferred it shouldn't matter if you use * or string, no? So the focus should probably be to change the code to be well inferred, not just change * -> string which will run into the same problems if someone creates similar string methods as was the problem for *

IanButterworth · 2021-01-12T07:27:26Z

Agreed. I couldn't figure out where the inference issues where, but your pointers help. I'll take a stab at fixing them. Thanks

But I think I'll leave the sprint() changes as they seem more efficient?

KristofferC · 2021-01-12T07:44:54Z

But I think I'll leave the sprint() changes as they seem more efficient?

👍

IanButterworth · 2021-01-12T19:10:26Z

Validated to still eliminate

inserting *(::Zero, ::Any) in ChainRulesCore at /Users/ian/.julia/packages/ChainRulesCore/cpHLu/src/differential_arithmetic.jl:68 invalidated:
   mt_backedges: 1: signature Tuple{typeof(*), Any, String} triggered MethodInstance for Pkg.REPLMode.promptf() (0 children)
                 2: signature Tuple{typeof(*), Any, String} triggered MethodInstance for Pkg.API.var"#precompile#195"(::Bool, ::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}, ::typeof(Pkg.API.precompile), ::Pkg.Types.Context) (15 children)

timholy · 2021-01-12T19:34:46Z

If you provide your versions of Julia & ChainRulesCore I can take a peek. On 1.6 and ChainRulesCore v0.9.25, I'm seeing invalidation of the bodyfunction for API.precompile, mostly likely due to several Core.Box variables (the infamous #15276). See https://timholy.github.io/SnoopCompile.jl/dev/snoopr/#Fixing-Core.Box.

IanButterworth · 2021-01-12T19:37:55Z

@timholy Sorry, I meant that this PR still fixes (removes) the invalidations I listed, after the review edit commit. Bad choice of wording

timholy · 2021-01-12T23:16:52Z

src/API.jl

@@ -114,13 +114,15 @@ end

 function check_package_name(x::AbstractString, mode=nothing)


I'm still a bit confused about whether this PR does everything you want it to. If not, one "cheap" way to get better inference is to make the signature check_package_name(x::String, mode::Union{Nothing,SomeType}=nothing) and then also provide an argument-standardizing method

check_package_name(x::AbstractString, mode=nothing) = check_package_name(String(x)::String, convert(Union{Nothing,SomeType}, mode)::Union{Nothing,SomeType})

The typeasserts guarantee you'll never get a StackOverflowError.

simeonschaub · 2021-02-03T14:56:28Z

Ah, sorry, I just saw this. This seems to fix the same invalidations as #2376. While that one is more minimal, we might still want to go with this one instead because of the style and performance improvements.

timholy · 2021-02-06T12:14:42Z

If @IanButterworth thinks this is good to go, I'm fine with it. I was just confused about what state this was in. From #2376 it looks like there may be Core.Boxes? I generally try to fix those at the source rather than putting bandaids on things.

IanButterworth · 2021-02-13T04:06:05Z

Sorry @timholy, my understanding of this was a bit fragile and only got back around to it now.

I've narrowed the def of check_package_name re-checked, and it fixes the Pkg invalidations.

REPL.LineEdit.Prompt caught my eye, but seems ok because it's not re-used after the repl is started

julia> using SnoopCompileCore

julia> invalidations = @snoopr using ChainRulesCore;

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations)
4-element Vector{SnoopCompile.MethodInvalidations}:
 inserting convert(::Type{var"#s4"} where var"#s4"<:Tuple, comp::Composite{var"#s3", var"#s2"} where {var"#s3", var"#s2"<:Tuple}) in ChainRulesCore at /Users/ian/.julia/packages/ChainRulesCore/7d1hl/src/differentials/composite.jl:68 invalidated:
   mt_backedges: 1: signature Tuple{typeof(convert), Type{Tuple{DataType, DataType, DataType}}, Any} triggered MethodInstance for Pair{DataType, Tuple{DataType, DataType, DataType}}(::Any, ::Any) (0 children)
                 2: signature Tuple{typeof(convert), Type{NTuple{8, DataType}}, Any} triggered MethodInstance for Pair{DataType, NTuple{8, DataType}}(::Any, ::Any) (0 children)
                 3: signature Tuple{typeof(convert), Type{NTuple{7, DataType}}, Any} triggered MethodInstance for Pair{DataType, NTuple{7, DataType}}(::Any, ::Any) (0 children)
                 4: signature Tuple{typeof(convert), Type{Tuple{Symbol, Any, Symbol, Symbol}}, Any} triggered MethodInstance for setindex!(::Vector{Tuple{Symbol, Any, Symbol, Symbol}}, ::Any, ::Int64) (0 children)
                 5: signature Tuple{typeof(convert), Type{Tuple{Any, String}}, Any} triggered MethodInstance for setindex!(::Vector{Tuple{Any, String}}, ::Any, ::Int64) (0 children)

 inserting *(s, comp::Composite) in ChainRulesCore at /Users/ian/.julia/packages/ChainRulesCore/7d1hl/src/differential_arithmetic.jl:138 invalidated:
   mt_backedges: 1: signature Tuple{typeof(*), Union{Regex, String}, Any} triggered MethodInstance for *(::Any, ::Char, ::Any) (0 children)

 inserting *(::DoesNotExist, ::Any) in ChainRulesCore at /Users/ian/.julia/packages/ChainRulesCore/7d1hl/src/differential_arithmetic.jl:28 invalidated:
   mt_backedges: 1: signature Tuple{typeof(*), Any, Char} triggered MethodInstance for *(::Any, ::Char, ::Any) (16 children)
   9 mt_cache

 inserting convert(::Type{var"#s4"} where var"#s4"<:Dict, comp::Composite{var"#s3", var"#s2"} where {var"#s3"<:Dict, var"#s2"<:Dict}) in ChainRulesCore at /Users/ian/.julia/packages/ChainRulesCore/7d1hl/src/differentials/composite.jl:69 invalidated:
   mt_backedges: 1: signature Tuple{typeof(convert), Type{Dict{String, Any}}, Any} triggered MethodInstance for setindex!(::Dict{Base.BinaryPlatforms.AbstractPlatform, Dict{String, Any}}, ::Any, ::Base.BinaryPlatforms.Platform) (0 children)
                 2: signature Tuple{typeof(convert), Type{Dict{Symbol, Any}}, Any} triggered MethodInstance for setindex!(::IdDict{Function, Dict{Symbol, Any}}, ::Any, ::Any) (8 children)
                 3: signature Tuple{typeof(convert), Type{Dict{Char, Any}}, Any} triggered MethodInstance for REPL.LineEdit.Prompt(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any) (18 children)

I'll merge once tests pass

…try is functional

* Avoid invalidations related to * string concat * use same JULIA_PKG_SERVER setting for processcoverage to ensure registry is functional (cherry picked from commit 42fc1bf)

IanButterworth mentioned this pull request Jan 11, 2021

Invalidation squashing attempts JuliaDiff/ChainRulesCore.jl#273

Closed

timholy approved these changes Jan 11, 2021

View reviewed changes

KristofferC reviewed Jan 12, 2021

View reviewed changes

timholy reviewed Jan 12, 2021

View reviewed changes

simeonschaub mentioned this pull request Feb 3, 2021

fix some invalidations with ChainRulesCore #2376

Closed

Avoid invalidations related to * string concat

b6e5690

IanButterworth force-pushed the ib/inv branch from 5d4da26 to b6e5690 Compare February 13, 2021 03:48

use same JULIA_PKG_SERVER setting for processcoverage to ensure regis…

e6f7418

…try is functional

IanButterworth requested a review from a team as a code owner February 13, 2021 04:35

IanButterworth merged commit 42fc1bf into JuliaLang:master Feb 13, 2021

IanButterworth deleted the ib/inv branch February 13, 2021 04:37

IanButterworth added the backport pending 1.6 label Feb 13, 2021

IanButterworth mentioned this pull request Mar 26, 2021

1.6.1 backports #2425

Merged

14 tasks

fredrikekre removed the backport pending 1.6 label Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid invalidations related to * string concat #2325

Avoid invalidations related to * string concat #2325

IanButterworth commented Jan 11, 2021

timholy left a comment

timholy Jan 11, 2021

IanButterworth Jan 12, 2021 •

edited

Loading

IanButterworth Jan 12, 2021

KristofferC Jan 12, 2021

KristofferC commented Jan 12, 2021

IanButterworth commented Jan 12, 2021

KristofferC commented Jan 12, 2021

IanButterworth commented Jan 12, 2021

timholy commented Jan 12, 2021

IanButterworth commented Jan 12, 2021 •

edited

Loading

timholy Jan 12, 2021

simeonschaub commented Feb 3, 2021

timholy commented Feb 6, 2021

IanButterworth commented Feb 13, 2021 •

edited

Loading

		@@ -114,13 +114,15 @@ end

		function check_package_name(x::AbstractString, mode=nothing)

Avoid invalidations related to * string concat #2325

Avoid invalidations related to * string concat #2325

Conversation

IanButterworth commented Jan 11, 2021

timholy left a comment

Choose a reason for hiding this comment

timholy Jan 11, 2021

Choose a reason for hiding this comment

IanButterworth Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

IanButterworth Jan 12, 2021

Choose a reason for hiding this comment

KristofferC Jan 12, 2021

Choose a reason for hiding this comment

KristofferC commented Jan 12, 2021

IanButterworth commented Jan 12, 2021

KristofferC commented Jan 12, 2021

IanButterworth commented Jan 12, 2021

timholy commented Jan 12, 2021

IanButterworth commented Jan 12, 2021 • edited Loading

timholy Jan 12, 2021

Choose a reason for hiding this comment

simeonschaub commented Feb 3, 2021

timholy commented Feb 6, 2021

IanButterworth commented Feb 13, 2021 • edited Loading

IanButterworth Jan 12, 2021 •

edited

Loading

IanButterworth commented Jan 12, 2021 •

edited

Loading

IanButterworth commented Feb 13, 2021 •

edited

Loading