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

fix #32699, more accurate apply_type_tfunc for NamedTuple #36366

Merged
merged 1 commit into from
Jul 13, 2020

Conversation

JeffBezanson
Copy link
Member

This can make inference a bit slower, so I tried limiting it to NamedTuple types with known names, to mitigate the effects a bit.

@quinnj @timholy See how this works for you.

fixes #32699

@JeffBezanson JeffBezanson added the compiler:inference Type inference label Jun 19, 2020
@quinnj
Copy link
Member

quinnj commented Jun 19, 2020

image

@timholy
Copy link
Member

timholy commented Jun 19, 2020

The inference looks great! In #36322 I failed to record which package's invalidations I was looking at. I think it was FilePathsBase, and I haven't seen load_direct_deps pop up, so I think it's likely this fixes it. Thanks!

@quinnj
Copy link
Member

quinnj commented Jun 20, 2020

Yep, the inference is looking super spiffy, though it seems the last part of my work-flow is failing to inline (Tables.eachcolumns):

%33 = Core.tuple(%20, %32)::Tuple{Int64,Union{Missing, Float64}}%34 = (NamedTuple{(:x, :y),T} where T<:Tuple)(%33)::NamedTuple{(:x, :y),_A} where _A<:Tuple{Int64,Union{Missing, Float64}}
└───       goto #14
14 ─       goto #15
15%37 = φ (#7 => true, #14 => false)::Bool%38 = φ (#14 => %34)::NamedTuple{(:x, :y),_A} where _A<:Tuple{Int64,Union{Missing, Float64}}%39 = φ (#14 => %16)::Int64
└───       goto #17 if not %37
16 ─       goto #20
17%42 = Base.add_int(%3, 1)::Int64
│          Tables.eachcolumns(Tables.add_or_widen!, sch, %38, columns, %42, updated, $(QuoteNode(Base.HasLength())))::Any

Is that failure to inline related? Like, the inline-cost is calculated too expensive because we have NamedTuple{(:x, :y),_A} where _A<:Tuple{Int64,Union{Missing, Float64}}?

(I've double checked that my definitions of eachcolumns and related calls all have @inline appropriately).

base/compiler/tfuncs.jl Outdated Show resolved Hide resolved
Comment on lines 1132 to 1138
if isnamedtuple && isa(ua, UnionAll) && uw.parameters[2] === ua.var
# If the names are known, keep the upper bound (which we know is
# at most Tuple), but otherwise widen to Tuple.
# This is a widening heuristic to avoid keeping type information
# that's unlikely to be useful.
if uw.parameters[1] isa Tuple || (i == 2 && tparams[1] isa Tuple)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it assumes NamedTuple can only be used in very specific ways. But I don't see a particular reason to believe that i == 2 implies anything about it being the bound for the second parameter. As a trivial counter example, we can just flip the order of them:

julia> NamedTuple{T, S} where {S, T}
NamedTuple{T,S} where T where S

julia> ans{Tuple{Int}, (:a,)}
NamedTuple{(:a,),Tuple{Int64}}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what uw.parameters[2] === ua.var above is trying to check for --- that the var we're substituting for corresponds to the second parameter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I guess it's just the i == 2 && tparams[1] isa Tuple that seemed strange, but I suppose that variable can be assumed that it must appear as the names for the NamedTuple in this case. And it's hopefully not important to deal with the case where the variables are swapped.

base/compiler/tfuncs.jl Outdated Show resolved Hide resolved
@JeffBezanson JeffBezanson force-pushed the jb/fix32699 branch 2 times, most recently from 0d17843 to fb82add Compare June 24, 2020 19:54
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very late night thought: didn't we attempt part of this 6 months ago, before we realized this bound would be wrong, and did #33472 instead?

Counter-example:

julia> NamedTuple{<:Any, <:Any}
NamedTuple{var"#s1",var"#s2"} where var"#s2" where var"#s1"

Comment on lines 1138 to 1142
if uw.parameters[1] isa Tuple || (i == 2 && tparams[1] isa Tuple)
ub = typeintersect(ub, Tuple)
else
ub = Tuple
end
else
ub = Any
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if uw.parameters[1] isa Tuple || (i == 2 && tparams[1] isa Tuple)
ub = typeintersect(ub, Tuple)
else
ub = Tuple
end
else
ub = Any
end
if !(uw.parameters[1] isa Tuple || (i == 2 && tparams[1] isa Tuple))
ub = Any
end
else
ub = Any
end

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. The Tuple bound should be valid only if ai_w <: Type above holds, i.e. if whatever we're putting in there is not a typevar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not even then?

julia> NamedTuple{<:Any, Val{T}} where T
NamedTuple{var"#s1",Val{T}} where var"#s1" where T

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch.

@JeffBezanson JeffBezanson merged commit 5f2bb1d into master Jul 13, 2020
@JeffBezanson JeffBezanson deleted the jb/fix32699 branch July 13, 2020 20:55
@quinnj
Copy link
Member

quinnj commented Jul 13, 2020

Exciting! @JeffBezanson , do you have any thoughts on my inlining question from above? Just wondered if you were aware of something obvious. I'm planning on looking into it a little deeper as well.

@JeffBezanson
Copy link
Member Author

Since we infer eachcolumns as Any, I imagine there might be dynamic dispatches in it, which make the inlining cost very high.

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

Successfully merging this pull request may close these issues.

better upper bounds in apply_type_tfunc for NamedTuple
4 participants