Improve precision of abstract_iteration #36524

Keno · 2020-07-03T06:03:36Z

Currently abstract iteration works fine for length-1 iterators, but
fails over for other small length iterators, such as Pair{Int, Int},
or common patterns where iterate iterates over the fields of a
small struct. Other examples include StaticVectors and constant
iterators, which should all now unroll properly up to the
MAX_TUPLE_SPLAT limit. That said, MAX_TUPLE_SPLAT is quite high
at the moment, but because abstract_iteration isn't very precise,
it's unlikely to be limiting. With this increase in precision,
we may find that MAX_TUPLE_SPLAT is too high and we should lower it.

Also note that while this is a nice improvement, performance is still
not great, since we currently can't inline these apply calls. However,
fixing that is in progress in a parallel PR and with both changes
put together, the performance improvement of splatting of small
iterators is quite sizeable.

Keno · 2020-07-03T06:32:48Z

Also note that while this is a nice improvement, performance is still
not great, since we currently can't inline these apply calls. However,
fixing that is in progress in a parallel PR and with both changes
put together, the performance improvement of splatting of small
iterators is quite sizeable.

Some numbers on this:

Before

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

julia> @benchmark f(1=>2)
BenchmarkTools.Trial: 
  memory estimate:  96 bytes
  allocs estimate:  3
  --------------
  minimum time:     351.580 ns (0.00% GC)
  median time:      357.165 ns (0.00% GC)
  mean time:        373.367 ns (0.97% GC)
  maximum time:     9.967 μs (96.17% GC)
  --------------
  samples:          10000
  evals/sample:     212

After:

julia> @benchmark f(1=>2)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     2.026 ns (0.00% GC)
  median time:      2.032 ns (0.00% GC)
  mean time:        2.035 ns (0.00% GC)
  maximum time:     6.816 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

martinholters · 2020-07-03T06:41:35Z

Something I long had on my to do list, but a half-hearted attempt didn't work out, so it stayed there. Glad you tackled it!

I wouldn't worry too much about the high MAX_TUPLE_SPLAT: For those cases where this PR makes a difference, i.e. where iterate returns a const-inferable state, it is also probably quite simple and therefore cheap to infer/const-propagate. Depending on the context, the more precise inference result might make up for the extra work. The worst case is probably where MAX_TUPLE_SPLAT is exceeded: Extra work with little profit. Code splatting lots of large StaticArrays might take a hit? A middle ground might be to have a second, lower cut-off and start widenconsting (i.e. like before this PR) once that is exceeded. But let's first see whether this causes any relevant latency issues as is.

base/compiler/abstractinterpretation.jl

martinholters

I have a proposal for fixing the (1:33...,) case which also fixes the abstractarray test, see suggestions.

base/compiler/abstractinterpretation.jl

martinholters · 2020-07-03T09:47:05Z

I've noticed something else funny going on here: After include("compiler/compiler.jl"), displaying the result of splatting into tuple produces

Internal error: encountered unexpected error in runtime:
TypeError(func=:Tuple, context="parameter", expected=Type, got=Core.Compiler.Const(val=2, actual=false))
jl_type_error_rt at ../src/rtutils.c:118
jl_f_apply_type at ../src/builtins.c:962
jl_apply at ../src/julia.h:1712 [inlined]
do_apply at ../src/builtins.c:655
argtypes_to_type at ./compiler/typeutils.jl:48
abstract_call_known at ./compiler/abstractinterpretation.jl:931
abstract_call at ./compiler/abstractinterpretation.jl:955
abstract_call at ./compiler/abstractinterpretation.jl:939
abstract_eval_statement at ./compiler/abstractinterpretation.jl:1061
typeinf_local at ./compiler/abstractinterpretation.jl:1300
typeinf_nocycle at ./compiler/abstractinterpretation.jl:1371
typeinf at ./compiler/typeinfer.jl:12
abstract_call_method_with_const_args at ./compiler/abstractinterpretation.jl:289
unknown function (ip: 0x7f6dcb9b1af7)
abstract_call_gf_by_type at ./compiler/abstractinterpretation.jl:139
abstract_call_known at ./compiler/abstractinterpretation.jl:932
abstract_call at ./compiler/abstractinterpretation.jl:955
[...]
jl_compile_method_internal at ../src/gf.c:2074
jl_compile_method_internal at ../src/gf.c:2037 [inlined]
_jl_invoke at ../src/gf.c:2344 [inlined]
jl_apply_generic at ../src/gf.c:2535
display at ../stdlib/v1.6/REPL/src/REPL.jl:218
display at ../stdlib/v1.6/REPL/src/REPL.jl:232
display at ./multimedia.jl:328

E.g.

julia> f() = (()...,)
f (generic function with 1 method)

julia> f();

julia> f()
Internal error: encountered unexpected error in runtime:
TypeError(func=:Tuple, context="parameter", expected=Type, got=Core.Compiler.Const(val=2, actual=false))
[...]
()

julia> @code_typed f() # also here
CodeInfo(
1 ─     return Internal error: encountered unexpected error in runtime:
TypeError(func=:Tuple, context="parameter", expected=Type, got=Core.Compiler.Const(val=2, actual=false))
[...]
()
) => Tuple{}

The argtypes given to argtypes_to_type in these cases are

