Eliminate most invalidations from loading FixedPointNumbers by timholy · Pull Request #35733 · JuliaLang/julia

timholy · 2020-05-04T18:10:20Z

On master (or really, on #35714), loading FixedPointNumbers ("FPN") produces a jl_debug_method_invalidation log of nearly 600 lines. In conjunction with a couple of changes to FPN (below), this reduces it to 43, of which only 17 are actual MethodInstances. Most of the remaining ones derive from invalidating convert(Type{T<:Int64}, Int64) where {T<:Int64}, which gets invalidated by

(::Type{X})(x::Number) where {X <: FixedPoint} = _convert(X, x)

I have not been successful in figuring out how to head that off short of @evaling dedicated methods like convert(::Type{Int64}, x::Int64) = x.

Here's the FPN diff (EDIT: new and improved):

diff --git a/src/FixedPointNumbers.jl b/src/FixedPointNumbers.jl
index 160c133..3c3bd33 100644
--- a/src/FixedPointNumbers.jl
+++ b/src/FixedPointNumbers.jl
@@ -48,7 +48,8 @@ rawtype(::Type{X}) where {T, X <: FixedPoint{T}} = T
 *(x::Real, ::Type{X}) where {X <: FixedPoint} = _convert(X, x)
 
 # constructor-style conversions
-(::Type{X})(x::Real) where {X <: FixedPoint} = _convert(X, x)
+(::Type{X})(x::X) where {X <: FixedPoint}      = x
+(::Type{X})(x::Number) where {X <: FixedPoint} = _convert(X, x)
 
 function (::Type{<:FixedPoint})(x::AbstractChar)
     throw(ArgumentError("FixedPoint (Fixed or Normed) cannot be constructed from a Char"))
@@ -97,7 +98,6 @@ one(::Type{X}) where {X <: FixedPoint} = oneunit(X)
 inv_rawone(x) = (@generated) ? (y = 1.0 / rawone(x); :($y)) : 1.0 / rawone(x)
 
 # traits
-sizeof(::Type{X}) where {X <: FixedPoint} = sizeof(rawtype(X))
 eps(::Type{X}) where {X <: FixedPoint} = X(oneunit(rawtype(X)), 0)
 typemax(::Type{T}) where {T <: FixedPoint} = T(typemax(rawtype(T)), 0)
 typemin(::Type{T}) where {T <: FixedPoint} = T(typemin(rawtype(T)), 0)

timholy · 2020-05-04T18:13:13Z

CC @johnnychen94, @kimikage

StefanKarpinski · 2020-05-04T19:04:05Z

 """
 function convert end

+convert(::Type{Union{}}, x::Union{}) = throw(MethodError(convert, (Union{}, x)))


This is my favorite method.

Yes, this PR has been an exercise in "how on earth can inference discover an ambiguity here?"

timholy · 2020-05-04T19:16:34Z

I added another couple of weird methods and got it down to 33 lines with 6 real methods. I've also been able to simplify the FPN diff; I just edited the one above to be the new version.

One benchmark (Pkg is a juicy target for invalidation, it seems):

julia 1.4:

julia> using Pkg

julia> @time try Pkg.add("NoPkg") catch err end
   Updating registry at `~/.julia/registries/General`
   Updating git-repo `git@github.com:JuliaRegistries/General.git`
   Updating registry at `~/.julia/registries/HolyLabRegistry`
   Updating git-repo `git@github.com:HolyLab/HolyLabRegistry.git`
  3.968906 seconds (1.31 M allocations: 70.182 MiB, 1.79% gc time)

julia> @time try Pkg.add("NoPkg") catch err end
  0.100881 seconds (365.53 k allocations: 19.821 MiB, 50.80% gc time)

julia> @time try Pkg.add("NoPkg") catch err end
  0.053736 seconds (365.53 k allocations: 19.821 MiB)

julia> using FixedPointNumbers

julia> @time try Pkg.add("NoPkg") catch err end
  2.568494 seconds (555.27 k allocations: 28.923 MiB, 0.47% gc time)

So a 2.5 second hit to recompile Pkg methods after loading FixedPointNumbers.

This PR:

julia> using Pkg

julia> @time try Pkg.add("NoPkg") catch err end
   Updating registry at `~/.julia/registries/General`
   Updating git-repo `git@github.com:JuliaRegistries/General.git`
   Updating registry at `~/.julia/registries/HolyLabRegistry`
   Updating git-repo `git@github.com:HolyLab/HolyLabRegistry.git`
  4.528111 seconds (891.47 k allocations: 52.362 MiB, 0.24% gc time)

julia> @time try Pkg.add("NoPkg") catch err end
  0.015854 seconds (98.80 k allocations: 7.125 MiB)

julia> using FixedPointNumbers

julia> @time try Pkg.add("NoPkg") catch err end
  0.067083 seconds (125.96 k allocations: 8.486 MiB, 54.02% gc time)

Definitely not enough for a cup of tea.

So, another package made largely "side-effect free" with respect to other functionality. Whee!

timholy · 2020-05-04T19:23:33Z

Residual log, in case anyone has additional suggestions for improvement:

-- oneunit(Int32)
 first(Base.OneTo{T<:AbstractChar}) where {T<:AbstractChar}
>> FixedPointNumbers.oneunit(...) Tuple{typeof(Base.oneunit), Type{X}} where X<:(FixedPointNumbers.FixedPoint{T, f} where f where T<:Integer)
-- max(Int32, Int32)
-- *(Float64, Int64)
-- *(UInt64, Float64)
-- min(Int64, Int64)
-- min(Int32, Int32)
-- /(UInt64, Float64)
-- zero(Type{Int32})
-- <(Base.VersionNumber, Base.VersionNumber)
 promote_type(Type{Float64}, Type{S} where S<:Integer)
 promote_type(Type{Float64}, Type{S} where S<:Integer)
>> FixedPointNumbers.promote_rule(...) Tuple{typeof(Base.promote_rule), Type{T}, Type{Tf}} where Tf<:AbstractFloat where T<:(FixedPointNumbers.Normed{T, f} where f where T<:Unsigned)
oneunit(Type{T} where T<:AbstractChar)
-- one(Type{UInt64})
>> FixedPointNumbers.one(...) Tuple{typeof(Base.one), Type{X}} where X<:(FixedPointNumbers.FixedPoint{T, f} where f where T<:Integer)
-- +(Float64, Int64)
-- ==(Module, Module)
-- ==(Int64, Int64)
-- ==(Distributed.WorkerState, Distributed.WorkerState)
-- ==(Char, String)
-- ==(Base.Multimedia.TextDisplay, REPL.REPLDisplay{REPL.LineEditREPL})
-- ==(Expr, Int64)
-- ==(Int32, Int64)
-- ==(Int32, Int32)
-- ==(Symbol, Int64)
-- reinterpret(Type{Char}, UInt32)
-- <=(Int32, Int64)
-- (::Type{Char})(Int64)
convert(Type{T<:Int64}, Int64) where {T<:Int64}
 setindex!(Array{K, 1} where K<:Union{Int64, String, Symbol}, Int64, Int64)
>> FixedPointNumbers.FixedPoint(...) Tuple{Type{X}, Number} where X<:(FixedPointNumbers.FixedPoint{T, f} where f where T<:Integer)

JeffBezanson · 2020-05-04T19:53:52Z

I have some code somewhere that skips doing invalidations if the intersection would be ambiguous. I think that could avoid the need for the Union{} methods here. In fact I can't be confident nothing will go wrong having an argument marked ::Union{}. I didn't PR the changes before since in the cases I tried it only affected a small fraction of invalidations, but maybe it will work in this case.

timholy · 2020-05-04T19:56:16Z

ColorTypes only needs this change:

diff --git a/src/conversions.jl b/src/conversions.jl
index b74d961..79c1014 100644
--- a/src/conversions.jl
+++ b/src/conversions.jl
@@ -70,9 +70,9 @@ end
 # no-op and element-type conversions, plus conversion to and from transparency
 # Colorimetry conversions are in Colors.jl
 convert(::Type{C}, c::C) where {C<:Colorant} = c
-convert(::Type{C}, c) where {C<:Colorant} = cconvert(ccolor(C, typeof(c)), c)
+convert(::Type{C}, c::Colorant) where {C<:Colorant} = cconvert(ccolor(C, typeof(c)), c)
 cconvert(::Type{C}, c::C) where {C} = c
-cconvert(::Type{C}, c) where {C} = _convert(C, base_color_type(C), base_color_type(c), c)
+cconvert(::Type{C}, c::Colorant) where {C} = _convert(C, base_color_type(C), base_color_type(c), c)
 
 convert(::Type{C}, c::Color, alpha) where {C<:TransparentColor} = cconvert(ccolor(C, typeof(c)), c, alpha)
 cconvert(::Type{C}, c::Color, alpha) where {C<:TransparentColor} =_convert(C, base_color_type(C), base_color_type(c), c, alpha)

(plus some stuff to disable the error hints since that changed) and the total invalidation log for FixedPointNumbers + ColorTypes is down to 55 lines. This is from an original of 2053.

The fact that ColorTypes has largely been fixed by the same changes made for FPN is a relief; I was beginning to worry that each package would be a big lift on its own, this is the first sign I've seen of synergy.

timholy · 2020-05-04T19:57:33Z

skips doing invalidations if the intersection would be ambiguous

I wondered about that. I figured it would be worth seeing how much this contributed to the problem; it's certainly the majority for this package.

These changes, combined with more extensive changes to Julia itself, greatly reduce latency stemming from loading FixedPointNumbers. Ref JuliaLang/julia#35733. There will be very little benefit to this on its own, but we can at least find out if it works across Julia versions.

JuliaLang/julia#35733. This will have very little impact on its own, but it shouldn't hurt anything and it prepares the way for future gains.

vtjnash · 2020-05-04T21:46:00Z

 sizeof(x) = Core.sizeof(x)
+# The next two methods prevent invalidation
+sizeof(::Type{Union{}}) = Core.sizeof(Union{})
+sizeof(::Type{T}) where T = Core.sizeof(T)


This'll force us to generate code for each type, and forces us to add a dispatch, which I believe we don't want for it?

vtjnash · 2020-05-04T21:48:11Z

 zero(x::Number) = oftype(x,0)
 zero(::Type{T}) where {T<:Number} = convert(T,0)
+# prevent invalidation
+zero(x::Union{})      = throw(MethodError(zero, (x,)))


Suggested change

zero(x::Union{}) = throw(MethodError(zero, (x,)))

zero(x::Union{}) = unreachable

Though I can't promise that defining these might not cause other, unintended, side-effects.

💯 Agreed, I would hold off on adding these --- they could cause problems, and they're the kind of thing we should be able to handle automatically (i.e. if an argument is bottom, obviously the code is unreachable).

vtjnash · 2020-05-04T21:49:31Z

 """
-reduce_empty(op, T) = _empty_reduce_error()
-reduce_empty(::typeof(+), T) = zero(T)
+reduce_empty(op, ::Type{T}) where T = _empty_reduce_error()


Style nit: I think we usually write short form functions with the braces {} to visually distinguish them:

Suggested change

reduce_empty(op, ::Type{T}) where T = _empty_reduce_error()

reduce_empty(op, ::Type{T}) where {T} = _empty_reduce_error()

timholy · 2020-05-05T00:42:41Z

I'd be very happy to hold off on this, especially if there's an automatic solution on the horizon. I don't have a burning need to see these littering our method tables. Happy to treat it as a proof of principle for a more systematic approach.

JuliaLang/julia#35733. This will have very little impact on its own, but it shouldn't hurt anything and it prepares the way for future gains.

KristofferC · 2020-05-08T11:16:51Z

We should be able to keep all the ::Type{Union{}) ones though? And there are some other here that could be good to get in? Can we factor out the ::Union{} ones so the rest could potentially be merged?

Edit: #35803

tkf · 2020-05-09T01:31:38Z

-reduce_empty(op, T) = _empty_reduce_error()
-reduce_empty(::typeof(+), T) = zero(T)
+reduce_empty(op, ::Type{T}) where T = _empty_reduce_error()
+reduce_empty(::typeof(+), ::Type{Union{}}) = _empty_reduce_error()   # avoid invalidation


I think we should have this. It is not for avoiding invalidation but for "correctness." I think it's better to do _empty_reduce_error() in sum(Union{}[]) than throwing the MethodError.

These changes, combined with more extensive changes to Julia itself, greatly reduce latency stemming from loading FixedPointNumbers. Ref JuliaLang/julia#35733. There will be very little benefit to this on its own, but we can at least find out if it works across Julia versions.

timholy · 2020-08-02T20:37:12Z

Superseded by lots of other PRs

Eliminate most invalidations from loading FixedPointNumbers

a4e0f47

Prevent a few more invalidations

cd39cf7

StefanKarpinski reviewed May 4, 2020

View reviewed changes

timholy mentioned this pull request May 4, 2020

Reduce invalidations of other methods JuliaMath/FixedPointNumbers.jl#180

Merged

timholy added a commit to JuliaGraphics/ColorTypes.jl that referenced this pull request May 4, 2020

Reduce invalidations of other methods

e7c3f1b

JuliaLang/julia#35733. This will have very little impact on its own, but it shouldn't hurt anything and it prepares the way for future gains.

vtjnash reviewed May 4, 2020

View reviewed changes

timholy added a commit to JuliaGraphics/ColorTypes.jl that referenced this pull request May 5, 2020

Reduce invalidations of other methods

f71b9a9

JuliaLang/julia#35733. This will have very little impact on its own, but it shouldn't hurt anything and it prepares the way for future gains.

timholy added a commit to JuliaGraphics/ColorTypes.jl that referenced this pull request May 5, 2020

Reduce invalidations of other methods

fb4955d

JuliaLang/julia#35733. This will have very little impact on its own, but it shouldn't hurt anything and it prepares the way for future gains.

This was referenced May 5, 2020

Reduce invalidations of other methods JuliaGraphics/ColorTypes.jl#194

Merged

jl_debug_method_invalidation: return a list of instances #35768

Merged

RFC: eliminate Type{Union{}} as a source of invalidations and ambiguity #35780

Closed

KristofferC mentioned this pull request May 8, 2020

Eliminate some invalidations from loading FixedPointNumbers [tweaked #35733] #35803

Closed

tkf reviewed May 9, 2020

View reviewed changes

tkf mentioned this pull request May 9, 2020

remove test for empty tuple for mean JuliaStats/Statistics.jl#36

Merged

timholy mentioned this pull request May 11, 2020

Support init keyword in maximum/minimum #35839

Closed

tkf mentioned this pull request May 11, 2020

Implement reduce_empty(op, Union{}) #35843

Merged

timholy closed this Aug 2, 2020

timholy deleted the teh/invalidations_fpn branch August 2, 2020 20:37

	zero(x::Union{}) = throw(MethodError(zero, (x,)))
	zero(x::Union{}) = unreachable

	reduce_empty(op, ::Type{T}) where T = _empty_reduce_error()
	reduce_empty(op, ::Type{T}) where {T} = _empty_reduce_error()

Uh oh!

Conversation

timholy commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timholy commented May 4, 2020

Uh oh!

StefanKarpinski May 4, 2020

Choose a reason for hiding this comment

Uh oh!

timholy May 4, 2020

Choose a reason for hiding this comment

Uh oh!

timholy commented May 4, 2020

Uh oh!

timholy commented May 4, 2020

Uh oh!

JeffBezanson commented May 4, 2020

Uh oh!

timholy commented May 4, 2020

Uh oh!

timholy commented May 4, 2020

Uh oh!

vtjnash May 4, 2020

Choose a reason for hiding this comment

Uh oh!

vtjnash May 4, 2020

Choose a reason for hiding this comment

Uh oh!

JeffBezanson May 4, 2020

Choose a reason for hiding this comment

Uh oh!

vtjnash May 4, 2020

Choose a reason for hiding this comment

Uh oh!

timholy commented May 5, 2020

Uh oh!

KristofferC commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tkf May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timholy commented Aug 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

timholy commented May 4, 2020 •

edited

Loading

KristofferC commented May 8, 2020 •

edited

Loading

tkf May 9, 2020 •

edited

Loading