inference: ~~refine~~ replace recursion non-detection algorithm #23912
Conversation
| @@ -5463,6 +5464,7 @@ t4 = vcat(A23567, t2, t3) | |||
| @test t4[11:15] == A23567 | |||
|
|
|||
| for U in unboxedunions | |||
| Base.unionlen(U) > 5 && continue # larger values cause subtyping to crash | |||
There was a problem hiding this comment.
Related? If so, how? Does the better inference perform more/other subtype checks that now trigger the crash?
| && !istopfunction(tm, f, :setindex!) | ||
| # the construct-to-convert method is a bottleneck in inference, | ||
| # so just assume that recursion will get prevented at some other point | ||
| && !(method.sig == Tuple{Type, Any})) |
There was a problem hiding this comment.
💯 for being able to remove all those special cases!
|
@nanosoldier |
|
Still seems to be an inference problem on AV. Oh well. |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
vtjnash
force-pushed
the
jn/infer-norecur-more
branch
from
ea5bce5 to
5a4dd99
Compare
vtjnash
changed the title
|
@nanosoldier |
|
A few words on the changes to the type complexity limiting would be appreciated. |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
The main change is just that I relaxed the rules for unbounded recursion detection. For example, the hard cutoff on depth and length is gone. As a trade-off, the handling of recursion now limits more aggressively. Overall, that should mean this is actually friendlier to code that wants to recursively reduce some tuple (as demonstrated by the nanosoldier results), while being faster to handle cases where there is "accidental" recursion (Typical cases were I see this are from depwarn – due to a couple deprecated primitive functions – and the REPL code – due to specializations of refresh_multi_line. Also the inference code – since it is indeed a heavily recursive algorithm.) |
…riggering recursion detection This is generally sufficient to ensure we can infer "bottleneck"-type functions, without significantly sacrificing our convergence requirements
These should now be covered by the improved recursion detector.
remove code to handle exponential blowup, since there isn't any
vtjnash
force-pushed
the
jn/infer-norecur-more
branch
from
5a4dd99 to
b89e88e
Compare
|
All passing finally (except for new flakiness in sprand tests). Will merge tomorrow. |
|
Does this fix #22255 ? |
RefinementReplacement of #21933 recursion detection and handling.The general idea here of the first commit is that it is sufficient to prevent recursion complexity growth over entire edges. Previously, we only considered the callee vertex when detecting recursive method calls. However, it should also be valid to consider both vertexes. But considering the callee also, we can significantly increase the range of cases that we can allow inference to consider, without triggering the recursion detection limiter. This should be generally sufficient to ensure we can infer "bottleneck"-type functions, without significantly sacrificing our convergence requirements.
For an example of when this new heuristic comes into effect, consider
print: a significant number of these methods are reached from calling varargprintand themselves call varargprint.Naively, those recursive vararg
printcalls have no obvious complexity bound on their own, and thus can't be inferred. However, when we add in the consideration of this PR that each one has a different caller method, that doesn’t need to be detected as recursion.Thus,
this may incidentallyshould fix #23371(though my actually reason for this is for some other WIP I have locally).Update:
The remaining commits in the PR make it harder to trigger the unbounded recursion detection, but increase the effective penalty for doing so. Now inference runs in one of two modes: limited or unlimited. This allows us to cache the limited and unlimited inference results separately.