Array{Any, (3,)}[
  Core.Compiler.Const(val=typeof(Base.iterate)(), actual=false),
  Core.Compiler.Const(val=("#= circular reference @-", 1, " =#"), actual=false),
  Core.Compiler.Const(val=2, actual=false)]

and get widened to

Array{Any, (3,)}[
  typeof(Base.iterate),
  Tuple{String, Int64, String},
  Core.Compiler.Const(val=2, actual=false)]

No idea what to make of this...

EDIT: This doesn't depend on how the value to show produced, just what is show, e.g.

julia> ()
TypeError(func=:Tuple, context="parameter", expected=Type, got=Core.Compiler.Const(val=2, actual=false))
[...]
()

martinholters · 2020-07-03T11:11:07Z

Ah, maybe the Core.Compiler.Const(val=2, actual=false) is a result cached from before reloading compiler.jl and hence is not recognized by widenconst? And this is to be expected and include("compiler/compiler.jl") is just a don't-do-that? If so sorry for the noise.

Keno · 2020-07-03T20:02:43Z

After include("compiler/compiler.jl"), displaying the result of splatting into tuple produces

I can't reproduce this. I usually just use using Revise; Revise.track(Core.Compiler) which works fine most of the time.

Keno · 2020-07-03T20:05:13Z

Anyway, I rotated the second loop, which I think should address the bug you found.

martinholters

LGTM. Should we add an explicit test for (1:N...,) with N > MAX_TUPLE_SPLAT?

Keno · 2020-07-08T00:42:17Z

I've noticed something else funny going on here: After include("compiler/compiler.jl"), displaying the result of splatting into tuple produces

This will be fixed by #36570

Currently abstract iteration works fine for length-1 iterators, but fails over for other small length iterators, such as Pair{Int, Int}, or common patterns where `iterate` iterates over the fields of a small struct. Other examples include StaticVectors and constant iterators, which should all now unroll properly up to the MAX_TUPLE_SPLAT limit. That said, MAX_TUPLE_SPLAT is quite high at the moment, but because abstract_iteration isn't very precise, it's unlikely to be limiting. With this increase in precision, we may find that MAX_TUPLE_SPLAT is too high and we should lower it. Also note that while this is a nice improvement, performance is still not great, since we currently can't inline these apply calls. However, fixing that is in progress in a parallel PR and with both changes put together, the performance improvement of splatting of small iterators is quite sizeable.

Keno requested review from JeffBezanson, martinholters and vtjnash July 3, 2020 06:03

Keno force-pushed the kf/abstract_iterate_precision branch from a8b1eb5 to e272548 Compare July 3, 2020 06:14

Keno added the compiler:inference Type inference label Jul 3, 2020

martinholters reviewed Jul 3, 2020

View reviewed changes

base/compiler/abstractinterpretation.jl Show resolved Hide resolved

martinholters requested changes Jul 3, 2020

View reviewed changes

base/compiler/abstractinterpretation.jl Show resolved Hide resolved

base/compiler/abstractinterpretation.jl Outdated Show resolved Hide resolved

Keno force-pushed the kf/abstract_iterate_precision branch from e272548 to a1bdc2f Compare July 3, 2020 20:01

Keno requested a review from martinholters July 3, 2020 20:05

martinholters approved these changes Jul 6, 2020

View reviewed changes

vtjnash approved these changes Jul 7, 2020

View reviewed changes

Keno force-pushed the kf/partialedges branch from 408289a to a2de044 Compare July 8, 2020 21:16

Base automatically changed from kf/partialedges to master July 9, 2020 00:53

Keno force-pushed the kf/abstract_iterate_precision branch from a1bdc2f to 3531e2a Compare July 9, 2020 07:25

Keno merged commit bee6546 into master Jul 9, 2020

Keno deleted the kf/abstract_iterate_precision branch July 9, 2020 18:19

JeffBezanson mentioned this pull request Dec 21, 2020

case where immutable struct is 30% slower than mutable (and much slower on 1.6-dev) #38936

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Improve precision of abstract_iteration #36524

Improve precision of abstract_iteration #36524

Uh oh!

Keno commented Jul 3, 2020

Uh oh!

Keno commented Jul 3, 2020

Uh oh!

martinholters commented Jul 3, 2020

Uh oh!

Uh oh!

martinholters left a comment

Uh oh!

Uh oh!

Uh oh!

martinholters commented Jul 3, 2020 •

edited

Loading

Uh oh!

martinholters commented Jul 3, 2020

Uh oh!

Keno commented Jul 3, 2020

Uh oh!

Keno commented Jul 3, 2020

Uh oh!

martinholters left a comment

Uh oh!

Keno commented Jul 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Improve precision of abstract_iteration #36524

Improve precision of abstract_iteration #36524

Uh oh!

Conversation

Keno commented Jul 3, 2020

Uh oh!

Keno commented Jul 3, 2020

Uh oh!

martinholters commented Jul 3, 2020

Uh oh!

Uh oh!

martinholters left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

martinholters commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinholters commented Jul 3, 2020

Uh oh!

Keno commented Jul 3, 2020

Uh oh!

Keno commented Jul 3, 2020

Uh oh!

martinholters left a comment

Choose a reason for hiding this comment

Uh oh!

Keno commented Jul 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

martinholters commented Jul 3, 2020 •

edited

Loading