Add highlighting to code_warntype for non-leaf types (such as ::Type)

[mbauman]

In #10961, @swt30 found and fixed a performance problem with collect. It was type-unstable because it had been defined as collect(T::Type, itr) instead of collect{T}(::Type{T}, itr). But @code_warntype gave it a pass:

julia> @code_warntype collect([1,2,3])
Variables:
  itr::Array{Int64,1}

Body:
  begin  # array.jl, line 273:
      return collect(Int64,itr::Array{Int64,1})::Array{Int64,1}
  end::Array{Int64,1}

julia> @code_warntype collect(Int, [1,2,3])
  T::Type{Int64}
  itr::Array{Int64,1}
  i::Int64
  a::Array{Int64,1}
… # Long, but no red flags

It's particularly tricky in this case because inference gets it right outside the function, and then when you look inside with @code_warntype the macro automatically considers the type of its type arguments to be Type{T} and not DataType. Often this is what you want (in fact, I did this in one of my first PRs), but it isn't correct in this case.

This is what we should have seen:

julia> code_warntype(collect, Tuple{DataType, Array{Int,1}})
Variables:
  T::DataType
  itr::Array{Int64,1}
  i::Int64
  a::Array{T,N}
...

I'm not sure how to fix this — we want the Type{T} definition while looking up the method, but then we don't want to pass inference more information than it'll normally get when running the code.

[JeffBezanson]

IIUC, this is a case of specializing for a wider type (DataType instead of Type{Int})? I consider that a different issue than type stability.

[mbauman]

Ah, you're probably right. My language isn't quite right. It's not a type-instability, but it is resulting in a non-leaf type, which ends up allocating and is a non-obvious performance trap.

[pao]

Let's turn this into an enhancement request.

[mbauman]

My intention exactly, @pao. Thanks.

I know that inference is returning the correct result for the arguments (technically), but I'd like this sort of thing to be easier to spot. I looked into it myself hoping to figure out some solution, but my knowledge of the internals isn't strong enough.

Here's a roadmap: the introspection macros all consider the type of a type T to be Type{T}. I think this is what they have to do in order to find the correct method. What I think could change is that the code_typed function could check to see if the methods arguments are parameterized or not, and widen those unparameterized types to DataType.

[tkelman]

not release-blocking

[mbauman]

Example from #20217:

julia> @noinline f(T::DataType) = Array{T}(0);

julia> @code_warntype f(Int)
Variables:
  #self#::#f
  T::Type{Int64}

Body:
  begin
      return $(Expr(:foreigncall, :(:jl_alloc_array_1d), Array{Int64,1}, svec(Any,Int64), Array{Int64,1}, 0, 0, 0))
  end::Array{Int64,1}

julia> code_warntype(f, Tuple{DataType}) # What actually happens
Variables:
  #self#::#f
  T::DataType

Body:
  begin
      return ((Core.apply_type)(Main.Array,T::DataType)::Type{_} where _<:Array)(0)::Any
  end::Any

[vtjnash]

What I think could change is that the code_typed function could check to see if the methods arguments are parameterized or not, and widen those unparameterized types to DataType

There's now a predicate jl_is_cacheable_sig which we could call which would tell you whether this is a signature that may get run. This function is currently showing you exactly what inference will compute for this method call, that's just not the same as what the runtime cache will select to run the method. It might be useful to add a warning to code_warntype to let you know if the signature is unlikely to be used at runtime.

For the @code_warntype macro, it makes sense to me that we should munge the types through jl_cacheable_sig such that the aforementioned additional warning would be avoided.

[mbauman]

The more recent #32834 is a duplicate of this, but it seems to have a fresher and more comprehensive discussion so I'll close this one in favor of it.