697672 - MMgc adjustments to guarantee 16-byte alignment for float4 boxes

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P2)

Description

[Lars T Hansen]

(Forked from bug #697127)

Due to how GCAlloc works, float4 boxes are always 16-bit aligned in Release builds on all platforms.  But this is somewhat accidental.  We should codify it by documenting it.

In Debug builds there is no such guarantee because of the 8-byte padding added to the beginning of an object (the difference between the userptr and the realptr); for 16-byte alignment we need 16-byte padding.  There are probably two ways of fixing that, by having a per-allocator DebugSize or by using a 16-byte pad for all objects in all allocators.

Comment 1

[Lars T Hansen]

Attachment: 16 byte alignment everywhere

This is the variant that uses 16-byte alignment everywhere, I created this in order to unblock Virgil.  Is this good enough?  It will definitely increase memory pressure in Debug builds compared to a more elaborate 8-byte solution, but do we really care very much about that?

Comment 2

[Lars T Hansen]

Note, the backslash used at the end of the line in one of the comments will freak out some compilers and will need to be replaced eg by ')'.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 569916 [details] [diff] [review]
16 byte alignment everywhere

Review of attachment 569916 [details] [diff] [review]:
-----------------------------------------------------------------

::: MMgc/GC.cpp
@@ +299,1 @@
>  #endif

Yikes, I had not noticed that the prior version of this code was not truly parmeterized over DebugSize, since you need to keep the size class in sync with the total size.  Okay, I guess the expansion into the two cases here seems reasonable, given that.

::: MMgc/GCMemoryProfiler.cpp
@@ +1178,5 @@
> +        // 1 word of padding on 64-bit systems
> +        //
> +        // The 2 words of padding before the object are experimental / temporary, to get
> +        // 16-byte alignment for float4 boxes.  It's not ideal.
> +        return ((USER_POINTER_WORDS + 1 + sizeof(void*)/sizeof(int32_t) + 1) & ~1) * sizeof(int32_t);

Huge +1 to the comment above.

Why not break down the code into corresponding pieces?  I just find this line to go over my threshold of #components to parse mentally.

::: MMgc/GCMemoryProfiler.h
@@ +382,5 @@
> +    //
> +    // Traditionally USER_POINTER_WORDS is 2.  For proper alignment of float4 boxes
> +    // we require USER_POINTER_WORDS to be 4, however.  See comments in GC::GC for 
> +    // more about that.
> +#define USER_POINTER_WORDS 4

Huge +1 to the USER_POINTER_WORDS abstraction.

Comment 4

[Tommy Reilly]

Comment on attachment 569916 [details] [diff] [review]
16 byte alignment everywhere

This is fine to unblock float but I really think this whole MEMORY_INFO thing is too complicated and should be re-designed.  Some thoughts:

1) since the introduction of HEAP_GRAPH we no longer store a back pointer in the object, we should eradicate mention of that
2) one 16 byte block at the end should suffice, then the whole userpointer/realpointer business goes away.  Underwrites and overwrites could still be handled if we stored the underwrite_poison at the end of the block header (ie right before first item).
3) if we had one block at the end we wouldn't need to store actual_size.  We could just take the pointer, add the whole_size passed to DebugFree and check the poison before and after.


So you'd have for 64 bit:

[object][overwrite_poison][underwrite_poison]

and for 32 bit:

[object][overwrite_poison][8 byte slop][underwrite_poison]

poison values are uintptr_t.

Comment 5

[Lars T Hansen]

Another consideration here - when we reengineer the layout of metainfo - is that more likely than not I will argue in favor of going away from the per-block bitmaps and moving to object headers instead, as it will make allocation, deallocation, and marking faster and in particular more tractable to in-line.

I will offer a patch for review soonish with just the bare minimum of adjustments to the current one, but it may not need to land before we branch for Brannan so can land with the extra padding unconditionally enabled.  (Otherwise I'd be tempted to add an #ifdef VMCFG_FLOAT around it.)

Comment 6

[Lars T Hansen]

Attachment: Patch

Followed up on comments.  Added documentation to GCAlloc about the alignment guarantee.  Added some checks and balances on USER_POINTER_WORDS (must be 2 or 4).  Made USER_POINTER_WORDS 4 if VMCFG_FLOAT is defined and 2 otherwise (compatible with the old state of affairs, safeguards us if we have to land this code with a version that will not ship the feature).

Comment 7

[Lars T Hansen]

Comment on attachment 571620 [details] [diff] [review]
Patch

Not quite right.

Comment 8

[Lars T Hansen]

Attachment: Patch

Substantially the same as what was seen before.  Extracted from tr-float repo, ready for TR.  Diff relative to TR 6743:4473be1f334c.

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 576700 [details] [diff] [review]
Patch

Review of attachment 576700 [details] [diff] [review]:
-----------------------------------------------------------------

R+.  Looks good, but I admit confusion about your "sic" note, discussed below.

::: MMgc/GC.cpp
@@ +287,3 @@
>          GCAssert(DebugSize() == 24);
> +        bibopAllocFloat  = mmfx_new(GCAlloc(this, int(32), false, false, false, /*sizeclass*/3, avmplus::AtomConstants::kBibopFloatType));
> +        bibopAllocFloat4 = mmfx_new(GCAlloc(this, int(48), false, false, false, /*sizeclass*/5 /*sic!*/, avmplus::AtomConstants::kBibopFloat4Type));

I find the "sic!" here and below to be overly subtle; I spent a few minutes trying to see what (I presume to be) the "noted and/or quoted error" is here, and am now giving up.

(48 does indeed correspond to sizeclass 5 AFAICT, so either the reference is meant to reference both the 48 and the 5, or its just about the 5; either way its confusing.)

@@ +299,5 @@
> +        bibopAllocFloat4 = mmfx_new(GCAlloc(this, int(32), false, false, false, /*sizeclass*/3, avmplus::AtomConstants::kBibopFloat4Type));
> +#else
> +        GCAssert(DebugSize() == 24);
> +        bibopAllocFloat  = mmfx_new(GCAlloc(this, int(32), false, false, false, /*sizeclass*/3, avmplus::AtomConstants::kBibopFloatType));
> +        bibopAllocFloat4 = mmfx_new(GCAlloc(this, int(48), false, false, false, /*sizeclass*/5 /*sic!*/, avmplus::AtomConstants::kBibopFloat4Type));

See note above regarding "sic".

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

In hindsight I'm guessing the "sic" is just referring to the fact that 48 > (16 + DebugSize() == (16 + 24) == 40, and thus we "could" be allocating this with sizeclass 4 if not for the alignment constraints.

Assuming this guess is true, I'd still lose the "sic" (since its too subtle and scary) and replace it with slightly longer comment note on a separate line, i.e. the preceding line, that says "size of 48 and not 40 to satisfy alignment constraint; see notes above." or something along those lines.

Comment 11

[Lars T Hansen]

(In reply to Felix S Klock II from comment #10)
> In hindsight I'm guessing the "sic" is just referring to the fact that 48 >
> (16 + DebugSize() == (16 + 24) == 40, and thus we "could" be allocating this
> with sizeclass 4 if not for the alignment constraints.

Yup.

> Assuming this guess is true, I'd still lose the "sic" (since its too subtle
> and scary) and replace it with slightly longer comment note on a separate
> line, i.e. the preceding line, that says "size of 48 and not 40 to satisfy
> alignment constraint; see notes above." or something along those lines.

OK.

Comment 12

[Tamarin Bot]

changeset: 6745:d06ecb5d0bca
user:      Lars T Hansen <[e-mail]>
summary:   Fix 697672 - MMgc adjustments to guarantee 16-byte alignment for float4 boxes (r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/d06ecb5d0bca