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

Ensure invalidated target gets logged #49048

Merged
merged 1 commit into from
Mar 20, 2023
Merged

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Mar 18, 2023

To interpret causes of invalidation, it's helpful to understand what signature got invalidated. #48954 inadvertently dropped this info from the logging stream; this commit restores it.

To interpret causes of invalidation, it's helpful to understand what
signature got invalidated. #48954 inadvertently dropped this info
from the logging stream; this commit restores it.
@@ -2021,6 +2021,11 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
}
jl_array_ptr_1d_push(oldmi, (jl_value_t*)mi);
invalidate_external(mi, max_world);
if (_jl_debug_method_invalidation) {
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand this code, but isn't this pushed into the list later if invalidated is set? Why do we need to push something here too? Is this supposed to have a loctag of jl_box_int32(depth=1); instead (and increment depth argument of invalidate_method_instance)?

Copy link
Sponsor Member Author

@timholy timholy Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method gets pushed, but not the specific MethodInstance. On previous versions, the format was:

backedge => depth (e.g., `mcc48954i::MethodInstance, 1` in the new test)
targetmi => "jl_method_table_insert" (`target = mc48954i::MethodInstance` in the new test)
targetm => "jl_method_table_insert" (`target = which(mc48954, (AbstractFloat, Int))::Method` in the new test)

Recent master has dropped the middle of this pair; this PR just restores it.

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Mar 20, 2023
@vtjnash vtjnash merged commit 99fce41 into master Mar 20, 2023
@vtjnash vtjnash deleted the teh/invalidation_target branch March 20, 2023 23:42
@vtjnash vtjnash added backport 1.9 Change should be backported to release-1.9 and removed merge me PR is reviewed. Merge when all tests are passing labels Mar 20, 2023
KristofferC pushed a commit that referenced this pull request Mar 24, 2023
To interpret causes of invalidation, it's helpful to understand what
signature got invalidated. #48954 inadvertently dropped this info
from the logging stream; this commit restores it.

(cherry picked from commit 99fce41)
@KristofferC KristofferC mentioned this pull request Mar 24, 2023
52 tasks
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Mar 31, 2023
Xnartharax pushed a commit to Xnartharax/julia that referenced this pull request Apr 19, 2023
To interpret causes of invalidation, it's helpful to understand what
signature got invalidated. JuliaLang#48954 inadvertently dropped this info
from the logging stream; this commit restores it.
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