Add tests for method invalidation fixes

[rikhuijzer]

This PR adds tests for method invalidation fixes (fixes #47022).

As an example, it adds a test which verifies that we avoid 1154 method invalidations for downstream packages which insert Base.convert(::Type{String}, ::X) or similar and adds tests. Packages which benefit from these changes include CategoricalArrays.jl and ProgressLogging.jl. The PR which introduced this regression is listed below.

julia> using SnoopCompileCore

julia> import Base

julia> struct X end

julia> invs = @snoopr Base.convert(::Type{String}, ::X) = X()
[...]

julia> using SnoopCompile

julia> length(uinvalidated(invs))
1154

[KristofferC]

On 1.8 this seems to cause zero invalidations. Any idea what made it start?

[rikhuijzer]

On 1.8 this seems to cause zero invalidations. Any idea what made it start?

Well spotted. No, I have no idea.

[thchr]

It could be that this was caused by https://github.com/JuliaLang/julia/pull/46573/files#diff-85727f3920d7dc1daa391d682b072afbc4d4dddcc66f404d3d0675cf900fd14f or https://github.com/JuliaLang/julia/pull/46573/files#diff-a793cc0e967ca6d4cb69ce74f86f9d6908ad01fab7b50e63a3fad8c7455409fa; not sure.

[rikhuijzer]

It could be that this was caused by https://github.com/JuliaLang/julia/pull/46573/files#diff-85727f3920d7dc1daa391d682b072afbc4d4dddcc66f404d3d0675cf900fd14f or https://github.com/JuliaLang/julia/pull/46573/files#diff-a793cc0e967ca6d4cb69ce74f86f9d6908ad01fab7b50e63a3fad8c7455409fa; not sure.

Thanks. It was this change which caused the invalidations:

- convert(::Type{T}, s::AbstractString) where {T<:AbstractString} = T(s)
+ convert(::Type{T}, s::AbstractString) where {T<:AbstractString} = T(s)::T

I've reverted that change in 98f8c23 and, with that, fixed all method invalidations for the insertion of the convert (credits to Kristoffer for the question). It doesn't appear to have much of a negative effect on @snoopr using StaticArrays:

julia> using SnoopCompileCore

julia> invs = @snoopr(using StaticArrays);

julia> using SnoopCompile

julia> length(uinvalidated(invs))
84

[thchr]

Do you understand why the change in #46573 caused these new invalidations? I don't understand how type-asserting there could negatively impact invalidations here - but would really like to understand that (e.g., do the other type-assertions in that PR have a similar risk?).

[rikhuijzer]

Do you understand why the change in #46573 caused these new invalidations?

I also do not understand how the change causes the invalidations. The only difference visible in @code_warntype is the extra Core.typeassert.

(e.g., do the other type-assertions in that PR have a similar risk?).

It does look like it yes. This is what happens inside OrderedCollections.jl:

julia> using SnoopCompileCore

julia> import Base

julia> struct A{K,V} <: AbstractDict{K,V} end

julia> invs = @snoopr(Base.convert(::Type{A}, ::AbstractDict) = A());

julia> using SnoopCompile

julia> length(uinvalidated(invs))
12

and the number went to zero after the following change:

function convert(::Type{T}, x::AbstractDict) where T<:AbstractDict
-   h = T(x)::T
+   h = T(x)
    if length(h) != length(x)
        error("key collision during dictionary conversion")
    end
    return h
end

But, based on some large packages that I've checked for invalidations the invalidations caused by #46573 aren't too bad. Only a few dozen. It's probably best to consider those fixes out of scope for this PR?

[rikhuijzer]

Test failures are in Sockets and should be unrelated

[thchr]

It's probably best to consider those fixes out of scope for this PR?

Agreed: but confounding that my changes in #46573 - intended to decrease invalidations - seems to have caused them elsewhere.

[KristofferC]

It's a bit weird now, we introduce ::T for all convert methods to help with invalidations and now we start removing them. It feels like we need to understand the root cause here a bit better to know what is really going on so this doesn't just become a game of whack a mole.

[ranocha]

It feels like we need to understand the root cause here a bit better to know what is really going on so this doesn't just become a game of whack a mole.

I definitely agree with this. Would it make sense to discuss this aspect somewhere else and merge this PR soon to get the new testing infrastructure into main? This should help with understanding the issue and notify us when new invalidations pop up again after fixing them in a tedious process.

[rikhuijzer]

I've changed the title of the PR to more clearly indicate that the main focus is the testing logic. The invalidation fix is more of a test case.