Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix invalidations due to convert(Nothing, ::Any) #39434

Merged
merged 1 commit into from
Feb 6, 2021

Conversation

simeonschaub
Copy link
Member

Before this PR:

julia> using SnoopCompileCore

julia> struct A end

julia> @snoopr (::Type{<:A})(::AbstractVector) = 1
14-element Vector{Any}:
  MethodInstance for convert(::Type{Union{}}, ::AbstractArray)
 1
  MethodInstance for convert(::Type{Nothing}, ::Any)
 2
  MethodInstance for Union{}(::AbstractArray)
  "jl_method_table_insert"
  MethodInstance for Core.Compiler.convert(::Type{V} where V<:Core.Compiler.BitArray{1}, ::Vector{var"#s122"} where var"#s122"<:Union{Float32, Float64})
 1
  MethodInstance for Core.Compiler.convert(::Type{T}, ::Vector{var"#s122"} where var"#s122"<:Union{Float32, Float64}) where T<:(Vector{T} where T)
 1
  MethodInstance for Union{}(::Vector{var"#s122"} where var"#s122"<:Union{Float32, Float64})
  "jl_method_table_insert"
  (::Type{var"#s3"} where var"#s3"<:A)(::AbstractVector{T} where T) in Main at REPL[3]:1
  "jl_method_table_insert"

After:

julia> using SnoopCompileCore

julia> struct A end

julia> @snoopr (::Type{<:A})(::AbstractVector) = 1
Any[]

This was causing invalidations in StaticArrays.

@simeonschaub simeonschaub added the compiler:latency Compiler latency label Jan 28, 2021
base/some.jl Outdated
Comment on lines 37 to 38
convert(::Type{Nothing}, x) = throw(MethodError(convert, (Nothing, x)))
convert(::Type{Nothing}, x::T) where {T>:Nothing} = throw(MethodError(convert, (Nothing, x)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two are equivalent, methods(convert) should only list one of them, in particular the latter as it has replaced the former. If you find that you need both to get the desired effect, something weird is going on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, you are correct:

julia> methods(convert, Tuple{Type{Nothing}, Any})
# 2 methods for generic function "convert":
[1] convert(::Type{Nothing}, ::Nothing) in Base at some.jl:39
[2] convert(::Type{Nothing}, x::T) where T>:Nothing in Base at some.jl:38

I think I was just a bit confused because I previously assumed the definition in line 35 would just be equivalent to defining convert(::Type{Nothing}, ::Nothing), but that caused problems.

Before this PR:
```julia
julia> using SnoopCompileCore

julia> struct A end

julia> @Snoopr (::Type{<:A})(::AbstractVector) = 1
14-element Vector{Any}:
  MethodInstance for convert(::Type{Union{}}, ::AbstractArray)
 1
  MethodInstance for convert(::Type{Nothing}, ::Any)
 2
  MethodInstance for Union{}(::AbstractArray)
  "jl_method_table_insert"
  MethodInstance for Core.Compiler.convert(::Type{V} where V<:Core.Compiler.BitArray{1}, ::Vector{var"#s122"} where var"#s122"<:Union{Float32, Float64})
 1
  MethodInstance for Core.Compiler.convert(::Type{T}, ::Vector{var"#s122"} where var"#s122"<:Union{Float32, Float64}) where T<:(Vector{T} where T)
 1
  MethodInstance for Union{}(::Vector{var"#s122"} where var"#s122"<:Union{Float32, Float64})
  "jl_method_table_insert"
  (::Type{var"#s3"} where var"#s3"<:A)(::AbstractVector{T} where T) in Main at REPL[3]:1
  "jl_method_table_insert"
```

After:
```julia
julia> using SnoopCompileCore

julia> struct A end

julia> @Snoopr (::Type{<:A})(::AbstractVector) = 1
Any[]
```

This was causing invalidations in StaticArrays.
@simeonschaub
Copy link
Member Author

@timholy Since this is my first foray into invalidation busting, would you mind taking a look whether this looks sensible to you?

Copy link
Sponsor Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect.

This PR should fix this ambiguity:

julia> convert(Nothing, 3)
ERROR: MethodError: convert(::Type{Union{}}, ::Int64) is ambiguous. Candidates:
  convert(::Type{T}, x::Number) where T<:AbstractChar in Base at char.jl:179
  convert(::Type{T}, x::Number) where T<:Number in Base at number.jl:7
  convert(::Type{Union{}}, x) in Base at essentials.jl:203
  convert(::Type{T}, arg) where T<:VecElement in Base at baseext.jl:8
Possible fix, define
  convert(::Type{Union{}}, ::Number)
Stacktrace:
 [1] convert(#unused#::Type{Nothing}, x::Int64)
   @ Base ./some.jl:36
 [2] top-level scope
   @ REPL[4]:1

But I'm puzzled about why this isn't picked up by detect_ambiguities(Base).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 6, 2021

Ambiguities are permitted (in the test) for Union{}

@simeonschaub
Copy link
Member Author

This PR should fix this ambiguity:

Ah, cool! Didn't even realize that. >: constraints in method dispatch always confuse me a bit 😄

@simeonschaub
Copy link
Member Author

@vtjnash I am sure this has been discussed at some point, but how much havoc would it cause to have a non-inclusive <:? It seems like most of the time, if you are dispatching on Type{<:...}, you don't actually want to include Type{Union{}}, but there is not really a good way to express this in the current type system.

@simeonschaub simeonschaub merged commit 8e61551 into master Feb 6, 2021
@simeonschaub simeonschaub deleted the sds/invalidations branch February 6, 2021 17:03
@timholy
Copy link
Sponsor Member

timholy commented Feb 7, 2021

xref #33780, #24179. Of course those still don't cover subset-but-not-equal as you need, but fixing #24179 would have to be done before this could even be attempted.

@timholy
Copy link
Sponsor Member

timholy commented Feb 10, 2021

xref #38455 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:latency Compiler latency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants