RFC: Widen less aggressively in tmerge #23077

martinholters · 2017-08-01T18:03:53Z

I admit I don't have any practical use cases where this leads to a noticeable improvement in the inference result, but if nothing else, it's at least a net reduction of code. And (at least locally) the test times, which at least in part are quite inference-heavy, don't seem to be adversely affected, so why not...

```julia
julia> typejoin(NTuple{2,Int}, NTuple{3,Int})
Tuple{Int64,Int64,Vararg{Int64,N} where N}

julia> typejoin(NTuple{1,Int}, NTuple{3,Int})
Tuple{Int64,Vararg{Int64,N} where N}

julia> typejoin(NTuple{0,Int}, NTuple{3,Int})
Tuple
```
~~~While with this PR, the last one gives `Tuple{Vararg{Int64,N} where N}`, which is more in line with he others.~~~ EDIT: The `typejoin` change was separated out to #24485.

martinholters · 2017-08-01T18:04:56Z

base/inference.jl

+        u = typejoin(typea, typeb)
+    end
+    if type_too_complex(u, MAX_TYPE_DEPTH)
+        # TODO: is this needed?
        return Any


In what situations is this needed for convergence?

It helps convergence speed (large types that are changing value become very slow to work with very quickly)

Ok, then I'll delete the comment.

Or better still, put what @vtjnash wrote as a comment.

ararslan · 2017-08-01T18:39:48Z

@nanosoldier runbenchmarks(ALL, vs=":master")

vtjnash · 2017-08-01T18:36:46Z

base/inference.jl

-        end
-        if isa(typea, Union) || isa(typeb, Union) || (isa(typea,DataType) && length(typea.parameters)>3) ||
-            (isa(typeb,DataType) && length(typeb.parameters)>3)
-            # widen tuples faster (see #6704), but not too much, to make sure we can infer


I assume removing this doesn't reintroduce #6704? (it's partially handled by #21933 now, but I also don't handle Tuple correctly there – don't widen it fast enough – partially due to assuming that this code here existed)

I assume removing this doesn't reintroduce #6704?

It doesn't, at least not the code as posted there. I didn't try to find variations that might still cause problems, though.

(it's partially handled by #21933 now, but I also don't handle Tuple correctly there – don't widen it fast enough – partially due to assuming that this code here existed)

Ah, I wasn't aware of that.

vtjnash · 2017-08-01T18:40:57Z

base/inference.jl

-        # don't let type unions get too big
-        # TODO: something smarter, like a common supertype
+    if unionlen(u) > MAX_TYPEUNION_LEN
+        u = typejoin(typea, typeb)


perhaps do u = typejoin(u.a, u.b) – it might as well get some benefit out of already having computed the union of a and b

To make this case convergent however, I think typejoin would need to be reordered to handle Union first (even before checking subtyping). Thus, it might be better to do foldl(typejoin, Union{}, uniontypes(u)).

Not sure I completely understand the problem... if typea <: typeb or vice verse, we don't get here. Now if say typea is a Union, surely neither type.a <: typea.b nor vice verse, so in the recursive call of typejoin, the subtype test should always be false. Likewise for typeb, of course. But admittedly, if say typea is a TypeVar and typeb is a Union, things get a little trickier to reason about. So no matter whether it's actually necessary, your proposed version is much easier to reason about :)

Monotonicity here seems like it may depend on typejoin being associative, such that typejoin(A, typejoin(B, C)) == typejoin(typejoin(A, B), C), but I'm not sure that's true. And my version may actually be worse at assuming that. I think we may want typejoin(remove_unions(typea), remove_unions(typeb)), which is what your version does.

Hm, then typea <: typea′ would need to imply remove_unions(typea) <: remove_unions(typea′) (monotonicity) and I had simply assumed remove_unions(u::Union) = typejoin(u.a, u.b) to fulfill that... but it's not that obvious...

I think we can see from the implementation that at least remove_unions(u::Union) is typejoin(u.a, u.b); the case that I'm not sure about is where one of the types isn't a union (remove_unions(x::ANY) = x).

Making typejoin associative would be another option – I think any failure of associativity would be pretty awful. Not sure how to go about assuring that, but it climbs up the type lattice and lattice operations are, by definition, associative, so hopefully that can help.

Correct me if I'm wrong, but ensuring convergence requires monotonicity in the sense that a <: a´ && b <: b´ implies tmerge(a, b) <: tmerge(a´, b´), right? Then indeed we cannot let typejoin do the core work because of e.g. this:

