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

confusing output of mt_backedges #357

Closed
vtjnash opened this issue Apr 18, 2023 · 4 comments · Fixed by #359
Closed

confusing output of mt_backedges #357

vtjnash opened this issue Apr 18, 2023 · 4 comments · Fixed by #359

Comments

@vtjnash
Copy link
Collaborator

vtjnash commented Apr 18, 2023

I am confused about seeing mt_backedges after using OmniPackage, since == should never have mt_backedges created (there already exists a catchall method) and indeed, it is printing a MethodInstance not a signature after the word signature. Does this make sense?

inserting ==(p::MultivariatePolynomials.AbstractPolynomialLike, α) @ MultivariatePolynomials ~/.julia/packages/MultivariatePolynomials/sWAOE/src/operators.jl:45 invalidated:
   mt_backedges:  1: signature ==(x, y) @ Base Base.jl:159 (formerly ==(x, y) @ Base Base.jl:159) triggered MethodInstance for !=(::Any, ::Symbol) (0 children)
                  2: signature ==(x, y) @ Base Base.jl:159 (formerly ==(x, y) @ Base Base.jl:159) triggered MethodInstance for isequal(::Any, ::Symbol) (0 children)
@timholy
Copy link
Owner

timholy commented Apr 20, 2023

I think so, and I think I just screwed this up. This comment seems suspicious and may be the source of the trouble. What I called "delayed" invalidations are those where a package's backedges got invalidated by code that was loaded earlier from an external source: where the calleemi is no longer the most specific applicable method that could be used for the callermi.

Would you agree that this test yielding

(SnoopCompile) pkg> activate .
  Activating project at `~/.julia/dev/SnoopCompile/test/testmodules/Stale`

julia> invalidations = @snoopr begin
           using StaleA, StaleC
           using StaleB
       end
15-element Vector{Any}:
  MethodInstance for StaleA.use_stale(::Vector{Any})
 1
  MethodInstance for StaleA.build_stale(::Int64)
 2
  MethodInstance for StaleA.stale(::Any)
  "jl_method_table_insert"
  stale(x::String) @ StaleC ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleC/src/StaleC.jl:5
  "jl_method_table_insert"
  MethodInstance for StaleA.stale(::String)
  "insert_backedges_callee"
 1
  Any[stale(x::String) @ StaleC ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleC/src/StaleC.jl:5]
  MethodInstance for StaleB.useA()
  "verify_methods"
 1

julia> trees = invalidation_trees(invalidations)
1-element Vector{SnoopCompile.MethodInvalidations}:
 inserting stale(x::String) @ StaleC ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleC/src/StaleC.jl:5 invalidated:
   mt_backedges: 1: signature stale(x::String) @ StaleC ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleC/src/StaleC.jl:5 (formerly stale(x) @ StaleA ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleA/src/StaleA.jl:3) triggered MethodInstance for StaleB.useA() (0 children)
   backedges: 1: superseding stale(x) @ StaleA ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleA/src/StaleA.jl:3 with MethodInstance for StaleA.stale(::Any) (2 children)

is emblematic of the same problem? If so, let's focus our fix attempts there, since it's just a wee bit simpler than OmniPackage.

@timholy
Copy link
Owner

timholy commented Apr 20, 2023

Does this look more accurate?

julia> tree = only(trees)
inserting stale(x::String) @ StaleC ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleC/src/StaleC.jl:5 invalidated:
   backedges: 1: superseding stale(x) @ StaleA ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleA/src/StaleA.jl:3 with MethodInstance for StaleA.stale(::Any) (2 children)
              2: superseding stale(x) @ StaleA ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleA/src/StaleA.jl:3 with MethodInstance for StaleA.stale(::String) (1 children)


julia> print_tree(tree.backedges[1])
MethodInstance for StaleA.stale(::Any) at depth 0 with 2 children
└─ MethodInstance for StaleA.use_stale(::Vector{Any}) at depth 1 with 1 children
   └─ MethodInstance for StaleA.build_stale(::Int64) at depth 2 with 0 children

julia> print_tree(tree.backedges[2])
MethodInstance for StaleA.stale(::String) at depth 1 with 1 children
└─ MethodInstance for StaleB.useA() at depth 2 with 0 children

Obtained from:

diff --git a/src/invalidations.jl b/src/invalidations.jl
index 550f38e..2d18d42 100644
--- a/src/invalidations.jl
+++ b/src/invalidations.jl
@@ -156,12 +156,12 @@ end
 struct MethodInvalidations
     method::Method
     reason::Symbol   # :inserting or :deleting
-    mt_backedges::Vector{Pair{Any,Union{InstanceNode,MethodInstance}}}   # sig=>root for immediate, calleemi=>callermi for delayed
+    mt_backedges::Vector{Pair{Type,Union{InstanceNode,MethodInstance}}}   # sig=>root
     backedges::Vector{InstanceNode}
     mt_cache::Vector{MethodInstance}
     mt_disable::Vector{MethodInstance}
 end
