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

vcat is type-unstable #247

Closed
aplavin opened this issue Sep 15, 2022 · 7 comments
Closed

vcat is type-unstable #247

aplavin opened this issue Sep 15, 2022 · 7 comments

Comments

@aplavin
Copy link
Member

aplavin commented Sep 15, 2022

A minimal example:

julia> @code_warntype vcat(StructArray(x=view([1], 1:1:1), y=view([:a], 1:1:1)))
...
Body::StructVector{NamedTuple{(:x, :y), Tuple{Int64, Symbol}}}  # red
...

The eltype of the result is correct, but the inner storage type (Vector{Int} and Vector{Symbol}) are not inferred.

Not sure where the issue lies...

@jishnub
Copy link
Member

jishnub commented Nov 24, 2022

This is because constant propagation doesn't happen at the top scope. This should work correctly when called inside a function.

julia> using Test

julia> @inferred (() -> vcat(StructArray(x=view([1], 1:1:1), y=view([:a], 1:1:1))))()
1-element StructArray(::Vector{Int64}, ::Vector{Symbol}) with eltype NamedTuple{(:x, :y), Tuple{Int64, Symbol}}:
 (x = 1, y = :a)

@aplavin
Copy link
Member Author

aplavin commented Nov 24, 2022

How constant propagation is relevant here? This vcat result type is completely defined by input types, so it should infer without any constprop.

@jishnub
Copy link
Member

jishnub commented Nov 24, 2022

So, what's happening here is that (for some reason), the constant key information isn't propagated to the function at

f = key -> $op((getproperty(t, key) for t in args)...; kwargs...)

and, as a consequence, the type of the result of getproperty can't be inferred.
Forcing aggressive constant propagation seems to fix this, so updating the code to:

Base.@constprop :aggressive reducekey(key, op, args; kwargs...) =
    op((getproperty(t, key) for t in args)...; kwargs...)
for op in [:cat, :hcat, :vcat]
    @eval begin
        function Base.$op(args::StructArray...; kwargs...)
            T = mapreduce(eltype, promote_type, args)
            StructArray{T}(map(key -> reducekey(key, $op, args; kwargs...), propertynames(args[1])))
        end
    end
end

I obtain

julia> A = StructArray(x=view([1], 1:1:1), y=view([:a], 1:1:1));

julia> using Test

julia> @inferred vcat(A)
1-element StructArray(::Vector{Int64}, ::Vector{Symbol}) with eltype NamedTuple{(:x, :y), Tuple{Int64, Symbol}}:
 (x = 1, y = :a)

However, ideally, this shouldn't be necessary on our side, and there might be some Julia heuristic that needs to be updated to propagate the constant information to the function. Do you mind opening an issue in the Julia repo about this?

@aplavin
Copy link
Member Author

aplavin commented Nov 24, 2022

Thanks for digging into it! I tried as well, but couldn't get this far.

I'm not familiar with how constprop works, just with very simple cases of immediate literals, like getproperty(x, :a). How could Julia guess that it better propagate key into the reducekey function? Doesn't really seem a straightforward decision for me.

Do you think the Base.@constprop approach is a reasonable solution here? Even if a compiler heuristic is somehow possible, it would only appear in some distant future in newer Julia versions.

@jishnub
Copy link
Member

jishnub commented Nov 24, 2022

Base.@constprop is not a part of the public API unfortunately, although it helps immensely with type-stability. The core developers have assured that they won't remove it unexpectedly without a warning. However, given that this isn't public, perhaps the constant propagation should be handled by Julia instead of us trying to force it in this package.

I'm not familiar with how it works as well, my guess is that there's some heuristic similar to inlining cost that it uses to determine if constants get propagated, and in this case, we cross the limit where it gives up. My guess is that using Base.@constprop :aggressive increases this cutoff (although I may be wrong about this).

I would say that it's reasonable to use Base.@constprop. It's unlikely that this will be changed significantly in Base, and I've noticed that people are generally ok with using it in other packages.

@aplavin
Copy link
Member Author

aplavin commented Nov 24, 2022

Now that you pointed out the actual issue, I think I have a cleaner solution. Will make a PR soon.

@aplavin
Copy link
Member Author

aplavin commented Nov 24, 2022

See #255

@aplavin aplavin closed this as completed Jan 11, 2023
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

Successfully merging a pull request may close this issue.

2 participants