626402 - Add floatAllocator

Status: VERIFIED FIXED

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

Description

[Virgil Palanciuc]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/8.0.552.237 Safari/534.10
Build Identifier: 

A new BIBIOP allocater is required in order to allow constructing float values

Reproducible: Always

Comment 1

[Virgil Palanciuc]

Attachment: patch

Comment 2

[Virgil Palanciuc]

Attachment: bibop float allocator

The allocator (re)uses kSpecialTag to tag bibop pages now, leaving kDoubleType untouched.
Note: changes in AvmCore.cpp are done merely to allow compilation, not complete handling of floats. This patch just enables the float allocator

Comment 3

[Virgil Palanciuc]

Attachment: bibop float allocator

changed when rebasing

Comment 4

[Lars T Hansen]

Editorial / surface comments only, the patch is fine:

The hardcoding of 4095 in atom.h is worrisome, is there any reason we could not use (GCHeap::kBlockSize-1) here?  If that name is not available then there should be an assert eg in atom.cpp that the block size remains 4K and comments here pointing to that assert.

Seems to me that we could simplify bibopKind() by having a page that has the necessary information on it so that there would not be a conditional here; should either fix that or log a bug on it and reference the bug from a comment here.

In GCAlloc.h comment about bitsShift should follow the definition of that variable and not be left hanging at the end of the struct.

Not clear to me why GCAlloc::GCAlloc could not take a "uint8_t bibopTag" argument instead of an "int bibopTag" argument that then requires a cast.

The change to GC::pruneBlacklist should not be in this patch, much as we'd like it to be.

There are some interactions with this patch with a longish patch queue that I'm developing in bug #660800 so I'll want to coordinate carefully because the patches overlap a bit, but it's nothing major.

We probably want buy-in on the new tagging scheme from Steven.

Comment 5

[Virgil Palanciuc]

I don't understand your reference to GC::pruneBlacklist, I didn't change it.

As for the others:
- added static assert to verify that block size & mask stay in sync (hard to use GCHead::kBlockSize-1 due to header inclusion order)
- simplified bibopKind - was too defensive, it should never be called on non-bibop atoms (I'll have to change a few places in the code, but it's worth it).
- used uint8_t instead of int

Comment 6

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

(In reply to comment #5)
> I don't understand your reference to GC::pruneBlacklist, I didn't change it.
> 

Explanation of Lars's comment for Virgil:

Your patch is attachment 535700 [details] [diff] [review].

Below I have linked to the specific portion of your attached patch that Lars is referring to.

https://bugzilla.mozilla.org/attachment.cgi?id=535700&action=diff&headers=0#a/MMgc/GC.cpp_sec10

Further explanation:
The change to GC::pruneBlacklist is a reversion to a prior version of the code; I suspect you used a somewhat broken methodology to generate your patch file, since I suspect you did not intend to revert such changes to their prior state.

Revision where the new version of the code was introduced:
  changeset: 6353:fed12375eb1f
  http://hg.mozilla.org/tamarin-redux/rev/fed12375eb1f

The new version has an extra level of nesting that Tommy *had* to introduce, so that the Iterator destructor will be called before blacklist.prune is invoked.  (The alternative of not requiring the extra-nesting would have required the alternative fix to Bug 637993 that was initially proposed but deemed undesirable; see the Bug 637993 ticket for further discussion of that matter.)

Comment 7

[Virgil Palanciuc]

Attachment: bibop float allocator

Fixed the issues identified by Lars, and rebased on tamarin-redux 6376:f75bdb601e36

Steven, please review the new tagging scheme

Comment 8

[Lars T Hansen]

Comment on attachment 537527 [details] [diff] [review]
bibop float allocator

Steven, we need a SR on the tagging scheme used here.

Comment 9

[Steven Johnson]

Comment on attachment 537527 [details] [diff] [review]
bibop float allocator

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

Generally fine, but I'd like the nits about comments-and-constants addressed before landing.

::: MMgc/GC-inlines.h
@@ +188,5 @@
>  #else
>          return Alloc(8,0);
>  #endif
>      }
> +    REALLY_INLINE void* GC::AllocFloat()

super-duper-style nit: blank line between methods, please.

@@ +191,5 @@
>      }
> +    REALLY_INLINE void* GC::AllocFloat()
> +    {
> +#if !defined _DEBUG && !defined AVMPLUS_SAMPLER && !defined MMGC_MEMORY_PROFILER
> +        return GetUserPointer(bibopAllocFloat->Alloc(0));

Why Alloc(0)? isn't Alloc(sizeof(float)) a better choice?

::: MMgc/GC.cpp
@@ +259,5 @@
> +        bibopAllocFloat =  mmfx_new(GCAlloc(this, 8, false, false, 0, avmplus::AtomConstants::kBibopFloatType));
> +#else
> +#ifdef MMGC_64BIT
> +        GCAssert(DebugSize() == 24);
> +        bibopAllocFloat = mmfx_new(GCAlloc(this, 32, false, false, 3, avmplus::AtomConstants::kBibopFloatType));

What are the magic numbers here? I presume 32 == 8 + DebugSize()? What's the 3?

@@ +858,5 @@
> +            core->sampleCheck();
> +#endif
> +        void* item;
> +#if defined _DEBUG || defined MMGC_MEMORY_PROFILER
> +        item = bibopAllocFloat->Alloc(4, 0);

Again, I'd like to see constants of sizeof() rather than magic numbers here. Or at least comments about the numbers.

::: MMgc/GCAlloc.cpp
@@ +141,5 @@
>  namespace MMgc
>  {
> +    /* Here we check that kBibopTypePtrMask is a proper mask for a GC block*/
> +    MMGC_STATIC_ASSERT((~avmplus::AtomConstants::kBibopTypePtrMask)+1 == GCHeap::kBlockSize);
> +    MMGC_STATIC_ASSERT((avmplus::AtomConstants::kBibopTypePtrMask& GCHeap::kBlockSize) == GCHeap::kBlockSize);

nit: prevailing style is a space on either side of "&" in bitwise-and; please conform to that style

@@ +142,5 @@
>  {
> +    /* Here we check that kBibopTypePtrMask is a proper mask for a GC block*/
> +    MMGC_STATIC_ASSERT((~avmplus::AtomConstants::kBibopTypePtrMask)+1 == GCHeap::kBlockSize);
> +    MMGC_STATIC_ASSERT((avmplus::AtomConstants::kBibopTypePtrMask& GCHeap::kBlockSize) == GCHeap::kBlockSize);
> +    MMGC_STATIC_ASSERT(( (~avmplus::AtomConstants::kBibopTypePtrMask)& GCHeap::kBlockSize) == 0);

nit: spacing around the "(" and ")" should be consistent; either have a space on both sides, or neither.

::: MMgc/GCAlloc.h
@@ +66,5 @@
>      struct GCBlockHeader
>      {
> +        uint8_t         bibopTag;   // *MUST* be the first byte.  0 means "not a bibop block."  For others, see vmbase/atom.h.
> +        uint8_t         bitsShift;  // Right shift for lower 12 bits of a pointer into the block to obtain the mark bit item for that pointer
> +        uint8_t         containsPointers;

I presume this is treated as a bool, but declared as a uint8_t to ensure size/alignment? if so, please add comment to that effect. (ditto for 'rcobject' below)

::: core/AvmCore-inlines.h
@@ +125,5 @@
>      return atomKind(atom) == kObjectType && !isNull(atom);
>  }
>  
>  REALLY_INLINE /*static*/ bool AvmCore::isPointer(Atom atom)
> +{ // kDouble, all kTypes less than kSpecialType, and kSpecialType if it's not "null" - i.e. not undefinedAtom, but a bibopType.

prevailing style in this code is to avoid comments on the line containing the opening {, eg

    {
        // kDouble...

rather than

    { // kDouble...

@@ +230,5 @@
>  }
>  
>  REALLY_INLINE /*static*/ bool AvmCore::isNumber(Atom atom)
>  {
>      // accept kIntptrType(6) or kDoubleType(7)

update the comment, please

::: vmbase/atom.h
@@ +106,5 @@
>           */
>          /*@{*/
>          // cannot use 0 as tag, breaks atomWriteBarrier
>          const Atom kUnusedAtomTag = 0;
> +        const Atom kObjectType    = 1;       // null=1

nit: let's update all of the constants so they still line up nicely

@@ +160,5 @@
>          /*@{*/
>          const Atom kAtomTypeMask = 7;       // mask for type tag
>          const Atom kAtomPtrMask = ~7;       // mask for atom pointer, or shifted Intptr value
>          const unsigned kAtomTypeSize = 3;   // width of type tag
> +        const Atom kBibopTypePtrMask= ~4095 ;// mask for bibop type pointer (char*, first char from the bibop page)

I'll echo Lar's comment here; this need to reference a constant somewhere, somehow, and also document why the magic number is what it is

@@ +205,4 @@
>      #define atomPtr(a)      ((void*)(uintptr_t(a) & ~7))
> +    // bibopKind should be kBibopFloatType or kBibopFloat4Type
> +    // in the future also kBibopDecimalType etc.
> +    #define bibopKind(a)     ( *(uint8_t*) (uintptr_t(a) & kBibopTypePtrMask) )

In the name of robustness, may I suggest



#ifdef _DEBUG
    REALLY_INLINE uint8_t bibopKind(const Atom& a)
    {
        AvmAssert(atomKind(a) == kSpecialBibopType);
        uint8_t const k = *(uint8_t*)(uintptr_t(a) & kBibopTypePtrMask);
        AvmAssert(k == kBibopUndefined || k == kBibopFloatType);
        return k;
    }
#else
    #define bibopKind(a)     ( *(uint8_t*) (uintptr_t(a) & kBibopTypePtrMask) )
#endif

Comment 10

[Lars T Hansen]

(In reply to comment #9)
> > +        return GetUserPointer(bibopAllocFloat->Alloc(0));
> 
> Why Alloc(0)? isn't Alloc(sizeof(float)) a better choice?

No, this is the flags argument.  I will add a comment to clarify.

> 
> ::: MMgc/GC.cpp
> @@ +259,5 @@
> > +        bibopAllocFloat =  mmfx_new(GCAlloc(this, 8, false, false, 0, avmplus::AtomConstants::kBibopFloatType));
> > +#else
> > +#ifdef MMGC_64BIT
> > +        GCAssert(DebugSize() == 24);
> > +        bibopAllocFloat = mmfx_new(GCAlloc(this, 32, false, false, 3, avmplus::AtomConstants::kBibopFloatType));
> 
> What are the magic numbers here? I presume 32 == 8 + DebugSize()? 

Yes.  And the GC can only allocate in units of 8 bytes.  I have clarified.

> What's the 3?

The 0, 3, and 2 are size class indices (as can be read from the signature of the GCAlloc constructor; not worth defining named constants for these).  I have added informative comments.

> @@ +858,5 @@
> > +            core->sampleCheck();
> > +#endif
> > +        void* item;
> > +#if defined _DEBUG || defined MMGC_MEMORY_PROFILER
> > +        item = bibopAllocFloat->Alloc(4, 0);
> 
> Again, I'd like to see constants of sizeof() rather than magic numbers here.
> Or at least comments about the numbers.

Commented, since in C and C++ sizeof(float) does not need to be 4.  (Lots of things would break if it's not but hey.)

> ::: MMgc/GCAlloc.cpp
> @@ +141,5 @@
> >  namespace MMgc
> >  {
> > +    /* Here we check that kBibopTypePtrMask is a proper mask for a GC block*/
> > +    MMGC_STATIC_ASSERT((~avmplus::AtomConstants::kBibopTypePtrMask)+1 == GCHeap::kBlockSize);
> > +    MMGC_STATIC_ASSERT((avmplus::AtomConstants::kBibopTypePtrMask& GCHeap::kBlockSize) == GCHeap::kBlockSize);
> 
> nit: prevailing style is a space on either side of "&" in bitwise-and;
> please conform to that style

Code rewritten as a result of other cleanup.

> ::: MMgc/GCAlloc.h
> @@ +66,5 @@
> >      struct GCBlockHeader
> >      {
> > +        uint8_t         bibopTag;   // *MUST* be the first byte.  0 means "not a bibop block."  For others, see vmbase/atom.h.
> > +        uint8_t         bitsShift;  // Right shift for lower 12 bits of a pointer into the block to obtain the mark bit item for that pointer
> > +        uint8_t         containsPointers;
> 
> I presume this is treated as a bool, but declared as a uint8_t to ensure
> size/alignment? if so, please add comment to that effect. (ditto for
> 'rcobject' below)

Commented.

> 
> ::: core/AvmCore-inlines.h
> @@ +125,5 @@
> >      return atomKind(atom) == kObjectType && !isNull(atom);
> >  }
> >  
> >  REALLY_INLINE /*static*/ bool AvmCore::isPointer(Atom atom)
> > +{ // kDouble, all kTypes less than kSpecialType, and kSpecialType if it's not "null" - i.e. not undefinedAtom, but a bibopType.
> 
> prevailing style in this code is to avoid comments on the line containing
> the opening {, eg
> 
>     {
>         // kDouble...
> 
> rather than
> 
>     { // kDouble...

Fixed.

> @@ +230,5 @@
> >  }
> >  
> >  REALLY_INLINE /*static*/ bool AvmCore::isNumber(Atom atom)
> >  {
> >      // accept kIntptrType(6) or kDoubleType(7)
> 
> update the comment, please

Removed the comment, it is redundant (adds no information to what's in the code).

> ::: vmbase/atom.h
> @@ +106,5 @@
> >           */
> >          /*@{*/
> >          // cannot use 0 as tag, breaks atomWriteBarrier
> >          const Atom kUnusedAtomTag = 0;
> > +        const Atom kObjectType    = 1;       // null=1
> 
> nit: let's update all of the constants so they still line up nicely

Did that.

> @@ +160,5 @@
> >          /*@{*/
> >          const Atom kAtomTypeMask = 7;       // mask for type tag
> >          const Atom kAtomPtrMask = ~7;       // mask for atom pointer, or shifted Intptr value
> >          const unsigned kAtomTypeSize = 3;   // width of type tag
> > +        const Atom kBibopTypePtrMask= ~4095 ;// mask for bibop type pointer (char*, first char from the bibop page)
> 
> I'll echo Lar's comment here; this need to reference a constant somewhere,
> somehow, and also document why the magic number is what it is

Fixed.

> @@ +205,4 @@
> >      #define atomPtr(a)      ((void*)(uintptr_t(a) & ~7))
> > +    // bibopKind should be kBibopFloatType or kBibopFloat4Type
> > +    // in the future also kBibopDecimalType etc.
> > +    #define bibopKind(a)     ( *(uint8_t*) (uintptr_t(a) & kBibopTypePtrMask) )
> 
> In the name of robustness, may I suggest
> 
> 
> 
> #ifdef _DEBUG
>     REALLY_INLINE uint8_t bibopKind(const Atom& a)
>     {
>         AvmAssert(atomKind(a) == kSpecialBibopType);
>         uint8_t const k = *(uint8_t*)(uintptr_t(a) & kBibopTypePtrMask);
>         AvmAssert(k == kBibopUndefined || k == kBibopFloatType);
>         return k;
>     }
> #else
>     #define bibopKind(a)     ( *(uint8_t*) (uintptr_t(a) &
> kBibopTypePtrMask) )
> #endif

Agreed, fixed.

Comment 11

[Lars T Hansen]

(In reply to comment #10)

> > #ifdef _DEBUG
> >     REALLY_INLINE uint8_t bibopKind(const Atom& a)
> >     {
> >         AvmAssert(atomKind(a) == kSpecialBibopType);
> >         uint8_t const k = *(uint8_t*)(uintptr_t(a) & kBibopTypePtrMask);
> >         AvmAssert(k == kBibopUndefined || k == kBibopFloatType);
> >         return k;
> >     }

Can't be Atom& because many arguments passed to this function are in local variables declared "register" (and they are declared "register" specifically in order to prevent their address from being taken), but that's an easy fix.  Just noting it for the record.

Comment 12

[Tamarin Bot]

changeset: 6417:5e3f637ac914
user:      Lars T Hansen <[e-mail]>
summary:   Fix 626402 - Add floatAllocator (patch=virgilp, r=lhansen, r=stejohns, adjustments=lhansen, push=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/5e3f637ac914

Comment 13

[Steven Johnson]

> in C and C++ sizeof(float) does not need to be 4

No, but in Tamarin they do. Let's add a static_assert to that effect and be done with it.

> Can't be Atom& because many arguments passed to this function are 
> in local variables declared "register"

Hmm, I thought that an inlined function with an input argument declared as "const Atom&" wouldn't require taking an address (ie the compiler could recognize the situation)... but I guess this does leave us at the mercy of dumb compilers.

Comment 14

[Lars T Hansen]

(In reply to comment #13)
> 
> > Can't be Atom& because many arguments passed to this function are 
> > in local variables declared "register"
> 
> Hmm, I thought that an inlined function with an input argument declared as
> "const Atom&" wouldn't require taking an address (ie the compiler could
> recognize the situation)... but I guess this does leave us at the mercy of
> dumb compilers.

Not just dumb compilers, but compilers without certain proprietary extensions.  An "inline" function is not required to be inlined.  Hence we have REALLY_INLINE.  But on some platforms (at least Solaris, last I looked) that is defined simply as "inline".

Comment 15

[Brent Baker]

Lars/Virgil: Is there anything that needs to be done before I can mark this as "Verified"?

Comment 16

[Lars T Hansen]

So far as I know it's done.