Ensure accurate invalidation logging data #48982
Conversation
src/staticdata_utils.c
Outdated
| if (matches == jl_nothing) { | ||
| assert(sig); | ||
| int ambig = 0; | ||
| matches = jl_matching_methods((jl_tupletype_t*)sig, jl_nothing, | ||
| -1, 0, minworld, &min_valid, &max_valid, &ambig); | ||
| } |
There was a problem hiding this comment.
this seems to calculate an entirely different thing from the original (which did a setdiff on the result computed with a length limit of +INT32_MAX not -1)
There was a problem hiding this comment.
This only gets triggered from matches == jl_nothing which comes from
Lines 872 to 876 in 7ba7e32
It basically restores the version that was used before the efficiency-improvement in #48841. If this isn't correct, let's put the correct thing here, but it seemed to pass muster before the efficiency-improvement.
There was a problem hiding this comment.
The change in #48841 alters the algorithm selection from the -1 algorithm to the positive limit algorithm. What you probably want is to make the jl_array_len(expected) parameter selectable based on how many conflicts you want returned. E.g. up to INT32_MAX conflicts, or just +4 or such until it gives us and returns "too many"
There was a problem hiding this comment.
OK, I added just one extra space. I think we only need to identify one cause; if there turn out to be multiples, folks can re-run the analysis after fixing the one that got identified. I pushed this change.
However, then I tried modifying the test to add a second invalidating method. Then I got nothing in that slot again 😢. Big bummer.
I haven't delved into how jl_matching_methods works in a while, but I am surprised it returned nothing rather than a partial list. Is there anything "safe" short of re-running the analysis with -1? While I'm OK with only returning a single source of invalidation, returning nothing is simply unacceptable. As you point out above, we might have to re-run the setdiff. But I think I'd rather do that than miss causes of invalidation. What are your thoughts?
There was a problem hiding this comment.
We can either do +1 to get the list if there was only 1 cause, or +N to get up to N causes (before declaring "a lot") or INT32_MAX to get all causes.
There was a problem hiding this comment.
OK, and INT32_MAX would be acceptable? Or does that cause a performance hit?
There was a problem hiding this comment.
I went ahead and just changed it in the original computation. But if instead you meant that I should use INT32_MAX to recompute the matches when logging, just LMK.
This modifies #48841 to restore the required logging data. By collecting at least one additional match, we retain the possibility of identifying at least one trigger of invalidation.
timholy
force-pushed
the
teh/repair_48841
branch
from
09ac725 to
9332018
Compare
Co-authored-by: Jameson Nash <[email protected]>
|
Please try to make commit messages accurate when merging. |
* Ensure accurate invalidation logging data This modifies JuliaLang#48841 to restore the required logging data. By collecting at least one additional match, we retain the possibility of identifying at least one trigger of invalidation. * Bump number of invalidation causes * Update src/staticdata_utils.c Co-authored-by: Jameson Nash <[email protected]> --------- Co-authored-by: Jameson Nash <[email protected]>
* Ensure accurate invalidation logging data This modifies JuliaLang#48841 to restore the required logging data. By collecting at least one additional match, we retain the possibility of identifying at least one trigger of invalidation. * Bump number of invalidation causes * Update src/staticdata_utils.c Co-authored-by: Jameson Nash <[email protected]> --------- Co-authored-by: Jameson Nash <[email protected]>
This modifies #48841 to restore the required logging data. When logging is on, it recalculates the method-match as needed, casting a wider net to ensure that we identify the trigger of the invalidation.
Closes #48980
Fixes #48967