julia> a´ = a = Ref{Int} Ref{Int64} julia> b = Ref{UInt} Ref{UInt64} julia> b´ = Ref{<:Integer} Ref{#s1} where #s1<:Integer julia> typejoin(a, b) Ref julia> typejoin(a, b´) Ref{#s1} where #s1<:Integer julia> a <: a´ && b <: b´ true julia> typejoin(a, b) <: typejoin(a, b´) false

But turns out we're already using typejoin in tmerge for (certain) tuples. And indeed (on master and 0.6):

julia> a´ = a = Tuple{Int32, Ref{Int}} Tuple{Int32,Ref{Int64}} julia> b = Tuple{Int64, Ref{UInt}} Tuple{Int64,Ref{UInt64}} julia> b´ = Tuple{Int64, Ref{<:Integer}} Tuple{Int64,Ref{#s30} where #s30<:Integer} julia> Core.Inference.tmerge(a, b) Tuple{Signed,Ref} julia> Core.Inference.tmerge(a, b´) Tuple{Signed,Ref{#s30} where #s30<:Integer} julia> a <: a´ && b <: b´ true julia> Core.Inference.tmerge(a, b) <: Core.Inference.tmerge(a, b´) false

Unless someone tells me I'm off track here and this is not an issue, my idea would be to first rethink typejoin and depending on the outcome either go ahead with this PR or move in the other direction and completely remove typejoin from tmerge.

No, that cannot be the right interpretation of "monotonicity" in this context, there are too many cases where tmerge violates that. Is it sufficient that the sequence of xs obtained by repeatedly doing x = tmerge(x, ...) is monotonic? Then obviously x >: tmerge(x, ...) suffices, but also using typejoin should be kosher then. @vtjnash, you brought up monotonicity, could you clarify what you meant and what potential problems with typejoin you fear exactly?

vtjnash · 2017-08-01T18:49:33Z

base/inference.jl

+        u = typejoin(typea, typeb)
+    end
+    if type_too_complex(u, MAX_TYPE_DEPTH)
+        # TODO: is this needed?
        return Any


It helps convergence speed (large types that are changing value become very slow to work with very quickly)

nanosoldier · 2017-08-01T21:58:37Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Jutho · 2017-08-14T08:25:37Z

Would these also fix the following issue reported in #22513 ,

tpermute(t::NTuple{N,T}, p::NTuple{N,Int}) where {T,N} = _tpermute((), t, p)
_tpermute(tdst::NTuple{N,T}, tsrc::NTuple{N,T}, p::NTuple{N,Int}) where {T,N} = tdst
_tpermute(tdst::NTuple{M,T}, tsrc::NTuple{N,T}, p::NTuple{N,Int}) where {T,M,N} = _tpermute((tdst...,tsrc[p[M+1]]), tsrc, p)

for whicht tpermute((1,2,3,4,5,6),(3,4,6,1,5,2)) is inferred as Tuple{Vararg{T,6}} where T<:Int64 and lights up in red in @code_warntype (while correctly reporting Tuple{Vararg{Int,6}} on Julia 0.6). If not, I'll file a separate issue for this as this is clearly a bug in inference.

Use `typejoin` for unions growing too large instead of just returning `Any`.

martinholters · 2017-11-09T13:16:14Z

Concerning the validity of the first commit---using typejoin to merge unions instead of widening to Any---we already do use typejoin for Tuples which leads to e.g. this strangeness:

julia> function foo(x)
           if x < 0.1
               y = 0%Int8
           elseif x < 0.2
               y = 0%Int16
           elseif x < 0.3
               y = 0%Int32
           else
               y = 0%Int64
           end
           y
       end
foo (generic function with 1 method)

julia> function bar(x)
           if x < 0.1
               y = (0%Int8,)
           elseif x < 0.2
               y = (0%Int16,)
           elseif x < 0.3
               y = (0%Int32,)
           else
               y = (0%Int64,)
           end
           y[1]
       end
bar (generic function with 1 method)

julia> Core.Inference.return_types(foo, Tuple{Float64})
1-element Array{Any,1}:
 Any

julia> Core.Inference.return_types(bar, Tuple{Float64})
1-element Array{Any,1}:
 Signed

So either the first commit (which lets foo also be inferred as returning a Signed) is valid in that it doesn't risk convergence (only maybe make it a bit slower), or there is a problem is the existing tmerge already. Or am I missing something?

martinholters added the compiler:inference Type inference label Aug 1, 2017

martinholters commented Aug 1, 2017

View reviewed changes

vtjnash reviewed Aug 1, 2017

View reviewed changes

martinholters mentioned this pull request Nov 6, 2017

Improve typejoin involving an empty tuple #24485

Merged

martinholters force-pushed the mh/tmerge branch from dc920bc to 32537f2 Compare November 9, 2017 07:40

Widen unions less agressively during inference

cfd039b

Use `typejoin` for unions growing too large instead of just returning `Any`.

martinholters force-pushed the mh/tmerge branch from 32537f2 to ebc40fd Compare November 9, 2017 08:56

Remove special treatment of Tuples from tmerge

7a26e18

martinholters force-pushed the mh/tmerge branch from ebc40fd to 7a26e18 Compare November 9, 2017 12:26

vtjnash mentioned this pull request Mar 27, 2018

inference: remove MAX_TYPE_DEPTH limit, rewrite tmerge #26628

Merged

vtjnash closed this in #26628 Apr 6, 2018

ararslan deleted the mh/tmerge branch April 6, 2018 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Widen less aggressively in tmerge #23077

RFC: Widen less aggressively in tmerge #23077

martinholters commented Aug 1, 2017 •

edited

Loading

martinholters Aug 1, 2017 •

edited

Loading

vtjnash Aug 1, 2017

martinholters Aug 1, 2017

StefanKarpinski Aug 1, 2017

ararslan commented Aug 1, 2017

vtjnash Aug 1, 2017

martinholters Aug 1, 2017

vtjnash Aug 1, 2017

vtjnash Aug 1, 2017

martinholters Aug 1, 2017

vtjnash Aug 1, 2017

martinholters Aug 1, 2017

vtjnash Aug 1, 2017

StefanKarpinski Aug 1, 2017

martinholters Aug 2, 2017

martinholters Aug 3, 2017

vtjnash Aug 1, 2017

nanosoldier commented Aug 1, 2017

Jutho commented Aug 14, 2017

martinholters commented Nov 9, 2017

RFC: Widen less aggressively in tmerge #23077

RFC: Widen less aggressively in tmerge #23077

Conversation

martinholters commented Aug 1, 2017 • edited Loading

martinholters Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanosoldier commented Aug 1, 2017

Jutho commented Aug 14, 2017

martinholters commented Nov 9, 2017

martinholters commented Aug 1, 2017 •

edited

Loading

martinholters Aug 1, 2017 •

edited

Loading