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.

Sign up for GitHub

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

Jump to bottom

ensure promotion rules don't alter eltype values #26109

Merged
merged 1 commit into from
Feb 27, 2018
Merged

ensure promotion rules don't alter eltype values #26109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions base/namedtuple.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,6 @@ indexed_next(t::NamedTuple, i::Int, state) = (getfield(t, i), i+1)
isempty(::NamedTuple{()}) = true
isempty(::NamedTuple) = false

promote_typejoin(::Type{NamedTuple{n, S}}, ::Type{NamedTuple{n, T}}) where {n, S, T} =
NamedTuple{n, promote_typejoin(S, T)}

convert(::Type{NamedTuple{names,T}}, nt::NamedTuple{names,T}) where {names,T<:Tuple} = nt
convert(::Type{NamedTuple{names}}, nt::NamedTuple{names}) where {names} = nt

Expand Down
44 changes: 21 additions & 23 deletions base/promotion.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,59 +12,58 @@ they both inherit.
typejoin() = (@_pure_meta; Bottom)
typejoin(@nospecialize(t)) = (@_pure_meta; t)
typejoin(@nospecialize(t), ts...) = (@_pure_meta; typejoin(t, typejoin(ts...)))
typejoin(@nospecialize(a), @nospecialize(b)) = (@_pure_meta; join_types(a, b, typejoin))

function join_types(@nospecialize(a), @nospecialize(b), f::Function)
function typejoin(@nospecialize(a), @nospecialize(b))
@_pure_meta
if a <: b
return b
elseif b <: a
return a
elseif isa(a,UnionAll)
return UnionAll(a.var, join_types(a.body, b, f))
return UnionAll(a.var, typejoin(a.body, b))
elseif isa(b,UnionAll)
return UnionAll(b.var, join_types(a, b.body, f))
return UnionAll(b.var, typejoin(a, b.body))
elseif isa(a,TypeVar)
return f(a.ub, b)
return typejoin(a.ub, b)
elseif isa(b,TypeVar)
return f(a, b.ub)
return typejoin(a, b.ub)
elseif isa(a,Union)
a′ = f(a.a,a.b)
return a′ === a ? typejoin(a, b) : f(a′, b)
a′ = typejoin(a.a, a.b)
return a′ === a ? typejoin(a, b) : typejoin(a′, b)
elseif isa(b,Union)
b′ = f(b.a,b.b)
return b′ === b ? typejoin(a, b) : f(a, b′)
b′ = typejoin(b.a, b.b)
return b′ === b ? typejoin(a, b) : typejoin(a, b′)
elseif a <: Tuple
if !(b <: Tuple)
return Any
end
ap, bp = a.parameters, b.parameters
lar = length(ap)::Int; lbr = length(bp)::Int
if lar == 0
return Tuple{Vararg{tailjoin(bp,1,f)}}
return Tuple{Vararg{tailjoin(bp, 1)}}
end
if lbr == 0
return Tuple{Vararg{tailjoin(ap,1,f)}}
return Tuple{Vararg{tailjoin(ap, 1)}}
end
laf, afixed = full_va_len(ap)
lbf, bfixed = full_va_len(bp)
if laf < lbf
if isvarargtype(ap[lar]) && !afixed
c = Vector{Any}(uninitialized, laf)
c[laf] = Vararg{f(unwrapva(ap[lar]), tailjoin(bp,laf,f))}
c[laf] = Vararg{typejoin(unwrapva(ap[lar]), tailjoin(bp, laf))}
n = laf-1
else
c = Vector{Any}(uninitialized, laf+1)
c[laf+1] = Vararg{tailjoin(bp,laf+1,f)}
c[laf+1] = Vararg{tailjoin(bp, laf+1)}
n = laf
end
elseif lbf < laf
if isvarargtype(bp[lbr]) && !bfixed
c = Vector{Any}(uninitialized, lbf)
c[lbf] = Vararg{f(unwrapva(bp[lbr]), tailjoin(ap,lbf,f))}
c[lbf] = Vararg{typejoin(unwrapva(bp[lbr]), tailjoin(ap, lbf))}
n = lbf-1
else
c = Vector{Any}(uninitialized, lbf+1)
c[lbf+1] = Vararg{tailjoin(ap,lbf+1,f)}
c[lbf+1] = Vararg{tailjoin(ap, lbf+1)}
n = lbf
end
else
Expand All @@ -73,7 +72,7 @@ function join_types(@nospecialize(a), @nospecialize(b), f::Function)
end
for i = 1:n
ai = ap[min(i,lar)]; bi = bp[min(i,lbr)]
ci = f(unwrapva(ai),unwrapva(bi))
ci = typejoin(unwrapva(ai), unwrapva(bi))
c[i] = i == length(c) && (isvarargtype(ai) || isvarargtype(bi)) ? Vararg{ci} : ci
end
return Tuple{c...}
Expand Down Expand Up @@ -114,8 +113,7 @@ Compute a type that contains both `T` and `S`, which could be
either a parent of both types, or a `Union` if appropriate.
Falls back to [`typejoin`](@ref).
"""
promote_typejoin(@nospecialize(a), @nospecialize(b)) =
(@_pure_meta; join_types(a, b, promote_typejoin))
promote_typejoin(@nospecialize(a), @nospecialize(b)) = typejoin(a, b)
promote_typejoin(::Type{Nothing}, ::Type{T}) where {T} =
isconcretetype(T) || T === Union{} ? Union{T, Nothing} : Any
promote_typejoin(::Type{T}, ::Type{Nothing}) where {T} =
Expand Down Expand Up @@ -143,14 +141,14 @@ function full_va_len(p)
return length(p)::Int, true
end

# reduce join_types over A[i:end]
function tailjoin(A, i, f::Function)
# reduce typejoin over A[i:end]
function tailjoin(A, i)
if i > length(A)
return unwrapva(A[end])
end
t = Bottom
for j = i:length(A)
t = f(t, unwrapva(A[j]))
t = typejoin(t, unwrapva(A[j]))
end
return t
end
Expand Down
4 changes: 2 additions & 2 deletions test/namedtuple.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,9 +226,9 @@ abstr_nt_22194_3()
for T in (Nothing, Missing)
x = [(a=1, b=T()), (a=1, b=2)]
y = map(v -> (a=v.a, b=v.b), [(a=1, b=T()), (a=1, b=2)])
@test y isa Vector{NamedTuple{(:a,:b),Tuple{Int,Union{T,Int}}}}
@test y isa Vector{NamedTuple{(:a,:b), T} where T<:Tuple}
@test isequal(x, y)
end
y = map(v -> (a=v.a, b=v.a + v.b), [(a=1, b=missing), (a=1, b=2)])
@test y isa Vector{NamedTuple{(:a,:b),Tuple{Int,Union{Missing,Int}}}}
@test y isa Vector{NamedTuple{(:a,:b), T} where T<:Tuple}
@test isequal(y, [(a=1, b=missing), (a=1, b=3)])
6 changes: 3 additions & 3 deletions test/tuple.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,11 +189,11 @@ end
for T in (Nothing, Missing)
x = [(1, T()), (1, 2)]
y = map(v -> (v[1], v[2]), [(1, T()), (1, 2)])
@test y isa Vector{Tuple{Int,Union{T,Int}}}
@test y isa Vector{Tuple{Int, Any}}
Copy link
Member

Choose a reason for hiding this comment

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

As I showed at #25924, Tuple{Union{T,Int}} is currently much faster than Tuple{Any}, so AFAICT this would be a clear regression.

I also think we should consider these questions in the light of broader issue of inference and NamedTuple at #25925.

Copy link
Sponsor Member Author

@vtjnash vtjnash Feb 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like this is simply a very minor known optimization miss (specifically, that we don't yet implement the obvious optimization to split x isa Union{Int, Missing} to x isa Int || x isa Missing), so we spend almost all of the time running subtyping. This is quite trivial – and non-breaking – to fix, so we haven't worked on it yet. We can verify that this PR doesn't actually affect performance by forcing away the handling of missing at the end (and also reducing the expense of the kernel operation from sin to *):

getx(x::Missing) = 0
getx(x) = x
f(x) = begin
              @time [ getx(2*(x[1])) for x in x ]
              @time Any[ getx(2*(x[1])) for x in x ]
              @time map(x -> getx(2*(x[1])), x)
               nothing
end; f(x); f(x);

Note that the data vector used to benchmark in #25924 isn't quite correct for general comparisons. I'm using x = map(i -> (isodd(i) ? missing : 12345,), 1:10_000); which ensures I have the same percent missingness and avoids the small-integer cache.

By contrast, I don't really know that the issue at #25925 has a good solution. It's possible just to widen incrementally – which will give fairly decent performance on micro-benchmarks – but will be impossible to precompile effectively, so load times might always be very long.

Addendum: if you actually do care about performance, doing this column-wise (e.g. as a Tuple{Vector...}) would give me a significant (3-10x) speedup*.

f(x) = begin
              @time [ getx(2*(x)) for x in x ]
              @time Any[ getx(2*(x)) for x in x ]
              @time map(x -> getx(2*(x)), x)
              nothing
end; f(xx);
xx = map(x -> x[1], x);

* this is failing to inline, which will also prevent vectorization, and is known performance issue #23338 – it should be even faster yet

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is simply a very minor known optimization miss (specifically, that we don't yet implement the obvious optimization to split x isa Union{Int, Missing} to x isa Int || x isa Missing), so we spend almost all of the time running subtyping. This is quite trivial – and non-breaking – to fix, so we haven't worked on it yet. We can verify that this PR doesn't actually affect performance by forcing away the handling of missing at the end (and also reducing the expense of the kernel operation from sin to *):

Interesting. But how is the compiler supposed to detect that it can do x isa Int || x isa Missing given that the only thing it knows about the input is that it's a Tuple{Any}? I guess in your example it's possible since getx has only two methods, but what happens for e.g. sin or +?

Also the return type of map is only inferred as ::AbstractArray when the input is a Vector{Tuple{Any}}, which can be very problematic down the line.

Note that the data vector used to benchmark in #25924 isn't quite correct for general comparisons. I'm using x = map(i -> (isodd(i) ? missing : 12345,), 1:10_000); which ensures I have the same percent missingness and avoids the small-integer cache.

OK. I'd rather use something like map(i -> (rand(Bool) ? missing : i,), 1:10_000) though, to make sure the repetition of (missing, 12345) doesn't allow the CPU to predict the result.

By contrast, I don't really know that the issue at #25925 has a good solution. It's possible just to widen incrementally – which will give fairly decent performance on micro-benchmarks – but will be impossible to precompile effectively, so load times might always be very long.

That's precisely why I propose using inference rather than widening incrementally. But can we have that discussion there instead?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

given that the only thing it knows about the input

The input type doesn't actually play a role here, it's the output type that is failing to codegen. There's an open PR about this, although I can't find it right now.

which can be very problematic down the line

As long as there's a function call boundary (or we finally implement loop out-lining), this has no performance significance

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

using inference

Many inference optimizations currently rely on being able to compute the expanded form of the input unions, so I don't know that will make much of a difference.

Copy link
Member

Choose a reason for hiding this comment

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

The input type doesn't actually play a role here, it's the output type that is failing to codegen. There's an open PR about this, although I can't find it right now.

But how can the compiler infer the output type given that the input is Tuple{Any}?

As long as there's a function call boundary (or we finally implement loop out-lining), this has no performance significance

Well, yeah, but by this line of reasoning we wouldn't care about type stability of any functions, would we?

Many inference optimizations currently rely on being able to compute the expanded form of the input unions, so I don't know that will make much of a difference.

Sorry, but I'm not sure what this means. Could you try to explain this for mere mortals like me? Also this PR affects the public API, while inference could be improved without breaking the API in 1.x releases. So I think the question is more: what public API will allow for the best optimizations in the future?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

But how can the compiler infer the output type given that the input is Tuple{Any}?

It doesn't need to – we don't lose any performance from this. All that matters is that the output type is something easy to optimize (like a concrete type or a trivial union).

we wouldn't care about type stability of any functions

Well, actually, no. It's critical to be able to write kernels with "type stability" so that for-loops over Array are optimized. That translates to requiring that most Base code be coded very carefully. But it doesn't actually carry over to mattering much for a significant portion of stdlib and beyond.

while inference could be improved

There are limits to what will be possible. We optimize unions by computing the fully expanded version. In general, this has O(2^n) components, so that will never help here – it's really just present to handle simple cases like iteration (and even there, it's not simple, ref redesign in #26079 needed to handle just this low order case). The best optimizations in the future will be for code kernels that operate directly on columns of Array{T} or Array{Union{T, Nothing}} – which also happens to be the fastest way to do it now (cf. above addendum).

@test isequal(x, y)
end
y = map(v -> (v[1], v[1] + v[2]), [(1, missing), (1, 2)])
@test y isa Vector{Tuple{Int,Union{Missing,Int}}}
@test y isa Vector{Tuple{Int, Any}}
@test isequal(y, [(1, missing), (1, 3)])
end

Expand Down Expand Up @@ -425,4 +425,4 @@ end
@test findprev(equalto(1), (1, 1), 1) == 1
@test findnext(equalto(1), (2, 3), 1) === nothing
@test findprev(equalto(1), (2, 3), 2) === nothing
end
end