-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add binding invalidations to log #58226
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds logging for binding invalidations to help SnoopCompile attribute a cause to these events.
- Introduces the jl_maybe_log_binding_invalidation function in src/gf.c.
- Augments the debug logging mechanism by appending both the triggering value (if non-null) and a location tag.
Files not reviewed (3)
- base/invalidation.jl: Language not supported
- test/precompile.jl: Language not supported
- test/worlds.jl: Language not supported
Comments suppressed due to low confidence (1)
src/gf.c:1842
- [nitpick] Consider adding tests for jl_maybe_log_binding_invalidation to ensure it logs correctly in both cases when 'replaced' is non-null and when it is null.
JL_DLLEXPORT void jl_maybe_log_binding_invalidation(jl_value_t *replaced)
base/invalidation.jl
Outdated
end | ||
end | ||
# This assumes that we haven't gotten here unless something needed to be invalidated | ||
ccall(:jl_maybe_log_binding_invalidation, Cvoid, (Any,), invalidated_bpart) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems potentially problematic, because the invalidated code instances will get interleaved with nested invalidations from other bindings. Can SnoopCompile disambiguate this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also should probably check if anything was actually invalidated by adding appropriate boolean returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's exactly the answer I was fishing for.
WRT interleaving, here's what the log looks like now: 12-element Vector{Any}:
MethodInstance for Main.Test98Main_worlds.makelbi(::Int64)
1
MethodInstance for Main.Test98Main_worlds.makelbi(::Int64)
"jl_maybe_log_binding_invalidation"
BindingPartition 39141:39152 - constant binding to @world(Main.Test98Main_worlds.LogBindingInvalidation, 39141:39152)
"jl_maybe_log_binding_invalidation"
MethodInstance for getproperty(::Module, ::Symbol)
1
MethodInstance for Main.Test98Main_worlds.flbi()
2
BindingPartition 39146:39155 - constant binding to @world(Main.Test98Main_worlds.LogBindingInvalidation, 39141:39152)(1)
"jl_maybe_log_binding_invalidation" Is your concern that if other types depend on With this implementation from the second commit, I had to define |
The interleaving concern is that if there is a backedge between bindings, it is ambiguous, whether a particular CodeInstance was invalidated by the outer binding or by an inner recursion to |
Good point. I think this fixes it. It's worth noting that, unless I'm not looking in the right places, |
Currently we log the invalidated backedges of a binding invalidation, but the actual trigger of the invalidation is not logged. This is needed to allow SnoopCompile to attribute a cause to those invalidations.
08bd20a
to
48b1f7f
Compare
Currently we log the invalidated backedges of a binding invalidation, but the actual trigger of the invalidation is not logged. This is needed to allow SnoopCompile to attribute a cause to those invalidations. (cherry picked from commit 75946ce)
Currently we log the invalidated backedges of a binding invalidation, but the actual trigger of the invalidation is not logged. This is needed to allow SnoopCompile to attribute a cause to those invalidations. (cherry picked from commit 75946ce)
Currently we log the invalidated backedges of a binding invalidation, but the actual trigger of the invalidation is not logged. This is needed to allow SnoopCompile to attribute a cause to those invalidations.