Add indirection in == fallback #44678
Conversation
The fallback for `==` just calls `===`. However, we also have types like `WeakRef` that add three specializations of `==` that just unwrap the `WeakRef`. It turns out this pattern is also used in packages, and one of them, ChainRulesCore, has >2300 dependent packages as of now. Unfortunately, when ChainRulesCore is loaded, it invalidates code that calls `==` on `WeakRef`. This includes much of the Serialization stdlib. This addresses the problem by defining the notion of "unwrapping for ==" by introducing the non-exported function `unwrap_isequal`. The fallback is just ``` unwrap_isequal(x) = x ``` but one can add specializations.
|
Regarding ChainRulesCore, there is a PR to just remove them JuliaDiff/ChainRulesCore.jl#524 because they are "only used for tests". |
|
Something went wrong when running your job: Unfortunately, the logs could not be uploaded. |
| @@ -116,7 +116,13 @@ include("range.jl") | |||
| include("error.jl") | |||
|
|
|||
| # core numeric operations & types | |||
| ==(x, y) = x === y | |||
| unwrap_isequal(x) = x | |||
There was a problem hiding this comment.
I think, if we go with this, it should be used much more broadly:
| unwrap_isequal(x) = x | |
| unwrap_for_compare(x) = x |
Then we need to go update ==, <, isless, isequal, hash etc.
But perhaps instead I should open an issue about eliminating WeakRef (in favor, similar to the atomic attribute, of being a runtime-settable attribute in Array flags or in the field properties of a struct). Unfortunately, we cannot delete WeakRef until v2 regardless, since we can give a replacement for it, but we cannot remove it, until then.
|
Is the intention to have |
|
I was intending that it should be overloaded by packages. However, I do think this has some issues. One of them is that conceptually this seems like it would fail to prevent invalidation if two "confusable" types both specialize julia> abstract type AbstractMyType end
julia> struct MyType1 <: AbstractMyType
val
end
julia> Base.unwrap_isequal(x::MyType1) = x.val
julia> d = Dict{AbstractMyType,Int}(MyType1(1) => 1)
Dict{AbstractMyType, Int64} with 1 entry:
MyType1(1) => 1
julia> f(x) = (d::Dict{AbstractMyType,Int})[AbstractMyType[x][1]]
f (generic function with 1 method)
julia> f(MyType1(1))
1
julia> code_typed(==, (AbstractMyType, AbstractMyType))
1-element Vector{Any}:
CodeInfo(
1 ── %1 = (isa)(x, MyType1)::Bool
└─── goto #3 if not %1
2 ── %3 = π (x, MyType1)
│ %4 = Base.getfield(%3, :val)::Any
└─── goto #4
3 ── %6 = Base.unwrap_isequal(x)::Any
└─── goto #4
4 ┄─ %8 = φ (#2 => %4, #3 => %6)::Any
│ %9 = (isa)(y, MyType1)::Bool
└─── goto #6 if not %9
5 ── %11 = π (y, MyType1)
│ %12 = Base.getfield(%11, :val)::Any
└─── goto #7
6 ── %14 = Base.unwrap_isequal(y)::Any
└─── goto #7
7 ┄─ %16 = φ (#5 => %12, #6 => %14)::Any
│ %17 = (%8 === x)::Bool
└─── goto #11 if not %17
8 ── %19 = (%16 === y)::Bool
└─── goto #10 if not %19
9 ── %21 = (x === y)::Bool
└─── return %21
10 ─ nothing::Nothing
11 ┄ %24 = (%8 == %16)::Any
└─── return %24
) => Any
julia> using SnoopCompileCore
julia> struct MyType2 <: AbstractMyType
val
end
julia> invs = @snoopr Base.unwrap_isequal(x::MyType2) = x.val
Any[]
julia> f(MyType2(1))
ERROR: KeyError: key MyType2(1) not found
Stacktrace:
[1] getindex(h::Dict{AbstractMyType, Int64}, key::MyType2)
@ Base ./dict.jl:516
[2] f(x::MyType2)
@ Main ./REPL[5]:1
[3] top-level scope
@ REPL[11]:1
julia> code_typed(==, (AbstractMyType, AbstractMyType))
1-element Vector{Any}:
CodeInfo(
1 ── %1 = (isa)(x, MyType1)::Bool
└─── goto #3 if not %1
2 ── %3 = π (x, MyType1)
│ %4 = Base.getfield(%3, :val)::Any
└─── goto #6
3 ── %6 = (isa)(x, MyType2)::Bool
└─── goto #5 if not %6
4 ── %8 = π (x, MyType2)
│ %9 = Base.getfield(%8, :val)::Any
└─── goto #6
5 ── %11 = Base.unwrap_isequal(x)::Any
└─── goto #6
6 ┄─ %13 = φ (#2 => %4, #4 => %9, #5 => %11)::Any
│ %14 = (isa)(y, MyType1)::Bool
└─── goto #8 if not %14
7 ── %16 = π (y, MyType1)
│ %17 = Base.getfield(%16, :val)::Any
└─── goto #11
8 ── %19 = (isa)(y, MyType2)::Bool
└─── goto #10 if not %19
9 ── %21 = π (y, MyType2)
│ %22 = Base.getfield(%21, :val)::Any
└─── goto #11
10 ─ %24 = Base.unwrap_isequal(y)::Any
└─── goto #11
11 ┄ %26 = φ (#7 => %17, #9 => %22, #10 => %24)::Any
│ %27 = (%13 === x)::Bool
└─── goto #15 if not %27
12 ─ %29 = (%26 === y)::Bool
└─── goto #14 if not %29
13 ─ %31 = (x === y)::Bool
└─── return %31
14 ─ nothing::Nothing
15 ┄ %34 = (%13 == %26)::Any
└─── return %34
) => Any
julia> struct MyType3 <: AbstractMyType
val
end
julia> invs = @snoopr Base.unwrap_isequal(x::MyType3) = x.val
Any[]
julia> code_typed(==, (AbstractMyType, AbstractMyType))
1-element Vector{Any}:
CodeInfo(
1 ─ %1 = Base.unwrap_isequal(x)::Any
│ %2 = Base.unwrap_isequal(y)::Any
│ %3 = (%1 === x)::Bool
└── goto #5 if not %3
2 ─ %5 = (%2 === y)::Bool
└── goto #4 if not %5
3 ─ %7 = (x === y)::Bool
└── return %7
4 ─ nothing::Nothing
5 ┄ %10 = (%1 == %2)::Any
└── return %10
) => AnyWhy aren't there any invalidations when you can verify that the implementation of |
|
By "confusable type" I assume you mean just The |
|
The fallback is always a runtime dispatch that returns |
|
I like the idea. Could also do |
|
Is there an alternate solution in the works or does this PR just need some more work? |
The fallback for
==just calls===. However, we also have typeslike
WeakRefthat add three specializations of==that justunwrap the
WeakRef. It turns out this pattern is also used inpackages, and one of them, ChainRulesCore, has >2300 dependent
packages as of now. Unfortunately, when ChainRulesCore is loaded,
it invalidates code that calls
==onWeakRef. This includesmuch of the Serialization stdlib.
Since I have a personal goal of getting Makie's TTFP to less than one second (see #44527), and I recently learned from @SimonDanisch that Makie uses the Serialization stdlib, I will need to find a way to fix this.
This addresses the problem by defining the notion of "unwrapping
for ==" by introducing the non-exported function
unwrap_isequal.The fallback is just
but one can add specializations.
Now, the obvious concern about this implementation is that it may hurt performance. In cases of concrete inference it might have no impact, as something that uses the fallback like
produces the same code before and after. However,
@code_typedshows that the extra steps are not entirely DCEd:and so this might affect inlining. Moreover, there is also a chance for a performance regression in poorly-inferred code. So benchmarks are crucial: @nanosoldier
runbenchmarks(ALL, vs=":master").To show that this fixes the problem, on master:
but with this diff in ChainRulesCore:
one gets