fix `apply_type_tfunc` accuracy #47136

ranocha · 2022-10-12T06:35:23Z

As discussed in #47089

Closes #47089

CC @aviatesk

aviatesk

Please tweak indentations so that they are aligned with the other parts.

aviatesk · 2022-10-12T06:36:44Z

@nanosoldier runbenchmarks("inference", vs=":master")

ranocha · 2022-10-12T06:42:26Z

Please tweak indentations so that they are aligned with the other parts.

Done (and rebased on master)

nanosoldier · 2022-10-12T07:31:10Z

Your benchmark job has completed - no performance regressions were detected. A full report can be found here.

ranocha · 2022-10-12T08:14:38Z

julia> code_warntype(keys, (Dict{String},))
MethodInstance for keys(::Dict{String})
  from keys(a::AbstractDict) @ Base abstractdict.jl:105
Arguments
  #self#::Core.Const(keys)
  a::Dict{String}
Body::Base.KeySet{String, T} where T<:(Dict{String})
1 ─ %1 = Base.KeySet(a)::Base.KeySet{String, T} where T<:(Dict{String})
└──      return %1

That's definitely better than

julia> code_warntype(keys, (Dict{String},))
MethodInstance for keys(::Dict{String})
  from keys(a::AbstractDict) in Base at abstractdict.jl:105
Arguments
  #self#::Core.Const(keys)
  a::Dict{String}
Body::Base.KeySet{String, _A} where _A<:(AbstractDict{String})
1 ─ %1 = Base.KeySet(a)::Base.KeySet{String, _A} where _A<:(AbstractDict{String})
└──      return %1

in Julia v1.8.2 🥳
Xref #47089

test/compiler/inference.jl

aviatesk · 2022-10-12T09:03:01Z

Well, how does this change prevent the invalidation? collect(::Base.KeySet{String, T} where T<:(Dict{String})) is robust enough against the invalidation?

ranocha · 2022-10-12T09:28:29Z

This PR:

julia> import Pkg; Pkg.activate(temp=true); Pkg.add("DataStructures")

julia> using SnoopCompileCore; invalidations = @snoopr(using DataStructures); using SnoopCompile

julia> length(uinvalidated(invalidations))
174

julia> trees = invalidation_trees(invalidations)
...
 inserting iterate(v::Union{Base.KeySet{<:Any, <:SwissDict}, Base.ValueIterator{<:SwissDict}}, state) @ DataStructures ~/.julia/packages/DataStructures/59MD0/src/swiss_dict.jl:646 invalidated:
   backedges: 1: superseding iterate(v::T, i::Int64) where T<:Union{KeySet{<:Any, <:Dict}, ValueIterator{<:Dict}} @ Base dict.jl:698 with MethodInstance for iterate(::Base.KeySet{K, T} where {K, T<:(Dict{K})}, ::Int64) (13 children)
              2: superseding iterate(v::T, i::Int64) where T<:Union{KeySet{<:Any, <:Dict}, ValueIterator{<:Dict}} @ Base dict.jl:698 with MethodInstance for iterate(::Base.KeySet{String, T} where T<:(Dict{String}), ::Int64) (40 children)
   14 mt_cache
...

Compared to

julia> length(uinvalidated(invalidations))
482

julia> trees = invalidation_trees(invalidations)
...
 inserting iterate(v::Union{Base.KeySet{<:Any, <:SwissDict}, Base.ValueIterator{<:SwissDict}}, state) in DataStructures at ~/.julia/packages/DataStructures/59MD0/src/swiss_dict.jl:646 invalidated:
   backedges: 1: superseding iterate(v::Union{Base.KeySet, Base.ValueIterator}, state...) in Base at abstractdict.jl:63 with MethodInstance for iterate(::Base.KeySet, ::Any) (1 children)
              2: superseding iterate(v::Union{Base.KeySet, Base.ValueIterator}, state...) in Base at abstractdict.jl:63 with MethodInstance for iterate(::Base.KeySet{String, _A} where _A<:(AbstractDict{String}), ::Any) (8 children)
              3: superseding iterate(v::T, i::Int64) where T<:Union{Base.KeySet{<:Any, <:Dict}, Base.ValueIterator{<:Dict}} in Base at dict.jl:712 with MethodInstance for iterate(::Base.KeySet{var"#s82", T} where {var"#s82", T<:(Dict{var"#s82"})}, ::Int64) (44 children)
              4: superseding iterate(v::T, i::Int64) where T<:Union{Base.KeySet{<:Any, <:Dict}, Base.ValueIterator{<:Dict}} in Base at dict.jl:712 with MethodInstance for iterate(::Base.KeySet{String, _A} where _A<:(Dict{String}), ::Int64) (317 children)

on Julia v1.8.2. So there are still some invalidations left but a big chunk is gone. We cannot test invalidations directly but should probably do so when/if #47093 is merged.

aviatesk · 2022-10-18T04:56:21Z

While our benchmark result seems good, I'd like to confirm this change doesn't come with noticeable regression in compiling a large package like Plots.jl or OrdinaryDiffEq.jl. We currently don't have a proper benchmark setup for that, so it needs some manual inspection.

ranocha · 2022-10-18T05:36:46Z

OrdinaryDiffEq.jl cannot be precompiled with the current nightly build since LinearSolve.jl fails to precompile there 🤷

ERROR: LoadError: MethodError: no method matching SparseArrays.UMFPACK.UmfpackLU(::Ptr{Nothing}, ::Ptr{Nothing}, ::Int64, ::Int64, ::Vector{Int64}, ::Vector{Int64}, ::Vector{Float64}, ::Int64)

Closest candidates are:
  SparseArrays.UMFPACK.UmfpackLU(::SparseArrays.UMFPACK.Symbolic{Tv, Ti}, ::SparseArrays.UMFPACK.Numeric{Tv, Ti}, ::Int64, ::Int64, ::Vector{Ti}, ::Vector{Ti}, ::Vector{Tv}, ::Int64, ::SparseArrays.UMFPACK.UmfpackWS{Ti}, ::Vector{Float64}, ::Vector{Float64}, ::ReentrantLock) where {Tv<:Union{Float64, ComplexF64}, Ti<:Union{Int32, Int64}}
   @ SparseArrays .../julia-dc3a2e8d27/share/julia/stdlib/v1.9/SparseArrays/src/solvers/umfpack.jl:224

Stacktrace:
  [1] init_cacheval(alg::LinearSolve.UMFPACKFactorization, A::SparseArrays.SparseMatrixCSC{Float64, Int64}, b::Vector{Float64}, u::Vector{Float64}, Pl::IterativeSolvers.Identity, Pr::IterativeSolvers.Identity, maxiters::Int64, abstol::Float64, reltol::Float64, verbose::Bool, assumptions::LinearSolve.OperatorAssumptions{Nothing})
    @ LinearSolve ~/.julia/packages/LinearSolve/POAsl/src/factorization.jl:278
  [2] init(::SciMLBase.LinearProblem{Nothing, true, SparseArrays.SparseMatrixCSC{Float64, Int64}, Vector{Float64}, SciMLBase.NullParameters, Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}}, ::LinearSolve.UMFPACKFactorization; alias_A::Bool, alias_b::Bool, abstol::Float64, reltol::Float64, maxiters::Int64, verbose::Bool, Pl::IterativeSolvers.Identity, Pr::IterativeSolvers.Identity, assumptions::LinearSolve.OperatorAssumptions{Nothing}, kwargs::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ LinearSolve ~/.julia/packages/LinearSolve/POAsl/src/common.jl:111
  [3] init(::SciMLBase.LinearProblem{Nothing, true, SparseArrays.SparseMatrixCSC{Float64, Int64}, Vector{Float64}, SciMLBase.NullParameters, Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}}, ::LinearSolve.UMFPACKFactorization)
    @ LinearSolve ~/.julia/packages/LinearSolve/POAsl/src/common.jl:91
  [4] solve(::SciMLBase.LinearProblem{Nothing, true, SparseArrays.SparseMatrixCSC{Float64, Int64}, Vector{Float64}, SciMLBase.NullParameters, Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}}, ::LinearSolve.UMFPACKFactorization; kwargs::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ LinearSolve ~/.julia/packages/LinearSolve/POAsl/src/common.jl:154
  [5] solve(::SciMLBase.LinearProblem{Nothing, true, SparseArrays.SparseMatrixCSC{Float64, Int64}, Vector{Float64}, SciMLBase.NullParameters, Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}}}, ::LinearSolve.UMFPACKFactorization)
    @ LinearSolve ~/.julia/packages/LinearSolve/POAsl/src/common.jl:151
  [6] macro expansion
    @ ~/.julia/packages/LinearSolve/POAsl/src/LinearSolve.jl:67 [inlined]
  [7] top-level scope
    @ ~/.julia/packages/SnoopPrecompile/UWvXF/src/SnoopPrecompile.jl:51
  [8] include
    @ ./Base.jl:450 [inlined]
  [9] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt64}}, source::String)
    @ Base ./loading.jl:1649
 [10] top-level scope
    @ stdin:1

ranocha · 2022-10-18T05:58:56Z

Nevertheless, I ran the following code to benchmark precompilation time of some packages I use frequently:

julia-nightly -e 'rm(joinpath(homedir(), ".julia", "compiled", "v1.9"), force=true, recursive=true); import Pkg; Pkg.activate(temp=true); @time Pkg.add(["Plots", "OrdinaryDiffEq", "Trixi", "BSeries", "Symbolics", "SymPy", "PyPlot"])'

Nightly build of Julia (Commit dc3a2e8):

Precompiling project...
  ✗ LinearSolve
  ✗ OrdinaryDiffEq
  309 dependencies successfully precompiled in 161 seconds
  2 dependencies errored. To see a full report either run `import Pkg; Pkg.precompile()` or load the packages

This PR:

