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

jl_debug_method_invalidation: return a list of instances #35768

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented May 6, 2020

Rather than print information about method invalidations, this allows us to store the MethodInstances themselves. The reason I think this is attractive is that analyzing the reason for invalidations sometimes benefits from type intersection, and it will be easier to perform that kind of analysis if we don't have to try to reconstruct the type from its repr.

You can see a WIP analysis in SnoopCompile here: https://github.com/timholy/SnoopCompile.jl/blob/teh/invalidations/src/invalidations.jl

The biggest negative here is that this gets in the way of some potential debug information, e.g., if Julia crashes or doesn't get through bootstrap. So this isn't without some loss, CC @vtjnash. Nevertheless I think method invalidation is rarely an issue in such cases, so perhaps it's a loss we can live with.

There are some "competing" PRs, I'll list them as being closed by this one (should we choose to go this way):
Closes #35745
Closes #35747

CC @ianshmean

@KristofferC
Copy link
Sponsor Member

Should this also be updated?

julia/src/dump.c

Lines 2389 to 2391 in 7ba7daf

if (jl_debug_method_invalidation) {
jl_static_show(JL_STDOUT, (jl_value_t*)caller);
jl_uv_puts(JL_STDOUT, "<<<\n", 4);

@vtjnash
Copy link
Sponsor Member

vtjnash commented May 6, 2020

Also needs to be added to the GC and marked as globally rooted. I'm not worried about the loss in early printing—this'll be useful if I start adding a lot of false positive that need to be filtered out later (and add the code to filter them).

@timholy
Copy link
Sponsor Member Author

timholy commented May 6, 2020

Thanks @vtjnash. Sorry to be dense.... Is this just call JL_GC_PROMISE_ROOTED? It's been a while since I looked at the GC and I think some things have changed. Are there docs?

@timholy
Copy link
Sponsor Member Author

timholy commented May 6, 2020

Also, the invalidations last "forever," right? In which case do we need to save the methods invalidated through the backedges? We could just recover those later, right?

EDIT: on second thought, we wouldn't want those changing before we returned to the user, so probably best to keep this design.

src/gf.c Outdated Show resolved Hide resolved
src/gf.c Outdated Show resolved Hide resolved
@timholy
Copy link
Sponsor Member Author

timholy commented May 6, 2020

This should pass. Still 1.5 material or are we into 1.6 now?

The last commit is worth noting, I've found it really helps make sense of why some methods are being invalidated. Example
New signature: Tuple{typeof(convert),Type{C},Any} where C<:Colorant
Old signature: Tuple{typeof(convert),Type{T} where T<:(Base.Generator{_A,Base.var"#5#6"{Pair}} where _A),Base.Generator{_A,Base.var"#5#6"{Pair}} where _A}
The old signature is a back edge of

julia/base/iterators.jl

Lines 1223 to 1226 in a4641c8

@inline function Stateful(itr::T) where {T}
VS = approx_iter_type(T)
return new{T, VS}(itr, iterate(itr)::VS, 0)
end

Intersection:

julia> typeintersect(sig1, sig2)
Tuple{typeof(convert),Type{Union{}},Base.Generator{_A,Base.var"#5#6"{Pair}} where _A}

But this is already covered by

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

It makes me think we should check the intersection and see if it corresponds to a different method. Of course that will slow things down some. But I think that one check would obviate most of #35733.

@timholy
Copy link
Sponsor Member Author

timholy commented May 10, 2020

Used in JuliaLang/www.julialang.org#794

… and tags

Rather than print debug info, this stores the relevant entities to an
array.  This facilitates easier analysis of the causes, for example
making it possible to automatically walk the hierarchy of related code
objects (methods, functions, signature types), perform
type-intersection, etc.

Analysis code will live outside Julia proper (SnoopCompile and MethodAnalysis
are two early "consumers").
@timholy
Copy link
Sponsor Member Author

timholy commented May 14, 2020

I've squashed this down to a single commit to make it easier to resolve conflicts with #35877. From my standpoint this is good to go and can be merged any time. With apologies to Jeff, it might make sense to merge this before #35877 so one can more readily compare before & after. (For instance, in the blog post I analyze unique MethodInstance invalidations.)

@timholy timholy merged commit 65c2a03 into master Jun 3, 2020
@timholy timholy deleted the teh/invalidations_tree branch June 3, 2020 12:27
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
… and tags (JuliaLang#35768)

Rather than print debug info, this stores the relevant entities to an
array.  This facilitates easier analysis of the causes, for example
making it possible to automatically walk the hierarchy of related code
objects (methods, functions, signature types), perform
type-intersection, etc.

Analysis code will live outside Julia proper (SnoopCompile and MethodAnalysis
are two early "consumers").
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants