Skip to content

Conversation

timholy
Copy link
Member

@timholy timholy commented Apr 25, 2025

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.

@timholy timholy requested review from Keno and Copilot April 25, 2025 12:19
Copy link

@Copilot Copilot AI left a 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)

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)
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

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.

@timholy
Copy link
Member Author

timholy commented Apr 25, 2025

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 LogBindingInvalidation, the sequencing in the log may become confusing to parse?

With this implementation from the second commit, I had to define makelbi to trigger logging for the type invalidation. (The global const glbi invalidation happens regardless.) Not sure exactly which behavior we prefer.

@Keno
Copy link
Member

Keno commented Apr 25, 2025

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 invalidate_code_for_globalref!

@timholy
Copy link
Member Author

timholy commented May 7, 2025

Good point. I think this fixes it.

It's worth noting that, unless I'm not looking in the right places, there's little to no direct testing of bindings with backedges. rebinding.jl is what I was missing!

@timholy timholy added this to the 1.12 milestone May 7, 2025
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label May 29, 2025
timholy added 3 commits May 29, 2025 19:28
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.
@vtjnash vtjnash force-pushed the teh/log_binding_invalidations branch from 08bd20a to 48b1f7f Compare May 29, 2025 19:28
@giordano giordano merged commit 75946ce into master May 30, 2025
7 checks passed
@giordano giordano deleted the teh/log_binding_invalidations branch May 30, 2025 10:53
@giordano giordano added backport 1.12 Change should be backported to release-1.12 and removed merge me PR is reviewed. Merge when all tests are passing labels May 30, 2025
KristofferC pushed a commit that referenced this pull request Jul 8, 2025
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)
KristofferC pushed a commit that referenced this pull request Jul 8, 2025
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)
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Jul 9, 2025
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.

5 participants