-methinv_storage() = Pair{Any,InstanceNode}[], InstanceNode[], MethodInstance[], MethodInstance[]
+methinv_storage() = Pair{Type,InstanceNode}[], InstanceNode[], MethodInstance[], MethodInstance[]
 function MethodInvalidations(method::Method, reason::Symbol)
     MethodInvalidations(method, reason, methinv_storage()...)
 end
@@ -467,8 +467,14 @@ function invalidation_trees(list; exclude_corecompiler::Bool=true)
                         ret = get(backedge_table, next, nothing)
                         ret === nothing && (@warn "$next not found in `backedge_table`"; continue)
                         trig, causes = ret
-                        newnode = InstanceNode(mi, 1)
-                        push!(mt_backedges, trig => newnode)
+                        if isa(trig, MethodInstance)
+                            newnode = InstanceNode(trig, 1)
+                            push!(newnode.children, InstanceNode(mi, 2))
+                            push!(backedges, newnode)
+                        else
+                            newnode = InstanceNode(mi, 1)
+                            push!(mt_backedges, trig => newnode)
+                        end
                         backedge_table[mi] = newnode
                         for cause in causes
                             add_method_trigger!(methodinvs, cause, :inserting, mt_backedges, backedges, mt_cache, mt_disable)

@timholy
Copy link
Owner

timholy commented Apr 20, 2023

Hmm, something else I'm a bit worried about: if I modify the end of StaleB like this:

diff --git a/test/testmodules/Stale/StaleB/src/StaleB.jl b/test/testmodules/Stale/StaleB/src/StaleB.jl
index 6f05e35..baa64be 100644
--- a/test/testmodules/Stale/StaleB/src/StaleB.jl
+++ b/test/testmodules/Stale/StaleB/src/StaleB.jl
@@ -7,8 +7,12 @@ using StaleA

 # This will be invalidated if StaleC is loaded
 useA() = StaleA.stale("hello")
+useAA() = useA()

 # force precompilation
-useA()
+begin
+    Base.Experimental.@force_compile
+    useAA()
+end

 end

I can verify that useA has a backedge with useAA. Yet if I run the invalidation test:

julia> invalidations = @snoopr begin
           using StaleA, StaleC
           using StaleB
       end
15-element Vector{Any}:
  MethodInstance for StaleA.use_stale(::Vector{Any})
 1
  MethodInstance for StaleA.build_stale(::Int64)
 2
  MethodInstance for StaleA.stale(::Any)
  "jl_method_table_insert"
  stale(x::String) @ StaleC ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleC/src/StaleC.jl:5
  "jl_method_table_insert"
  MethodInstance for StaleA.stale(::String)
  "insert_backedges_callee"
 1
  Any[stale(x::String) @ StaleC ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleC/src/StaleC.jl:5]
  MethodInstance for StaleB.useA()
  "verify_methods"
 1

Why isn't useAA on this list? It is invalidated, but it's not logged:

julia> m = only(methods(StaleB.useAA))
useAA()
     @ StaleB ~/.julia/dev/SnoopCompile/test/testmodules/Stale/StaleB/src/StaleB.jl:10

julia> mi = first(m.specializations)
MethodInstance for StaleB.useAA()

julia> mi.cache
Core.CodeInstance(MethodInstance for StaleB.useAA(), #undef, 0x00000000000082cc, 0x0000000000000000, Int64, #undef, UInt8[0x04, 0x00, 0x14, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x16  …  0x0b, 0x2c, 0x00, 0x01, 0xaf, 0x3e, 0x05, 0x01, 0x00, 0x01], 0x00000d09, 0x00000d09, nothing, false, false, 0x01, Ptr{Nothing} @0x0000000000000000, Ptr{Nothing} @0x0000000000000000)

julia> mi.cache.max_world
0x0000000000000000

@vtjnash
Copy link
Collaborator Author

vtjnash commented Apr 20, 2023

It looks like _jl_debug_method_invalidation is only populated for methods after the first root, looking here it excludes idx from being reported: https://github.com/JuliaLang/julia/blob/bb118c99ce5b08dc1be2c88a4f9d561646b06d63/src/staticdata_utils.c#L1025-L1034

timholy added a commit to JuliaLang/julia that referenced this issue Apr 20, 2023
Addresses timholy/SnoopCompile.jl#357 (comment)
using the observation in the following comment.

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
timholy added a commit that referenced this issue Apr 20, 2023
These were incorrectly assigned to mt_backedges.
Fixes #357
timholy added a commit that referenced this issue Apr 21, 2023
These were incorrectly assigned to mt_backedges.
Fixes #357
timholy added a commit to JuliaLang/julia that referenced this issue Apr 21, 2023
* Add missing entry to invalidation log

Addresses timholy/SnoopCompile.jl#357 (comment)
using the observation in the following comment.

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Fix indentation

---------

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
KristofferC pushed a commit to JuliaLang/julia that referenced this issue Apr 22, 2023
* Add missing entry to invalidation log

Addresses timholy/SnoopCompile.jl#357 (comment)
using the observation in the following comment.

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Fix indentation

---------

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
(cherry picked from commit 23a5b04)
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 a pull request may close this issue.

2 participants