isapprox() not type-stable if called with keywords

[gwater]

I noticed that isapprox allocates a lot of memory when it is called with a keyword argument, eg.

isapprox(3.0, 0.0, atol=1.0)

Inspection with @code_warntype reveals a type instability. (Note the Any on the final line.)

julia> @code_warntype isapprox(3.0, 0.0, atol=1.0)
Variables:
  #unused#::Base.#kw##isapprox
  #temp#@_2::Array{Any,1}
  ::Base.#isapprox
  x::Float64
  y::Float64
  #temp#@_6::Bool
  atol::Any
  rtol::Real
  #temp#@_9::Int64
  #temp#@_10::Int64
  #temp#@_11::Int64
  #temp#@_12::Any

Body:
  begin 
      NewvarNode(:(rtol::Real))
      #temp#@_6::Bool = true
      atol::Any = 0
      SSAValue(2) = (Base.arraylen)(#temp#@_2::Array{Any,1})::Int64
      SSAValue(3) = (Base.select_value)((Base.sle_int)(0,1)::Bool,(Base.box)(Int64,(Base.ashr_int)(SSAValue(2),(Base.box)(UInt64,1))),(Base.box)(Int64,(Base.shl_int)(SSAValue(2),(Base.box)(UInt64,(Base.box)(Int64,(Base.neg_int)(1))))))::Int64
      SSAValue(7) = (Base.select_value)((Base.sle_int)(1,SSAValue(3))::Bool,SSAValue(3),(Base.box)(Int64,(Base.sub_int)(1,1)))::Int64
      #temp#@_9::Int64 = 1
      8: 
      unless (Base.box)(Base.Bool,(Base.not_int)((#temp#@_9::Int64 === (Base.box)(Int64,(Base.add_int)(SSAValue(7),1)))::Bool)) goto 31
      SSAValue(8) = #temp#@_9::Int64
      SSAValue(9) = (Base.box)(Int64,(Base.add_int)(#temp#@_9::Int64,1))
      #temp#@_10::Int64 = SSAValue(8)
      #temp#@_9::Int64 = SSAValue(9)
      #temp#@_11::Int64 = (Base.box)(Int64,(Base.sub_int)((Base.box)(Int64,(Base.mul_int)(#temp#@_10::Int64,2)),1))
      #temp#@_12::Any = (Core.arrayref)(#temp#@_2::Array{Any,1},#temp#@_11::Int64)::Any
      unless (#temp#@_12::Any === :atol)::Bool goto 19
      atol::Any = (Core.typeassert)((Core.arrayref)(#temp#@_2::Array{Any,1},(Base.box)(Int64,(Base.add_int)(#temp#@_11::Int64,1)))::Any,Base.Real)::Real
      goto 29
      19: 
      unless (#temp#@_12::Any === :rtol)::Bool goto 24
      rtol::Real = (Core.typeassert)((Core.arrayref)(#temp#@_2::Array{Any,1},(Base.box)(Int64,(Base.add_int)(#temp#@_11::Int64,1)))::Any,Base.Real)::Real
      #temp#@_6::Bool = false
      goto 29
      24: 
      SSAValue(10) = ::Base.#isapprox
      SSAValue(11) = x::Float64
      SSAValue(12) = y::Float64
      (Base.throw)($(Expr(:new, :(Base.MethodError), :((Core.getfield)((Core.getfield)((Core.getfield)(Base.#isapprox,:name)::TypeName,:mt),:kwsorter)), :((Core.tuple)(#temp#@_2,SSAValue(10),SSAValue(11),SSAValue(12))::Tuple{Array{Any,1},Base.#isapprox,Float64,Float64}))))::Union{}
      29: 
      goto 8
      31: 
      unless #temp#@_6::Bool goto 38
      # meta: location floatfuncs.jl rtoldefault 260
      SSAValue(6) = (Base.Math.box)(Base.Math.Float64,(Base.Math.sqrt_llvm)($(QuoteNode(2.220446049250313e-16))))::Float64
      SSAValue(5) = (Base.Math.box)(Base.Math.Float64,(Base.Math.sqrt_llvm)($(QuoteNode(2.220446049250313e-16))))::Float64
      # meta: pop location
      rtol::Real = (Base.select_value)((Base.box)(Base.Bool,(Base.or_int)((Base.lt_float)(SSAValue(6),SSAValue(5))::Bool,(Base.box)(Base.Bool,(Base.and_int)((Base.slt_int)((Base.box)(Int64,SSAValue(6)),0)::Bool,(Base.box)(Base.Bool,(Base.not_int)((Base.slt_int)((Base.box)(Int64,SSAValue(5)),0)::Bool)))))),(Base.select_value)((Base.ne_float)(SSAValue(5),SSAValue(5))::Bool,SSAValue(6),SSAValue(5))::Float64,(Base.select_value)((Base.ne_float)(SSAValue(6),SSAValue(6))::Bool,SSAValue(5),SSAValue(6))::Float64)::Float64
      38: 
      return (Base.#isapprox#520)(rtol::Real,atol::Real,::Base.#isapprox,x::Float64,y::Float64)::Any
  end::Any

The definition in base/floatfuncs.jl suggests it only ever returns Bools. Therefore I would expect the return type to be stable.

Tested on:

Julia Version 0.5.2
Commit f4c6c9d (2017-05-06 16:34 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i7-2600 CPU @ 3.40GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Sandybridge)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.7.1 (ORCJIT, sandybridge)

[mauro3]

I'm pretty sure this is expected as keyword arguments are currently not very performant, see e.g. #9551.

Minimal example:

julia> g(x;y::Integer=5) = x+y
g (generic function with 1 method)

julia> @code_warntype g(5)
Variables:
  #self#::#g
  x::Int64

Body:
  begin 
      return (Base.box)(Int64,(Base.add_int)(x::Int64,5))
  end::Int64

julia> @code_warntype g(5,y=5)
Variables:
  #unused#::#kw##g
  #temp#@_2::Array{Any,1}
  ::#g
  x::Int64
  y::Any

...
 return (x::Int64 + y::Integer)::Any
  end::Any

Note though that the function g(x;y::Int=5) = x+y is type stable. However, this g{T<:Integer}(x;y::T=5) = x+y is still unstable. So, probably there is no fix for isapprox for now.

[andyferris]

Moving forward with this - is there currently a plan to solve this?

I remember @carnival had some prototype of a "named tuple" type which was targeted at this... but now I'm wondering why we shouldn't use the lowering phase to perform a trick like in the macros of NamedTuples.jl (or even just build a normal tuple and unpack it into local bindings, again via lowering).

[gwater]

I admit, I don't fully understand what is going on in that issue. But one example in that thread is a keyword-function with proper type inference.

Similarly, the manual suggests keyword-functions can be type stable.

[nalimilan]

Note though that the function g(x;y::Int=5) = x+y is type stable. However, this g{T<:Integer}(x;y::T=5) = x+y is still unstable. So, probably there is no fix for isapprox for now.

@mauro3 I don't think that's correct. The return type of isapprox does not depend on the type of the keyword arguments, so it can be made type-stable.

[giordano]

Would this work?

function isapprox(x::Number, y::Number; rtol::Real=rtoldefault(x,y), atol::Real=0, nans::Bool=false)
    (x == y)::Bool || (isfinite(x) && isfinite(y) && abs(x-y) <= atol + rtol*max(abs(x), abs(y)))::Bool || (nans && isnan(x) && isnan(y))::Bool
end

[mauro3]

@nalimilan, yes that is true but somehow inference still fails. This is the minimal example:

julia> g(x;y::Integer=5) = x>y
g (generic function with 1 method)

julia> @code_warntype g(5,y=5)
...
 end::Any

But isapprox could be fixed by specifying the function return type as ::Bool:

function isapprox(x::Number, y::Number; rtol::Real=rtoldefault(x,y), atol::Real=0)::Bool
           x == y || (isfinite(x) && isfinite(y) && abs(x-y) <= atol + rtol*max(abs(x), abs(y)))
end

(@giordano, you beat me to it.)

[andyferris]

Yes, we can make it type-stable (return value is inferred) but keyword arguments preclude that it is fully type-inferable (internal bindings have inferred types), both of which have a significant impact on speed.

[KristofferC]

julia> @inferred isapprox(3.0, 0.0, atol=1.0)
false