Skip to content

Are isbits-Unions isbits? #23567

Closed
Closed

Description

Noticed in the current master bug:

julia> t = Vector{Union{Float64, Void}}(5)
5-element Array{Union{Void, Float64},1}:
 nothing
 nothing
 nothing
 nothing
 nothing

julia> b = collect(Union{Float64, Void}, 1.0:3.0)
3-element Array{Union{Void, Float64},1}:
 1.0
 2.0
 3.0

julia> copy!(t, 2, b)
5-element Array{Union{Void, Float64},1}:
 0.0
 1.0
 2.0
  nothing
  nothing

The issue is that we currently go thru the codepath in array.jl

function unsafe_copy!(dest::Array{T}, doffs, src::Array{T}, soffs, n) where T
    if isbits(T)
        unsafe_copy!(pointer(dest, doffs), pointer(src, soffs), n)
    else
        ccall(:jl_array_ptr_copy, Void, (Any, Ptr{Void}, Any, Ptr{Void}, Int),
              dest, pointer(dest, doffs), src, pointer(src, soffs), n)
    end

but isbits(Union{Float64, Void}) is false; so we hit jl_array_ptr_copy. Now, I did had a hook in jl_array_ptr_copy to intercept isbits-Union arrays and handle them specially, the problem is it currently only works when you happen to be copying to the 1st element of the dest array. This is because we only pass dest_p, a pointer and not an index, to jl_array_ptr_copy, so we don't actually know where to copy selector bytes from src to dest.

There a couple of ways to address this:

  • go ahead and make isbits(Union{Float64, Void}) == true, this would involve making sure we're careful in a # of places in Base where we're currently checking isbits(T) on arrays and doing optimizations (i.e. vcat, unsafe_copy, flipdim). The biggest ? for me in doing this is how this propagates thru the rest of the system, in particular, inference.jl (which has quite a few calls to isbits(T).). Overall, I think it would be more correct, but it does seem a bit worrisome not quite knowing the full extent of this kind of change. I can certainly do more research here if we think it's the best way to try out first.
  • make another method jl_array_ptr_copy2, which also takes ind arguments so we can correctly adjust when copying isbits-Union arrays.

Ultimately, I think we should make isbits-Unions isbits, because we're essentially doing that everywhere in src/, so we might as well be consistent in Base.

cc: @JeffBezanson, @vtjnash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions