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

quinnj · 2019-06-28T21:27:04Z

…tored inline in arrays

This allows more structures to be stored inline in arrays, like:

struct Foo
    x::Int
    y::Union{Float64, Missing}
end
# also namedtuples
NamedTuple{(:x, :y), Tuple{Int, Union{Float64, Missing}}}

We already had an isinlinealloc flag on DataType, so we switch jl_islayout_inline to use that instead of just isbitstype. Then, when we're setting the isinlinealloc flag for a DataType, we just check that each field is isbitstype or isbitsunion.

quinnj · 2019-06-29T03:33:50Z

test/core.jl

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


@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.

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.

quinnj · 2019-06-29T03:38:52Z

test/core.jl

@@ -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


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

c42f · 2019-06-30T05:50:38Z

Dumb question: what does this mean for the data layout of structs with union fields? Would it be possible/make sense to always recursively strip the selector bytes out of such structs and store them separately? That is, make the non-selector parts of arrays of unions and arrays of structs of unions ABI compatible with the C version of the same thing?

quinnj · 2019-07-02T04:05:13Z

@c42f, so I'm actually not familiar with the C memory layout of structs with union fields, is it just the max size of the largest union type? I guess it doesn't track which element was put in the field, but relies on the user pulling out whatever they want?

c42f · 2019-07-02T04:27:11Z

is it just the max size of the largest union type?

I think it's ultimately defined by the platform ABI just as the struct layout is. But yes, I'd guess the union bits would just have the largest size and strictest alignment of types within the union.

I guess it doesn't track which element was put in the field, but relies on the user pulling out whatever they want?

Correct; in C you need to store the type tag info explicitly, or infer it somehow from other state in the system.

quinnj · 2019-07-08T19:50:32Z

Bump. Probably best if @JeffBezanson, @vtjnash, or @Keno review this. No urgency really, but when you have a free moment.

quinnj · 2019-07-17T15:32:49Z

Bump again.

vtjnash · 2019-07-17T16:56:17Z

Let's look at JuliaCon

quinnj · 2019-08-02T04:03:29Z

Bump :)

vtjnash · 2019-08-05T21:35:12Z

This seems pretty good so far. It's good to see how small it is as I wasn't sure how much of the rest of the system knew to check these flags, and not assume that isbits told all.

The one aspect to finish this up that I think may need to be preserved is to ensure that Vector{Foo}(undef, 1)[1] throws an UndefRefException.

quinnj · 2019-08-05T22:46:59Z

The one aspect to finish this up that I think may need to be preserved is to ensure that Vector{Foo}(undef, 1)[1] throws an UndefRefException.

Hmmm......do we really though? That's kind of a big crux of the change here is that Foo in a Vector{Foo}(undf, 1) now behaves more like if you did:

struct Foo
    x::Int
    y::Union{Missing, Float64}
    Foo() = new()
end
[Foo()] == Vector{Foo}(undef, 1)

So IMO, this almost feels more like a "bugfix" than change in behavior. How would we still do an UndefRefException? Special case these structs in isassigned and getindex?

quinnj · 2019-08-15T17:40:08Z

Bump, this would be nice to have for 1.3

JeffBezanson · 2019-08-15T18:08:53Z

src/datatype.c

@@ -248,7 +248,8 @@ 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))
+    {


{ on previous line

…tored inline in arrays

quinnj · 2019-08-16T02:54:36Z

I see this was marked for triage; was anything concluded?

StefanKarpinski · 2019-08-16T03:48:37Z

There was a heated discussion, I'm not sure what we concluded. @JeffBezanson, you seemed in favor.

quinnj · 2019-08-21T04:04:55Z

@vtjnash, do you have any follow up here from previous discussion/questions? Anything else we need to change here?

quinnj · 2019-08-27T15:59:23Z

I'll plan on merging this in 12 hours or so unless @vtjnash or anyone else has concerns.

c42f · 2019-08-28T06:30:38Z

If I define

struct WrappedUnion
    v::Union{Missing,Float64}
end

how does the storage layout now compare to Vector{Union{Missing,Float64}}?

I'd like to suggest that the storage layout for these two Vectors should be the same (cf. #32448 (comment)), and that the selector bits should be hoisted out in the same way in both cases.

JeffBezanson · 2019-08-29T17:07:48Z

That's pretty difficult to do, since it means the layout of the same struct is different depending on where/how that struct is stored. It's also hard to handle things like Vector{Union{StructWithUnions, StructWithoutUnions}}.

vtjnash · 2019-08-29T21:09:58Z

I still think this PR was a breaking change in its current state and needed updates to the documentation.

c42f · 2019-08-30T12:49:21Z

That's pretty difficult to do, since it means the layout of the same struct is different depending on where/how that struct is stored.

I feel like with systematic hoisting of selectors you can get consistent layout when each such struct comes in two separable but consistently laid out parts: the data and the type selector bits. I've tried to write it out in more detail over at #33120.

quinnj · 2019-08-30T16:58:12Z

@vtjnash, it would be helpful if you could be more specific about what exactly you consider breaking and what documentation needs updating. As I've posted a few times here, I'm happy to push forward on things, but the lack of responses/direction has left this in its current state. Personally, I still feel this is more "bugfix" than breaking given the work and docs that went in to general isbits-Union optimizations.

vtjnash · 2019-12-20T23:17:36Z

I still think this PR was a breaking change in its current state and needed updates to the documentation.

I still keep running into problems that this causes internally because it was a breaking change that changes assumptions made by some users and inference/codegen. I know you wanted to push ahead faster on this then I could review, and were rejecting my requests for changes to make it non-breaking. But it seems pretty yucky now that we implement isbitsunion to mean a union of things that aren't necessarily isbits (but still document it to mean the old thing).

Personally, I still feel this is more "bugfix" than breaking given the work and docs that went in to general isbits-Union optimizations.

I generally intended the isbits optimizations to be exactly that: optimizations for unions of isbits types. The documentation was intended to reflect that. Regardless of what you feel like calling it, the fact is it changed that memory model without fixing all of the places where we assume the prior memory model, both in the documentation and in the code.

quinnj requested a review from vtjnash June 28, 2019 21:27

quinnj commented Jun 29, 2019

View reviewed changes

StefanKarpinski added the forget me not PRs that one wants to make sure aren't forgotten label Jul 17, 2019

c42f mentioned this pull request Jul 22, 2019

Suboptimal inference for incompletely specified named tuples #29970

Closed

quinnj mentioned this pull request Aug 8, 2019

NamedTuple covariance #32825

Closed

JeffBezanson reviewed Aug 15, 2019

View reviewed changes

JeffBezanson added the triage This should be discussed on a triage call label Aug 15, 2019

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

d98544d

…tored inline in arrays

quinnj force-pushed the jq/inlineunionfields branch from 1995295 to d98544d Compare August 15, 2019 19:25

Add NEWS entry for inline isbits/isbitsunion structs

cf4796c

Merge branch 'master' into jq/inlineunionfields

d94e2b5

quinnj merged commit 8093a7c into master Aug 28, 2019

delete-merged-branch bot deleted the jq/inlineunionfields branch August 28, 2019 04:29

vtjnash removed forget me not PRs that one wants to make sure aren't forgotten triage This should be discussed on a triage call labels Aug 29, 2019

c42f mentioned this pull request Aug 30, 2019

mini julep: C ABI for inline structs and unions #33120

Open

quinnj mentioned this pull request Sep 17, 2019

Taking Juleps seriously #33239

Closed

This was referenced Oct 28, 2019

Self-recursive type definition with a Union segfaults #33709

Closed

Reorder self-referential types checking to prevent inline allocation #33718

Merged

vtjnash mentioned this pull request Oct 30, 2019

datatype: reorganize layout calculation code #33724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

quinnj commented Jun 28, 2019

quinnj Jun 29, 2019

quinnj Jun 29, 2019

quinnj Jun 29, 2019

c42f commented Jun 30, 2019 •

edited

Loading

quinnj commented Jul 2, 2019

c42f commented Jul 2, 2019

quinnj commented Jul 8, 2019

quinnj commented Jul 17, 2019

vtjnash commented Jul 17, 2019

quinnj commented Aug 2, 2019

vtjnash commented Aug 5, 2019

quinnj commented Aug 5, 2019

quinnj commented Aug 15, 2019

JeffBezanson Aug 15, 2019

quinnj Aug 15, 2019

quinnj commented Aug 16, 2019

StefanKarpinski commented Aug 16, 2019

quinnj commented Aug 21, 2019

quinnj commented Aug 27, 2019

c42f commented Aug 28, 2019

JeffBezanson commented Aug 29, 2019

vtjnash commented Aug 29, 2019

c42f commented Aug 30, 2019 •

edited

Loading

quinnj commented Aug 30, 2019

vtjnash commented Dec 20, 2019

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

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

Conversation

quinnj commented Jun 28, 2019

quinnj Jun 29, 2019

Choose a reason for hiding this comment

quinnj Jun 29, 2019

Choose a reason for hiding this comment

quinnj Jun 29, 2019

Choose a reason for hiding this comment

c42f commented Jun 30, 2019 • edited Loading

quinnj commented Jul 2, 2019

c42f commented Jul 2, 2019

quinnj commented Jul 8, 2019

quinnj commented Jul 17, 2019

vtjnash commented Jul 17, 2019

quinnj commented Aug 2, 2019

vtjnash commented Aug 5, 2019

quinnj commented Aug 5, 2019

quinnj commented Aug 15, 2019

JeffBezanson Aug 15, 2019

Choose a reason for hiding this comment

quinnj Aug 15, 2019

Choose a reason for hiding this comment

quinnj commented Aug 16, 2019

StefanKarpinski commented Aug 16, 2019

quinnj commented Aug 21, 2019

quinnj commented Aug 27, 2019

c42f commented Aug 28, 2019

JeffBezanson commented Aug 29, 2019

vtjnash commented Aug 29, 2019

c42f commented Aug 30, 2019 • edited Loading

quinnj commented Aug 30, 2019

vtjnash commented Dec 20, 2019

c42f commented Jun 30, 2019 •

edited

Loading

c42f commented Aug 30, 2019 •

edited

Loading