RFC: Better inference of `_apply()` (splatting) #20343

martinholters · 2017-01-31T16:41:08Z

Presently, something like

x::Union{Tuple{Foo,Bar},Tuple{Bork,Baz}}
f(x...)

will be inferred as f(::Any...), which is likely suboptimal. This PR tries to improve the situation in two ways:
1 If a Union is splatted, inference tries the individual types. In case multiple Unions are splatted, this would result in many possible combinations; I just limit to an arbitrarily chosen maximum of 7. Otherwise...
2. a Union of several Tuples of the same length is rewritten to a Tuple of Unions, e.g. Tuple{Union{Foo, Bork}, Union{Bar, Baz}} for the example above, if it wasn't handled by 1. already.

As an example, I have asserted the return type of setindex(::Tuple, ...) to be a large Union. This allows

julia> @code_warntype slicedim(ones(1,1), 1, 1)
[...]
  end::Union{Array{Float64,1},Array{Float64,2}}

julia> @code_warntype slicedim(ones(1,1,1,1,1,1), 1, 1)
[...]
  end::Union{Array{Float64,5},Array{Float64,6}}

which are the tightest bounds as slicedim is actually not type-stable. On master, the first one gives Union{Array{Float64,1},Array{Float64,2},Float64}, while for more than 2 dimensions it's just Any. (The modification to setindex on master would just always give Any here).

The code certainly needs some revising, but as I'm completely new to inference, I'd like some feedback on the general idea first. (Blame @vtjnash for encouraging me to even try this!)

Edit: This PR has evolved and grown a bit. It now further enhances cases where

a splatted argument of unknown length does not appear last. A call like f(1, ones(n::Int)..., "bar") is now treated like f(::Int, ::Union{Float64,String}...), while on master it is treated like f(::Any...).
a general iterable is passed. The iteration protocol is simulated to determine the element type produced by the iterable, instead of just always treating it as Any.

tkelman · 2017-01-31T20:26:14Z

base/inference.jl

+    tvnew = Vector{Any}[]
+    for tv in tp, t in tvs[1]
+        push!(tvnew, push!(copy(tv), t))
+        if length(tvnew) > 7


should probably make this cutoff a named parameter

Definitely. And I have no clue whether 7 is a reasonable choice.

vtjnash · 2017-02-03T05:19:15Z

This looks like a great start. I'm not quite sure if setindex_t is a good idea, but I think you are definitely on the right track.

martinholters · 2017-02-03T07:10:35Z

This looks like a great start.

Thanks!

I'm not quite sure if setindex_t is a good idea,

Neither am I, but it was the simplest thing to do to test/showcase the improved _apply inference.

but I think you are definitely on the right track.

I'm just refactoring things a bit. Probably it will look something like having a precise_container_type (note the singular) that handles just one of the arguments, and then the loop over the arguments and (if necessary) any Union contents inside abstract_apply, without the need for the extra uniontypes_prod. While doing so, I realized that presently, precise_container_types completely bails out if one the arguments is problematic. E.g. with inference having established x::Int and y::Any, foo(x, y...) would be inferred as foo(::Any...). After the refactoring, it should be easy to instead infer this as foo(:Int, ::Any...). Might there be any reason not to do that?

martinholters · 2017-02-03T13:05:15Z

Might there be any reason not to do that?

Maybe so. Doing only part of what's possible, all tests still pass locally without significant increase in run time. With one exception, that is:

Test (Worker) | Time (s) | GC (s) | GC % | Alloc (MB) | RSS (MB)
subarray (1)  | 4083.07  | 120.84 |  3.0 | 77743.82   | 1621.79

Now this does seem a bit excessive. Let me double-check whether I got something wrong...

martinholters · 2017-02-10T07:04:19Z

Hm, 32bit travis failed on one the new tests. I wonder whether it's build environment or test/worker association dependent. Build log is backed up here, will restart the travis job.

martinholters · 2017-02-10T08:18:35Z

Looks like the failure is platform dependent. Time to set up a 32bit VM. Unless someone has a good idea what might be causing this...

If a Union is splatted, inference tries the individual types, unless this would exceed MAX_APPLY_UNION_ENUM=8 total tries. As a stopgap in that case, a Union of several Tuples of the same length is rewritten to a Tuple of Unions.

martinholters · 2017-02-10T12:47:53Z

Amazing that it took me so long to realize that the Travis 32bit build does not use a 32bit (virtual) machine but just cross-compiles...

Anyway, the bug that caused the failure should be fixed (a missing initialization). Also, I added the specialization for Array to use eltype instead of abstract_iteration.

I'm still unsure about the last commit, which adds the setindex_t function to add a type-assertion to setindex(::Tuple, ...). While its effect for slicedim is quite nice, I wonder whether it's worth it. Should I just remove that commit? Or try whether setindex itself can be rewritten to be more inference-friendly without resorting to this monstrous type-assert? Thoughts?

JeffBezanson · 2017-02-11T21:11:04Z

base/inference.jl

-            end
-        elseif tti <: AbstractArray && i == n
-            result[i] = Any[Vararg{eltype(tti)}]
+# `type` is the inferred type for expression `arg`.


typ instead of type?

Yup, will fix.

JeffBezanson · 2017-02-11T21:31:24Z

This is some great work!

The huge union in setindex_t also makes me a bit nervous. Maybe better to leave it out for now, since it's not needed for the main improvement here.

Seems like this fixes #19957? Could you add a test for that?

martinholters · 2017-02-13T07:44:19Z

Seems like this fixes #19957?

Not completely: it will infer f(1...) like f(::Int...) instead of f(::Any...), but not like f(::Int). Certainly an improvement, but does that count as fixing #19957?

Could you add a test for that?

To the extent it is actually fixed, yes, will do.

Loop over the arguments in `abstract_apply` and invoke the modified/renamed `precise_container_type` for each individual argument. Translate some cases where the container type cannot be inferred precisely into Vararg's (instead of bailing out and inferring `f(::Any...)`) and fuse Vararg's with further arguments into a single trailing Vararg.

Closes #20518 and improves inference when splatting genetal iterables.

martinholters · 2017-02-13T08:33:53Z

Let's see whether this has any effect on the benchmarks:
@nanosoldier runbenchmarks(ALL, vs = ":master")

nanosoldier · 2017-02-13T11:53:45Z

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

martinholters · 2017-02-13T12:14:55Z

Two of the improvements look real, everything else is noise?

tkelman · 2017-02-13T12:17:42Z

the subarray slowdowns look pretty consistent though

KristofferC · 2017-02-13T12:30:34Z

Easiest to run again:

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

martinholters · 2017-02-13T12:31:17Z

It looks quite consistent indeed, as the same operations seem to be benchmarked twice: JuliaCI/BaseBenchmarks.jl@da9fda3#commitcomment-20858355

martinholters · 2017-02-13T12:35:04Z

Oh boy, @code_warntype for the benchmarked perf_lucompletepivCopy! gives a 1000+ lines output. But no type-instabilities there. In fact, no _apply there. So at first glance, no idea where the slowdown might come.

nanosoldier · 2017-02-13T15:49:28Z

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

tkelman · 2017-02-13T15:54:26Z

fair enough then

tkelman reviewed Jan 31, 2017

View reviewed changes

ararslan added the compiler:inference Type inference label Jan 31, 2017

martinholters force-pushed the mh/infer_apply_union branch from e1d95b6 to 30f080b Compare February 1, 2017 13:34

martinholters force-pushed the mh/infer_apply_union branch 4 times, most recently from 0fa6ada to b1e0de8 Compare February 7, 2017 12:28

martinholters mentioned this pull request Feb 9, 2017

Better inference for _apply of iterable #12579

Closed

martinholters force-pushed the mh/infer_apply_union branch 2 times, most recently from 958993d to e33f165 Compare February 9, 2017 17:09

Better inference of _apply(..., Union{Tuple...}, ...)

9b7cf84

If a Union is splatted, inference tries the individual types, unless this would exceed MAX_APPLY_UNION_ENUM=8 total tries. As a stopgap in that case, a Union of several Tuples of the same length is rewritten to a Tuple of Unions.

martinholters force-pushed the mh/infer_apply_union branch from e33f165 to 6e6a29f Compare February 10, 2017 12:33

martinholters changed the title ~~WIP/RFC: Better inference of _apply(..., Union{Tuple...}, ...)~~ WIP/RFC: Better inference of _apply() (splatting) Feb 10, 2017

JeffBezanson reviewed Feb 11, 2017

View reviewed changes

martinholters added 2 commits February 13, 2017 08:46

Simulate iteration protocal when inferring splatted arguments

a434dfc

Closes #20518 and improves inference when splatting genetal iterables.

martinholters force-pushed the mh/infer_apply_union branch from 6e6a29f to a434dfc Compare February 13, 2017 07:55

martinholters changed the title ~~WIP/RFC: Better inference of _apply() (splatting)~~ RFC: Better inference of _apply() (splatting) Feb 13, 2017

JeffBezanson merged commit 3f14659 into master Feb 13, 2017

StefanKarpinski deleted the mh/infer_apply_union branch February 13, 2017 21:00

JeffBezanson mentioned this pull request Mar 24, 2017

improve precompiles #20793

Merged

tkelman mentioned this pull request May 27, 2017

Inference failure when splatting a number #19957

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Better inference of `_apply()` (splatting) #20343

RFC: Better inference of `_apply()` (splatting) #20343

martinholters commented Jan 31, 2017 •

edited

Loading

tkelman Jan 31, 2017

martinholters Jan 31, 2017

vtjnash commented Feb 3, 2017

martinholters commented Feb 3, 2017

martinholters commented Feb 3, 2017

martinholters commented Feb 10, 2017

martinholters commented Feb 10, 2017

martinholters commented Feb 10, 2017

JeffBezanson Feb 11, 2017

martinholters Feb 13, 2017

JeffBezanson commented Feb 11, 2017

martinholters commented Feb 13, 2017 •

edited

Loading

martinholters commented Feb 13, 2017

nanosoldier commented Feb 13, 2017

martinholters commented Feb 13, 2017

tkelman commented Feb 13, 2017

KristofferC commented Feb 13, 2017

martinholters commented Feb 13, 2017 •

edited

Loading

martinholters commented Feb 13, 2017

nanosoldier commented Feb 13, 2017

tkelman commented Feb 13, 2017

RFC: Better inference of _apply() (splatting) #20343

RFC: Better inference of _apply() (splatting) #20343

Conversation

martinholters commented Jan 31, 2017 • edited Loading

tkelman Jan 31, 2017

Choose a reason for hiding this comment

martinholters Jan 31, 2017

Choose a reason for hiding this comment

vtjnash commented Feb 3, 2017

martinholters commented Feb 3, 2017

martinholters commented Feb 3, 2017

martinholters commented Feb 10, 2017

martinholters commented Feb 10, 2017

martinholters commented Feb 10, 2017

JeffBezanson Feb 11, 2017

Choose a reason for hiding this comment

martinholters Feb 13, 2017

Choose a reason for hiding this comment

JeffBezanson commented Feb 11, 2017

martinholters commented Feb 13, 2017 • edited Loading

martinholters commented Feb 13, 2017

nanosoldier commented Feb 13, 2017

martinholters commented Feb 13, 2017

tkelman commented Feb 13, 2017

KristofferC commented Feb 13, 2017

martinholters commented Feb 13, 2017 • edited Loading

martinholters commented Feb 13, 2017

nanosoldier commented Feb 13, 2017

tkelman commented Feb 13, 2017

RFC: Better inference of `_apply()` (splatting) #20343

RFC: Better inference of `_apply()` (splatting) #20343

martinholters commented Jan 31, 2017 •

edited

Loading

martinholters commented Feb 13, 2017 •

edited

Loading

martinholters commented Feb 13, 2017 •

edited

Loading