Precompiling project...
  ✗ LinearSolve
  ✗ OrdinaryDiffEq
  309 dependencies successfully precompiled in 165 seconds
  2 dependencies errored. To see a full report either run `import Pkg; Pkg.precompile()` or load the packages

So this does not look like a serious regression to me. Is this what you had in mind?

aviatesk · 2022-10-18T08:10:44Z

So this does not look like a serious regression to me. Is this what you had in mind?

Yeah, that check seems valid, thanks so much!

aviatesk · 2022-10-18T10:04:01Z

Added more unit tests. Going to merge once confirming that CI runs successfully.

aviatesk · 2022-10-18T12:22:06Z

@nanosoldier runbenchmarks("inference", vs=":master")

oscardssmith · 2022-10-18T13:05:02Z

Note that you can get OrdinaryDiffEq to run by pining version 1.26.0 of LinearSolve.

nanosoldier · 2022-10-18T13:23:10Z

Your benchmark job has completed - no performance regressions were detected. A full report can be found here.

vtjnash · 2022-10-18T19:57:58Z

Going to merge once confirming that CI runs successfully.

CI has never passed on this branch, apparently due to the added time required, so let's not be too hasty to remove the comment about this being disabled because it slows down inference too much

gbaraldi · 2022-10-18T20:03:51Z

It does seem that it's a hang/deadlock more than a slowdown


2022-10-18 07:42:16 BST | Pkg                                               (6) \|   404.31 \|  10.74 \|  2.7 \|   20931.10 \|  3440.63
-- | --
  | 2022-10-18 07:42:16 BST | LinearAlgebra/addmul                             (10) \|  1816.79 \|  45.92 \|  2.5 \|   76463.08 \|  2120.36
  | 2022-10-18 07:56:00 BST |  
  | 2022-10-18 08:52:34 BST |  
  | 2022-10-18 08:52:34 BST | Process failed to exit within 5400s, requesting termination (SIGTERM) of PID 211.

Co-authored-by: Shuhei Kadowaki <[email protected]>

aviatesk · 2023-01-18T08:30:59Z

There seems to be some inference convergence/performance issues to due to this. I would like to keep working on it on my branch, so let me supersede this by #48329.

ranocha added the compiler:inference Type inference label Oct 12, 2022

aviatesk requested changes Oct 12, 2022

View reviewed changes

aviatesk added the needs tests Unit tests are required for this change label Oct 12, 2022

ranocha force-pushed the hr/tfuncs branch from bc13c15 to 4353b42 Compare October 12, 2022 06:41

aviatesk reviewed Oct 12, 2022

View reviewed changes

test/compiler/inference.jl Outdated Show resolved Hide resolved

aviatesk approved these changes Oct 18, 2022

View reviewed changes

aviatesk closed this Oct 18, 2022

aviatesk reopened this Oct 18, 2022

aviatesk changed the title ~~enable commented out code in tfuncs.jl~~ fix apply_type_tfunc accuracy Oct 18, 2022

ranocha mentioned this pull request Dec 31, 2022

Big invalidation method: iterate(v::Union{Base.KeySet{<:Any, <:DataStructures.SwissDict}, Base.ValueIterator{<:DataStructures.SwissDict}}, state) JuliaCollections/DataStructures.jl#841

Closed

fix apply_type_tfunc accuracy

cf1c72f

Co-authored-by: Shuhei Kadowaki <[email protected]>

aviatesk force-pushed the hr/tfuncs branch from 8b036d7 to cf1c72f Compare January 13, 2023 06:14

aviatesk closed this Jan 18, 2023

Uh oh!

fix apply_type_tfunc accuracy #47136

fix apply_type_tfunc accuracy #47136

Uh oh!

Conversation

ranocha commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviatesk left a comment

Choose a reason for hiding this comment

Uh oh!

aviatesk commented Oct 12, 2022

Uh oh!

ranocha commented Oct 12, 2022

Uh oh!

nanosoldier commented Oct 12, 2022

Uh oh!

ranocha commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aviatesk commented Oct 12, 2022

Uh oh!

ranocha commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviatesk commented Oct 18, 2022

Uh oh!

ranocha commented Oct 18, 2022

Uh oh!

ranocha commented Oct 18, 2022

Uh oh!

aviatesk commented Oct 18, 2022

Uh oh!

aviatesk commented Oct 18, 2022

Uh oh!

aviatesk commented Oct 18, 2022

Uh oh!

oscardssmith commented Oct 18, 2022

Uh oh!

nanosoldier commented Oct 18, 2022

Uh oh!

vtjnash commented Oct 18, 2022

Uh oh!

gbaraldi commented Oct 18, 2022

Uh oh!

aviatesk commented Jan 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fix `apply_type_tfunc` accuracy #47136

fix `apply_type_tfunc` accuracy #47136

ranocha commented Oct 12, 2022 •

edited

Loading

ranocha commented Oct 12, 2022 •

edited

Loading

ranocha commented Oct 12, 2022 •

edited

Loading