sisl/GaussianFilters.jl

Type instability from keyword argument

Closed this issue · 2 comments

[Suggestion (3) in series started in #14]

I noticed here:

return return_prediction ? (bp, bn) : bn
that you return different types based on the value of an argument.

This results in type instability which can have significant performance effects. In the example below, the performance is reduced by a factor of 100 because of this.

(when there is no keyword present in the call, the compiler can deduce the return type, but if ether b=true or b=false is present, it can't find the exact type; it knows only that it is a Union{Int64, Tuple{Int64,Int64}})

julia> function f(a; b = false)
           if b
               return a^2, a^3
           else
               return a^2
           end
       end
f (generic function with 1 method)

julia> using BenchmarkTools

julia> @benchmark f(2, b=false)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     2.095 ns (0.00% GC)
  median time:      2.264 ns (0.00% GC)
  mean time:        2.313 ns (0.00% GC)
  maximum time:     16.213 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

julia> @benchmark f(2, b=$false)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     2.170 ns (0.00% GC)
  median time:      2.536 ns (0.00% GC)
  mean time:        2.501 ns (0.00% GC)
  maximum time:     474.652 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

julia> @benchmark f(2)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     0.029 ns (0.00% GC)
  median time:      0.032 ns (0.00% GC)
  mean time:        0.034 ns (0.00% GC)
  maximum time:     13.271 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

julia> @code_warntype f(2)
Variables
  #self#::Core.Compiler.Const(f, false)
  a::Int64

Body::Int64
1 ─ %1 = Main.:(#f#3)(false, #self#, a)::Int64
└──      return %1

julia> @code_warntype f(2, b=false)
Variables
  #unused#::Core.Compiler.Const(getfield(Main, Symbol("#kw##f"))(), false)
  @_2::NamedTuple{(:b,),Tuple{Bool}}
  @_3::Core.Compiler.Const(f, false)
  a::Int64
  b::Bool
  @_6::Bool

Body::Union{Int64, Tuple{Int64,Int64}}
1 ─ %1  = Base.haskey(@_2, :b)::Core.Compiler.Const(true, false)
│         %1
│         (@_6 = Base.getindex(@_2, :b))
└──       goto #3
2 ─       Core.Compiler.Const(:(@_6 = false), false)
3 ┄       (b = @_6)
│   %7  = (:b,)::Core.Compiler.Const((:b,), false)
│   %8  = Core.apply_type(Core.NamedTuple, %7)::Core.Compiler.Const(NamedTuple{(:b,),T} where T<:Tuple, false)
│   %9  = Base.structdiff(@_2, %8)::Core.Compiler.Const(NamedTuple(), false)
│   %10 = Base.pairs(%9)::Core.Compiler.Const(Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}(), false)
│   %11 = Base.isempty(%10)::Core.Compiler.Const(true, false)
│         %11
└──       goto #5
4 ─       Core.Compiler.Const(:(Base.kwerr(@_2, @_3, a)), false)
5 ┄ %15 = Main.:(#f#3)(b, @_3, a)::Union{Int64, Tuple{Int64,Int64}}
└──       return %15

This instance of type instability is fixed in d6a5112. I think it makes sense for update to only return the updated belief. If a user wants a predicted belief, they can run predict separately.

If a user wants a predicted belief, they can run predict separately.

agreed - I think this is the better way to organize it