Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inference issue with collect() on generators with filter #25433

Closed
nalimilan opened this issue Jan 6, 2018 · 0 comments
Closed

Inference issue with collect() on generators with filter #25433

nalimilan opened this issue Jan 6, 2018 · 0 comments
Assignees

Comments

@nalimilan
Copy link
Member

nalimilan commented Jan 6, 2018

I've noticed this when changing find to use generators at #24774. It's likely to have a large performance impact.

julia> @code_warntype collect(v for v in [1] if v > 0)
[...]
  end::Union{Array{Int64,1}, Array{Union{},1}}

This type instability also affects grow_to!. I first thought it could be due to the type-instability of start(::Filter), here #temp#@_9::Union{Tuple{Bool}, Tuple{Bool,Int64,Int64}}, but artificially fixing it by making Base.Iterators.start_filter return (true, 1, 2) as the fallback does not make the problem disappear.

julia> @code_warntype Base.grow_to!(Vector{Int}(), (v for v in [1] if v > 0))
Variables:
  dest::Array{Int64,1}
  itr::Base.Generator{Base.Iterators.Filter{getfield(, Symbol("##104#106")),Array{Int64,1}},getfield(, Symbol("##103#105"))}
  out::Any
  s::Int64
  v<optimized out>
  t<optimized out>
  #temp#@_9::Union{Tuple{Bool}, Tuple{Bool,Int64,Int64}}
  #temp#@_10::Any
  #temp#@_11::Bool
[...]
      #= line 397 =#
      #temp#@_9::Union{Tuple{Bool}, Tuple{Bool,Int64,Int64}} = (true,)
      59:
      # meta: pop location
      SSAValue(17) = #temp#@_9::Union{Tuple{Bool}, Tuple{Bool,Int64,Int64}}
      # meta: pop locations (2)
      SSAValue(50) = (SSAValue(17) isa Tuple{Bool})::Bool
      unless SSAValue(50) goto 67
      #temp#@_10::Any = $(Expr(:invoke, MethodInstance for grow_to!(::Array{Union{},1}, ::Base.Generator{Base.Iterators.Filter{getfield(, Symbol("##104#106")),Array{Int64,1}},getfield(, Symbol("##103#105"))}, ::Tuple{Bool}), :(Base.grow_to!), SSAValue(11), :(itr), SSAValue(17)))::Union{Array{Int64,1}, Array{Union{},1}}
      goto 76
      67:
      SSAValue(51) = (SSAValue(17) isa Tuple{Bool,Int64,Int64})::Bool
      unless SSAValue(51) goto 72
      #temp#@_10::Any = $(Expr(:invoke, MethodInstance for grow_to!(::Array{Union{},1}, ::Base.Generator{Base.Iterators.Filter{getfield(, Symbol("##104#106")),Array{Int64,1}},getfield(, Symbol("##103#105"))}, ::Tuple{Bool,Int64,Int64}), :(Base.grow_to!), SSAValue(11), :(itr), SSAValue(17)))::Union{Array{Int64,1}, Array{Union{},1}}
[...]
      return dest::Array{Int64,1}
      96:
      return out::Any
  end::Union{Array{Int64,1}, Array{Union{},1}}

julia> @code_warntype Base.grow_to!(Vector{Int}(), (v for v in [1] if v > 0), (false, 1, 2))
[...]
  end::Array{Int64,1}

julia> @code_warntype Base.grow_to!(Vector{Int}(), (v for v in [1] if v > 0), (false,))
[...]
  end::Array{Int64,1}

So the explanation might be that the function could theoretically return new rather than dest, and that new could have its type change in the loop. This might confuse inference when grow_to!(dest, itr) is inlined in grow_to!(dest, itr). I've tried using @noinline, but it doesn't appear to have an effect here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants