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.

Sign up for GitHub

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

For structs with all isbits or isbitsunion fields, allow them to be s… #32448

Merged
merged 3 commits into from
Aug 28, 2019
Merged

For structs with all isbits or isbitsunion fields, allow them to be s… #32448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d98544d
For structs with all isbits or isbitsunion fields, allow them to be s…
quinnj Aug 15, 2019
cf4796c
Add NEWS entry for inline isbits/isbitsunion structs
quinnj Aug 16, 2019
d94e2b5
Merge branch 'master' into jq/inlineunionfields
quinnj Aug 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Julia v1.4 Release Notes
New language features
---------------------

* Structs with all isbits and isbitsunion fields are now stored inline in arrays ([#32448]).

Language changes
----------------
Expand Down
30 changes: 15 additions & 15 deletions base/array.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,8 @@ size(a::Array{<:Any,N}) where {N} = (@_inline_meta; ntuple(M -> size(a, M), Val(

asize_from(a::Array, n) = n > ndims(a) ? () : (arraysize(a,n), asize_from(a, n+1)...)

allocatedinline(::Type{T}) where {T} = (@_pure_meta; ccall(:jl_array_store_unboxed, Cint, (Any,), T) != Cint(0))

"""
Base.isbitsunion(::Type{T})

Expand All @@ -172,14 +174,12 @@ julia> Base.isbitsunion(Union{Float64, String})
false
```
"""
isbitsunion(u::Union) = (@_pure_meta; ccall(:jl_array_store_unboxed, Cint, (Any,), u) != Cint(0))
isbitsunion(u::Union) = allocatedinline(u)
isbitsunion(x) = false

isptrelement(t::Type) = (@_pure_meta; ccall(:jl_array_store_unboxed, Cint, (Any,), t) == Cint(0))

function _unsetindex!(A::Array{T}, i::Int) where {T}
@boundscheck checkbounds(A, i)
if isptrelement(T)
if !allocatedinline(T)
t = @_gc_preserve_begin A
p = Ptr{Ptr{Cvoid}}(pointer(A))
unsafe_store!(p, C_NULL, i)
Expand Down Expand Up @@ -212,7 +212,7 @@ function bitsunionsize(u::Union)
end

length(a::Array) = arraylen(a)
elsize(::Type{<:Array{T}}) where {T} = isbitstype(T) ? sizeof(T) : (isbitsunion(T) ? bitsunionsize(T) : sizeof(Ptr))
elsize(::Type{<:Array{T}}) where {T} = isbitsunion(T) ? bitsunionsize(T) : (allocatedinline(T) ? sizeof(T) : sizeof(Ptr))
sizeof(a::Array) = Core.sizeof(a)

function isassigned(a::Array, i::Int...)
Expand Down Expand Up @@ -255,16 +255,16 @@ the same manner as C.
function unsafe_copyto!(dest::Array{T}, doffs, src::Array{T}, soffs, n) where T
t1 = @_gc_preserve_begin dest
t2 = @_gc_preserve_begin src
if isbitstype(T)
unsafe_copyto!(pointer(dest, doffs), pointer(src, soffs), n)
elseif isbitsunion(T)
if isbitsunion(T)
ccall(:memmove, Ptr{Cvoid}, (Ptr{Cvoid}, Ptr{Cvoid}, UInt),
pointer(dest, doffs), pointer(src, soffs), n * Base.bitsunionsize(T))
# copy selector bytes
ccall(:memmove, Ptr{Cvoid}, (Ptr{Cvoid}, Ptr{Cvoid}, UInt),
ccall(:jl_array_typetagdata, Ptr{UInt8}, (Any,), dest) + doffs - 1,
ccall(:jl_array_typetagdata, Ptr{UInt8}, (Any,), src) + soffs - 1,
n)
elseif allocatedinline(T)
unsafe_copyto!(pointer(dest, doffs), pointer(src, soffs), n)
else
ccall(:jl_array_ptr_copy, Cvoid, (Any, Ptr{Cvoid}, Any, Ptr{Cvoid}, Int),
dest, pointer(dest, doffs), src, pointer(src, soffs), n)
Expand Down Expand Up @@ -1548,28 +1548,28 @@ function vcat(arrays::Vector{T}...) where T
end
arr = Vector{T}(undef, n)
ptr = pointer(arr)
if isbitstype(T)
elsz = Core.sizeof(T)
elseif isbitsunion(T)
if isbitsunion(T)
elsz = bitsunionsize(T)
selptr = ccall(:jl_array_typetagdata, Ptr{UInt8}, (Any,), arr)
elseif allocatedinline(T)
elsz = Core.sizeof(T)
else
elsz = Core.sizeof(Ptr{Cvoid})
end
t = @_gc_preserve_begin arr
for a in arrays
na = length(a)
nba = na * elsz
if isbitstype(T)
ccall(:memcpy, Ptr{Cvoid}, (Ptr{Cvoid}, Ptr{Cvoid}, UInt),
ptr, a, nba)
elseif isbitsunion(T)
if isbitsunion(T)
ccall(:memcpy, Ptr{Cvoid}, (Ptr{Cvoid}, Ptr{Cvoid}, UInt),
ptr, a, nba)
# copy selector bytes
ccall(:memcpy, Ptr{Cvoid}, (Ptr{Cvoid}, Ptr{Cvoid}, UInt),
selptr, ccall(:jl_array_typetagdata, Ptr{UInt8}, (Any,), a), na)
selptr += na
elseif allocatedinline(T)
ccall(:memcpy, Ptr{Cvoid}, (Ptr{Cvoid}, Ptr{Cvoid}, UInt),
ptr, a, nba)
else
ccall(:jl_array_ptr_copy, Cvoid, (Any, Ptr{Cvoid}, Any, Ptr{Cvoid}, Int),
arr, ptr, a, pointer(a), na)
Expand Down
2 changes: 1 addition & 1 deletion base/arraymath.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ function reverse(A::Array{T}; dims::Integer) where T
end
end
else
if isbitstype(T) && M>200
if allocatedinline(T) && M>200
for i = 1:sd
ri = sd+1-i
for j=0:stride:(N-stride)
Expand Down
10 changes: 6 additions & 4 deletions src/datatype.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ static unsigned union_isbits(jl_value_t *ty, size_t *nbytes, size_t *align) JL_N
return 0;
return na + nb;
}
if (jl_isbits(ty)) {
if (jl_is_datatype(ty) && jl_datatype_isinlinealloc(ty)) {
size_t sz = jl_datatype_size(ty);
size_t al = jl_datatype_align(ty);
if (*nbytes < sz)
Expand Down Expand Up @@ -292,6 +292,7 @@ static int references_name(jl_value_t *p, jl_typename_t *name) JL_NOTSAFEPOINT
void jl_compute_field_offsets(jl_datatype_t *st)
{
size_t sz = 0, alignm = 1;
size_t fldsz = 0, fldal = 0;
int homogeneous = 1;
jl_value_t *lastty = NULL;
uint64_t max_offset = (((uint64_t)1) << 32) - 1;
Expand All @@ -302,12 +303,14 @@ void jl_compute_field_offsets(jl_datatype_t *st)
// compute whether this type can be inlined
// based on whether its definition is self-referential
if (w->types != NULL) {
st->isbitstype = st->isconcretetype && !st->mutabl;
st->isbitstype = st->isinlinealloc = st->isconcretetype && !st->mutabl;
size_t i, nf = jl_svec_len(st->types);
for (i = 0; i < nf; i++) {
jl_value_t *fld = jl_svecref(st->types, i);
if (st->isbitstype)
st->isbitstype = jl_is_datatype(fld) && ((jl_datatype_t*)fld)->isbitstype;
if (st->isinlinealloc)
st->isinlinealloc = (jl_is_datatype(fld) && ((jl_datatype_t*)fld)->isbitstype) || jl_islayout_inline(fld, &fldsz, &fldal);
if (!st->zeroinit)
st->zeroinit = (jl_is_datatype(fld) && ((jl_datatype_t*)fld)->isinlinealloc) ? ((jl_datatype_t*)fld)->zeroinit : 1;
if (i < st->ninitialized) {
Expand All @@ -317,8 +320,7 @@ void jl_compute_field_offsets(jl_datatype_t *st)
st->has_concrete_subtype &= !jl_is_datatype(fld) || ((jl_datatype_t *)fld)->has_concrete_subtype;
}
}
if (st->isbitstype) {
st->isinlinealloc = 1;
if (st->isinlinealloc) {
size_t i, nf = jl_svec_len(w->types);
for (i = 0; i < nf; i++) {
jl_value_t *fld = jl_svecref(w->types, i);
Expand Down
1 change: 1 addition & 0 deletions src/julia.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,6 +931,7 @@ STATIC_INLINE jl_value_t *jl_field_type_concrete(jl_datatype_t *st JL_PROPAGATES
#define jl_datatype_align(t) (((jl_datatype_t*)t)->layout->alignment)
#define jl_datatype_nbits(t) ((((jl_datatype_t*)t)->size)*8)
#define jl_datatype_nfields(t) (((jl_datatype_t*)(t))->layout->nfields)
#define jl_datatype_isinlinealloc(t) (((jl_datatype_t *)(t))->isinlinealloc)

// inline version with strong type check to detect typos in a `->name` chain
STATIC_INLINE char *jl_symbol_name_(jl_sym_t *s) JL_NOTSAFEPOINT
Expand Down
27 changes: 23 additions & 4 deletions test/core.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5852,9 +5852,9 @@ let
b3 = B23367[b][1] # copy b via array assignment
addr(@nospecialize x) = ccall(:jl_value_ptr, Ptr{Cvoid}, (Any,), x)
@test addr(b) == addr(b)
@test addr(b) == addr(b2)
@test addr(b) == addr(b3)
@test addr(b2) == addr(b3)
# @test addr(b) == addr(b2)
Copy link
Member Author

Choose a reason for hiding this comment

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

@vtjnash, these tests are failing on this branch; is that expected? I'm not sure why the Ref b2 case would be affected by anything I've changed, but I think I understand that for b3, because it's an array, when we store it in an array (inline now), then extract it back again, it's technically a different jl_value_ptr, but still the same bits.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so it looks like in the Ref case, the alignment of B23367 is now different, which is used by datatype_pointerfree, to determine how to unsafe_convert to a Ptr. Again, it seems like we're more correct now by saying that B23367 is indeed datatype_pointerfree. I'm not sure how much people really run into this kind of thing because doing ccall w/ structs w/ Union fields probably isn't very common.

# @test addr(b) == addr(b3)
# @test addr(b2) == addr(b3)

@test b === b2 === b3 === b
@test egal(b, b2) && egal(b2, b3) && egal(b3, b)
Expand All @@ -5863,7 +5863,7 @@ let
@test b.x === Int8(91)
@test b.z === Int8(23)
@test b.y === A23367((Int8(1), Int8(2), Int8(3), Int8(4), Int8(5), Int8(6), Int8(7)))
@test sizeof(b) == sizeof(Int) * 3
@test sizeof(b) == 12
Copy link
Member Author

Choose a reason for hiding this comment

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

So I think this is more correct now? The change is via the call to jl_islayout_inline in jl_f_sizeof here.

@test A23367(Int8(1)).x === Int8(1)
@test A23367(Int8(0)).x === Int8(0)
@test A23367(Int16(1)).x === Int16(1)
Expand Down Expand Up @@ -5891,6 +5891,25 @@ for U in boxedunions
end
end

struct UnionFieldInlineStruct
x::Int64
y::Union{Float64, Missing}
end

@test sizeof(Vector{UnionFieldInlineStruct}(undef, 2)) == sizeof(UnionFieldInlineStruct) * 2

let x = UnionFieldInlineStruct(1, 3.14)
AInlineUnion = [x for i = 1:10]
@test sizeof(AInlineUnion) == sizeof(UnionFieldInlineStruct) * 10
BInlineUnion = Vector{UnionFieldInlineStruct}(undef, 10)
copyto!(BInlineUnion, AInlineUnion)
@test AInlineUnion == BInlineUnion
@test BInlineUnion[end] == x
CInlineUnion = vcat(AInlineUnion, BInlineUnion)
@test sizeof(CInlineUnion) == sizeof(UnionFieldInlineStruct) * 20
@test CInlineUnion[end] == x
end

# issue 31583
a31583 = "a"
f31583() = a31583 === "a"
Expand Down