RFC: Less aggressive recursion limiting

[Keno]

Our recusion heuristic works by detection recursion of edges of methods (N.B.: Methods, not specializations). This works reasonably well, but there are some pathological cases that defeat it. One common one is to have a wrapper function that calls an internal function, e.g.

mymap(f, x) = map(f, x)

If a higher order function is written with such a pattern, it is quite easy to run into the recursion limit even in legitimate cases. For example, with the above definition, a fuction like:

f(x) = mymap(x) do t
mymap(sin, t)
end

will fail to get precise inference. There's various other patterns that cause similar issues, e.g. optional arguments and keyword arguments and is one of the more common causes of inference suboptimalities.

This PR attempts to relax this criterion significantly. It is still based on methods, but considers the entire recursion path rather than just a single edge. So for example, in our current heuristic, we would limit:

E -> A -> B -> C -> A -> B

immediately, but with the proposed heuristic we would not limit it until we reach:

E -> A -> B -> C -> A -> B -> C

And in particular, we would not limit

E -> A -> B -> C -> A -> B -> D -> A -> B -> E

even though the A->B edge repeats frequently. This is intentional to allow code that has a central dispatch function (e.g. Diffractor has code patterns like that).

If this turns out to be not aggressive enough, we could consider imposing additional limitations on the intermediate edges, but I think this is worth a try.

[Keno]

Guess I should have said, this does explicitly address the example given in the commit message, though that was just an example, not the primary motivation.
Master:

julia> mymap(f, x) = map(f, x)
mymap (generic function with 1 method)

julia> f(x) = mymap(x) do t
       mymap(sin, t)
       end
f (generic function with 1 method)

julia> (@code_typed f(Vector{Float64}[[1.],[2.],[3.]]))[2]
Vector (alias for Array{_A, 1} where _A)

PR:

julia> (@code_typed f(Vector{Float64}[[1.],[2.],[3.]]))[2]
Vector{Vector{Float64}} (alias for Array{Array{Float64, 1}, 1})

[oscardssmith]

does this notably effect inference speed? It's not obvious to me that this heuristic guarantees termination.

[Keno]

Fixes at least #47694, #40084, #46557, #31315, #29298, but of course the difficulty here is one of line drawing, not just fixing it itself, but hopefully that list will give a few cases to check if we want to restrict this further.

[Keno]

does this notably effect inference speed? It's not obvious to me that this heuristic guarantees termination.

I don't know. It certainly does more inference, in cases that would be limited before, but on the other hand getting limited makes inference much more expensive because it disables caching, so in cases where it actually is able to recover information, it might end up being cheaper.

[Keno]

does this notably effect inference speed? It's not obvious to me that this heuristic guarantees termination.

It guarantees termination at any given recursion depth, but that limit is more of a "technically", because it's strongly exponential, so I didn't bother limiting it. However, the non-terminating case is somewhat pathological, because you need to write code that generates exponential method cycles, which I don't think would happen accidentally.

[LilithHafner]

We can probably delete this hack if we merge this

[Keno]

@vtjnash Can you remind me what we decided to do here?

[vtjnash]

We discussed taking a Set comparison approach, where for each method we add to the call-stack, we maintain an indirect graph of when it most recently appeared on the stack prior to that. At least one method between our current call edge and the previous call edge from the same method must be new to the call stack (to have not appeared on the stack prior to our previous call edge, including in the cycle containing that prior call). Which can be done quickly by walking up the chain of identical method uses on the stack height number of times, and/or by comparing the height counters directly.

We proposed that this is fairly simple to show it should be usually pretty stable and predictable against random changes to the abstract interpretation order within a function, and can easily be shown to be convergent. We will of course need to check our work with PkgEval, to see if further refinement is required however.