Remove convert methods which cause method invalidations #2278

odow · 2020-07-13T01:58:16Z

TODO:

Wait for avoid invalidations when the overlap is ambiguous JuliaLang/julia#35877 to be merged and try again

These methods appear to be used from before we switched to MutableArithmetics.

In the future, instead of overloading Base.convert, we should use a new function
like to_constant(::AffExpr) which avoids the invalidation. Alternatively,
future versions of Julia may be clever enough to avoid the invalidations.

Background

@snoopr docs: https://timholy.github.io/SnoopCompile.jl/stable/snoopr/
Method invalidation blogpost WIP: blog post on invalidations JuliaLang/www.julialang.org#794

`master`

(@v1.6) pkg> st
Status `~/.julia/environments/v1.6/Project.toml`
  [4076af6c] JuMP v0.21.3 `https://github.com/jump-dev/JuMP.jl.git#master`
  [aa65fe97] SnoopCompile v1.7.1

julia> using SnoopCompile

julia> invalidations = @snoopr using JuMP
[ Info: Precompiling JuMP [4076af6c-e467-56ae-b986-b466b2749572]
6630-element Vector{Any}:
  MethodInstance for __init__()
  "insert_backedges"
  MethodInstance for __init__()
  "insert_backedges"
  MethodInstance for __init__()
  "insert_backedges"
  MethodInstance for #readline#308(::Bool, ::typeof(readline), ::IO)
 0
  Tuple{typeof(resize!),Any,Any}
  MethodInstance for _groupedunique!(::AbstractVector{var"#s71"} where var"#s71"<:Real)
 ⋮
 1
  MethodInstance for instantiate(::Base.Broadcast.Broadcasted{Style,Nothing,typeof(Base.wrap_string),Args} where Args<:Tuple where Style<:Union{Nothing, Base.Broadcast.BroadcastStyle})
  "jl_method_table_insert"
  MethodInstance for materialize(::Base.Broadcast.Broadcasted{_A,Nothing,typeof(string),_B} where _B<:Tuple where _A<:Union{Nothing, Base.Broadcast.BroadcastStyle})
 1
  MethodInstance for instantiate(::Base.Broadcast.Broadcasted{_A,Nothing,typeof(string),_B} where _B<:Tuple where _A<:Union{Nothing, Base.Broadcast.BroadcastStyle})
  "jl_method_table_insert"
  instantiate(bc::Base.Broadcast.Broadcasted{var"#s141",Axes,F,Args} where Args<:Tuple where F where Axes where var"#s141"<:JuMP.Containers.BroadcastStyle) in JuMP.Containers at /Users/oscar/.julia/packages/JuMP/bpp7v/src/Containers/SparseAxisArray.jl:175
  "jl_method_table_insert"

julia> length(invalidations)
6630

julia> trees[end]
inserting convert(::Type{T}, quad::GenericQuadExpr{T,VarType} where VarType) where T in JuMP at /Users/oscar/.julia/packages/JuMP/bpp7v/src/quad_expr.jl:328 invalidated:
   mt_backedges:   1: signature Tuple{typeof(convert),Type{Symbol},Any} triggered MethodInstance for Pair{Symbol,Base.IRShow.var"#33#35"}(::Any, ::Any) (0 children)
                   2: signature Tuple{typeof(convert),Type{Base.IRShow.var"#33#35"},Any} triggered MethodInstance for Pair{Symbol,Base.IRShow.var"#33#35"}(::Any, ::Any) (0 children)

# ... lines omitted ...

julia> trees[end].backedges[1]
MethodInstance for convert(::Type{Any}, ::Any) at depth 0 with 1050 children

This PR

(@v1.6) pkg> st
Status `~/.julia/environments/v1.6/Project.toml`
  [4076af6c] JuMP v0.21.3 `~/.julia/dev/JuMP`
  [aa65fe97] SnoopCompile v1.7.1

julia> using SnoopCompile

julia> invalidations = @snoopr using JuMP
[ Info: Precompiling JuMP [4076af6c-e467-56ae-b986-b466b2749572]
1896-element Vector{Any}:
  MethodInstance for __init__()
  "insert_backedges"
  MethodInstance for __init__()
  "insert_backedges"
  MethodInstance for __init__()
  "insert_backedges"
  MethodInstance for #readline#308(::Bool, ::typeof(readline), ::IO)
 0
  Tuple{typeof(resize!),Any,Any}
  MethodInstance for _groupedunique!(::AbstractVector{var"#s71"} where var"#s71"<:Real)
 ⋮
  "jl_method_table_insert"
  MethodInstance for broadcasted(::typeof(string), ::AbstractArray)
 1
  MethodInstance for #unpack#101(::Bool, ::typeof(Pkg.PlatformEngines.unpack), ::String, ::String)
 2
  MethodInstance for broadcasted(::Base.Broadcast.BroadcastStyle, ::typeof(string), ::AbstractArray)
  "jl_method_table_insert"
  broadcasted(::JuMP.Containers.DenseAxisArrayBroadcastStyle, f, args...) in JuMP.Containers at /Users/oscar/.julia/dev/JuMP/src/Containers/DenseAxisArray.jl:246
  "jl_method_table_insert"

julia> length(invalidations)
1896

codecov · 2020-07-13T03:12:42Z

Codecov Report

Merging #2278 (68b25c8) into master (e9b2bf3) will increase coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2278      +/-   ##
==========================================
+ Coverage   91.54%   91.71%   +0.16%     
==========================================
  Files          42       42              
  Lines        4448     4440       -8     
==========================================
  Hits         4072     4072              
+ Misses        376      368       -8

Impacted Files	Coverage Δ
src/aff_expr.jl	`91.15% <ø> (+2.41%)`	⬆️
src/quad_expr.jl	`94.96% <ø> (+2.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9b2bf3...68b25c8. Read the comment docs.

blegat · 2020-07-13T13:19:15Z

IIRC, it was replaced by

JuMP.jl/src/mutable_arithmetics.jl

Lines 80 to 93 in 1129e58

    
           # `SparseArrays/src/linalg.jl` convert numbers to JuMP expressions. MA calls 
        
           # `scaling` to convert them back to numbers. 
        
           function _MA.scaling(aff::GenericAffExpr{C}) where C 
        
               if !isempty(aff.terms) 
        
                   throw(InexactError("Cannot convert `$aff` to `$C`.")) 
        
               end 
        
               return _MA.scaling(aff.constant) 
        
           end 
        
           function _MA.scaling(quad::GenericQuadExpr{C}) where C 
        
               if !isempty(quad.terms) 
        
                   throw(InexactError("Cannot convert `$quad` to `$C`.")) 
        
               end 
        
               return _MA.scaling(quad.aff) 
        
           end

mlubin · 2020-07-13T13:22:29Z

What's the effect on compile time, if any?

odow · 2020-07-13T15:28:25Z

What's the effect on compile time, if any?

Virtually none.

From what I understand from reading the blogpost, the main issue is

A an outer method bar gets compiled for an abstract type (e.g., due to type instability)
Inside that method there is a function foo which also gets compiled for the abstract type
Later, we define a new method of the inner function foo that is more specific than the previously compiled inner function
Doing so forces a recompilation of the outer method bar

So to fix these, we can either remove the methods from JuMP (as above), or fix the type instability in the lower-level package. For example, the collect_to_with_first! method seems to be invalidated due to type instability in Pkg.TOML here: https://github.com/JuliaLang/Pkg.jl/blob/3eee6eb25974c2f320706da1b81ce1f7e1d82165/ext/TOML/src/TOML.jl#L25

The next three worst ones caused by JuMP are:

julia> trees[end - 1]
inserting collect_to_with_first!(dest::Union{JuMP.Containers.DenseAxisArray{T,N,Ax,L} where L<:Tuple{Vararg{Dict,N}} where Ax where N, JuMP.Containers.SparseAxisArray{T,N,K} where K<:Tuple{Vararg{Any,N}} where N}, first_value::T, iterator, state) where T in JuMP.Containers at /Users/oscar/.julia/dev/JuMP/src/Containers/Containers.jl:33 invalidated:
   backedges: 1: superseding collect_to_with_first!(dest::AbstractArray, v1, itr, st) in Base at array.jl:709 with MethodInstance for collect_to_with_first!(::AbstractArray, ::Any, ::Base.Generator{_A,MacroTools.var"#1#2"{Vector{Any}}} where _A, ::Any) (1 children)
              2: superseding collect_to_with_first!(dest, v1, itr, st) in Base at array.jl:715 with MethodInstance for collect_to_with_first!(::Any, ::Any, ::Base.Generator{_A,MacroTools.var"#1#2"{Vector{Any}}} where _A, ::Any) (3 children)
              3: superseding collect_to_with_first!(dest, v1, itr, st) in Base at array.jl:715 with MethodInstance for collect_to_with_first!(::Any, ::Dict{String,Any}, ::Base.Generator{_A,typeof(Pkg.TOML.table2dict)} where _A, ::Any) (126 children)

julia> trees[end - 2]
inserting +(α::T, arg::Union{MathOptInterface.ScalarAffineFunction{T}, MathOptInterface.ScalarQuadraticFunction{T}}, args::Union{MathOptInterface.SingleVariable, MathOptInterface.ScalarAffineFunction{T}, MathOptInterface.ScalarQuadraticFunction{T}}...) where T in MathOptInterface.Utilities at /Users/oscar/.julia/packages/MathOptInterface/bygN7/src/Utilities/functions.jl:1121 invalidated:
   backedges: 1: superseding +(a, b, c, xs...) in Base at operators.jl:538 with MethodInstance for +(::Int64, ::Any, ::Any) (98 children)

julia> trees[end - 6]
inserting eachindex(A::JuMP.Containers.DenseAxisArray) in JuMP.Containers at /Users/oscar/.julia/dev/JuMP/src/Containers/DenseAxisArray.jl:137 invalidated:
   backedges: 1: superseding eachindex(A::AbstractVector{T} where T) in Base at abstractarray.jl:212 with MethodInstance for eachindex(::AbstractVector{var"#s57"} where var"#s57"<:Symbol) (1 children)
              2: superseding eachindex(A::AbstractVector{T} where T) in Base at abstractarray.jl:212 with MethodInstance for eachindex(::AbstractVector{var"#s70"} where var"#s70"<:AbstractString) (1 children)
              3: superseding eachindex(A::AbstractVector{T} where T) in Base at abstractarray.jl:212 with MethodInstance for eachindex(::AbstractVector{var"#s71"} where var"#s71"<:Real) (1 children)
              4: superseding eachindex(A::AbstractVector{T} where T) in Base at abstractarray.jl:212 with MethodInstance for eachindex(::AbstractVector{T} where T) (2 children)
              5: superseding eachindex(A::AbstractVector{T} where T) in Base at abstractarray.jl:212 with MethodInstance for eachindex(::AbstractVector{var"#s843"} where var"#s843"<:Integer) (13 children)

timholy · 2020-07-13T21:03:37Z

Quite impressive, @odow! With convert I suspect avoiding invalidation upon ambiguity will eliminate most of these automatically. Nevertheless you've already developed some impressive analysis skills, and from a brief skim it looks like you're on the trail of some that are unlikely to be fixed even with that change to Julia.

As I explained elsewhere (JuliaLang/www.julialang.org#794 (comment)) fixing invalidations triggered by JuMP itself is mostly you being a good citizen for your users, not for direct usage of JuMP itself. (There are some exceptions, e.g., if you need methods you invalidate.) So you should already see some speedups on Julia 1.6 from changes that prevent your predecessors from triggering invalidations. But don't have exceptional hopes that eliminating invalidations caused by JuMP itself will massively decrease your own latency, unless you're explicitly using methods that you're invalidating.

timholy · 2020-07-13T21:15:33Z

I should mention that the invalidation work has a "secret" subtext: if we eliminate invalidations, then in addition to its direct benefits it will also eliminate one big obstacle to caching native code in *.ji files (JuliaLang/julia#30488). If someone does implement caching native code, then you can expect huge drops in latency for anything that was compile-time bottlenecked and which gets cached. (And you'll be able to control that caching with precompile directives, perhaps with the help of SnoopCompile.)

It will still take someone knowledgeable and enthusiastic to implement that feature, and I'm not sure how Jeff/Jameson feel about it. But until recently the invalidations were so rampant that it was basically pointless to even try. We're almost to a spot where it's no longer pointless.

odow · 2020-07-13T21:25:40Z

But don't have exceptional hopes that eliminating invalidations caused by JuMP itself will massively decrease your own latency, unless you're explicitly using methods that you're invalidating.

Yeah, we seem to be invalidating very generic fallback methods or ones in Pkg.

I'm coming round to the idea that our compilation issue when testing stems from the fact that almost all tests are unique enough that they are all touching different code paths. We test lots of different functions with different typed inputs, rather than testing lots of edge cases of the same input types.

I've also found a few inference failures in the JuMP source and various tests, but fixing them has no noticeable impact on the overall runtime.

you've already developed some impressive analysis skills

Cthulhu and SnoopCompile are incredible...

…ions. These methods appear to be used from before we switched to MutableArithmetics. In the future, instead of overloading Base.convert, we should use a new function like `to_constant(::AffExpr)` which avoids the invalidation. Alternatively, future versions of Julia may be clever enough to avoid the invalidations.

odow · 2020-11-26T01:17:10Z

To update this:

Current master

julia> using SnoopCompile

julia> invalidations = @snoopr using JuMP;
[ Info: Precompiling JuMP [4076af6c-e467-56ae-b986-b466b2749572]

julia> length(invalidations)
5778

This PR

julia> using SnoopCompile

julia> invalidations = @snoopr using JuMP;

julia> length(invalidations)
1650

So better than before, but this PR should be merged.

blegat · 2020-11-26T07:38:54Z

They were replaced by

JuMP.jl/src/mutable_arithmetics.jl

Lines 80 to 93 in e9b2bf3

    
           # `SparseArrays/src/linalg.jl` convert numbers to JuMP expressions. MA calls 
        
           # `scaling` to convert them back to numbers. 
        
           function _MA.scaling(aff::GenericAffExpr{C}) where C 
        
               if !isempty(aff.terms) 
        
                   throw(InexactError("Cannot convert `$aff` to `$C`.")) 
        
               end 
        
               return _MA.scaling(aff.constant) 
        
           end 
        
           function _MA.scaling(quad::GenericQuadExpr{C}) where C 
        
               if !isempty(quad.terms) 
        
                   throw(InexactError("Cannot convert `$quad` to `$C`.")) 
        
               end 
        
               return _MA.scaling(quad.aff) 
        
           end

which explains why removing them does not break any test.

odow changed the title ~~Remove convert methods which method invalidations~~ Remove convert methods which cause method invalidations Jul 13, 2020

odow mentioned this pull request Jul 13, 2020

WIP: blog post on invalidations JuliaLang/www.julialang.org#794

Merged

odow added the JuMP Development Sprint label Jul 13, 2020

odow force-pushed the od/invalidations branch from 575154d to 68b25c8 Compare November 26, 2020 01:15

blegat approved these changes Nov 26, 2020

View reviewed changes

odow merged commit 352f2c1 into master Nov 26, 2020

odow deleted the od/invalidations branch November 26, 2020 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove convert methods which cause method invalidations #2278

Remove convert methods which cause method invalidations #2278

odow commented Jul 13, 2020 •

edited

Loading

codecov bot commented Jul 13, 2020 •

edited

Loading

blegat commented Jul 13, 2020

mlubin commented Jul 13, 2020

odow commented Jul 13, 2020

timholy commented Jul 13, 2020

timholy commented Jul 13, 2020

odow commented Jul 13, 2020

odow commented Nov 26, 2020

blegat commented Nov 26, 2020

Remove convert methods which cause method invalidations #2278

Remove convert methods which cause method invalidations #2278

Conversation

odow commented Jul 13, 2020 • edited Loading

Background

master

This PR

codecov bot commented Jul 13, 2020 • edited Loading

Codecov Report

blegat commented Jul 13, 2020

mlubin commented Jul 13, 2020

odow commented Jul 13, 2020

timholy commented Jul 13, 2020

timholy commented Jul 13, 2020

odow commented Jul 13, 2020

odow commented Nov 26, 2020

blegat commented Nov 26, 2020

odow commented Jul 13, 2020 •

edited

Loading

`master`

codecov bot commented Jul 13, 2020 •

edited

Loading