654946 - Make GC::Alloc and GC::Calloc private to MMgc

Status: RESOLVED WONTFIX

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

Description

[Lars T Hansen]

Client code should use managed array types instead, see bug #654945.  Locking down the Alloc/Calloc APIs will make it easier to introduce changes for SafeGC and ExactGC, at least.

GC::AllocDouble can continue to be public, as will GC::AllocFloat and maybe others.

Comment 1

[Tommy Reilly]

Attachment: use DataList<uint64_t> for invocation count

Comment 2

[Tommy Reilly]

Attachment: Complete patch showing generated file changes and new template instantiation

Leaving review on simpler patch but this one is what will be pushed.

Comment 3

[Steven Johnson]

Comment on attachment 548458 [details] [diff] [review]
use DataList<uint64_t> for invocation count

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

Comment 4

[Tamarin Bot]

changeset: 6488:528a23c1afdf
user:      Tommy Reilly <[e-mail]>
summary:   Bug 654946 - Remove GC::Calloc call and use DataList for invocation counts (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/528a23c1afdf

Comment 5

[Tommy Reilly]

Attachment: LeafObject and LeafVector to allow Alloc/Calloc removal

Comment 6

[Tommy Reilly]

Attachment: Make BugCompatibily a leaf object instead of using Alloc

Comment 7

[Tommy Reilly]

Attachment: Have TranscodedCode use LeafVector instead of raw Alloc

Comment 8

[Tommy Reilly]

Attachment: Have unscanned Traits** use LeafVector instead of Alloc

Comment 9

[Tommy Reilly]

Motivation for terminology choice: http://www.memorymanagement.org/glossary/l.html#leaf.object

Comment 10

[Tommy Reilly]

Opaque/PointerFree/Untraced/Scalar were candidates dismissed, selection done by Felix/Tom in a closed door IM session ;-)

Comment 11

[Tommy Reilly]

Comment on attachment 550160 [details] [diff] [review]
LeafObject and LeafVector to allow Alloc/Calloc removal

LeafObject/LeafVector need the WriteBarrier,WriteBarrier_dtor methods so other classes can contain pointers to a Leaf with GCMember but Leaf shouldn't have its own GCMember definition.  Will fix and repost

Comment 12

[Tommy Reilly]

Attachment: LeafObject and LeafVector to replace Alloc/Calloc respectively

Comment 13

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

Comment on attachment 550174 [details] [diff] [review]
LeafObject and LeafVector to replace Alloc/Calloc respectively

>+    template<class T, GC::AllocFlags flags=GC::kNoFlags>
>+    class LeafVector : public LeafObject
>+    {
>+    public:
>+        // 'throw()' annotation to avoid GCC warning: 'operator new' must not return NULL unless it is declared 'throw()' (or -fcheck-new is in effect)
>+        static void *operator new(size_t size, GC *gc, size_t count) GNUC_ONLY(throw())
>+        {
>+            size_t bytes = GCHeap::CheckForAllocSizeOverflow(size,
>+                                                             GCHeap::CheckForCallocSizeOverflow(count, sizeof(T)));
>+            return gc->Alloc(bytes, flags & GC::kZero);
>+        }
>+        

I am confused by the bytes calculation above.

CheckForAllocSizeOverflow(A,B) returns the product A*B if it does not overflow, and otherwise signals overflow.

Consider the expression (adapted from the superlist patch):
  (Traits**) new (GC, SZ+1) LeafVector<Traits*, GC::kZero>()
when this is converted to an operator new invocation, I believe:

  size will be sizeof(LeafVector<T*, GC::kZero>) which is probably 1 since neither LeafVector nor LeafObject have any data members (but must be nonzero due to C++ rules, IIRC).

  sizeof(T) will be sizeof(Traits*)

  count will be sz+1.

So the bytes calculation, assuming no overflow, will yield size*(count*sizeof(T)) == 1*((sz+1)*sizeof(Traits*).

This probably runs correctly, but I don't understand the motivation for the outer-most product.  If anything, I would have expected an *addition*: size+(count*sizeof(T)), to account for any data members that LeafObject or LeafVector acquire.

Can you explain?  Do I misunderstand?  (Either way I think a comment is in order, so R-.)

Comment 14

[Tommy Reilly]

CheckForAllocSizeOverflow checks addition
CheckForCallocSizeOverflow checks product

Comment 15

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

(In reply to comment #14)
> CheckForAllocSizeOverflow checks addition
> CheckForCallocSizeOverflow checks product

Oh those pesky similar looking method names, I thought both calls with CheckForCallocSizeOverflow.  Okay let me reset my status to R?

Comment 16

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

Comment on attachment 550164 [details] [diff] [review]
Have unscanned Traits** use LeafVector instead of Alloc

Seeing this patch makes me realize that LeafVector (and thus LeafObject) can never carry any data members (or virtual methods for that matter), since the cast here would go horribly awry otherwise, yes?  I don't think there's any way to actually assert that constraint, unfortunately.

Anyway I guess this works, (I am assuming that the extra byte being allocated from the LeafVector::operator new() won't introduce any problems with data alignment rules, an area of C/C++ that I try to stay away from...)

Comment 17

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

Comment on attachment 550174 [details] [diff] [review]
LeafObject and LeafVector to replace Alloc/Calloc respectively



>+    template<class T, GC::AllocFlags flags=GC::kNoFlags>
>+    class LeafVector : public LeafObject
>+    {
>+    public:
>+        // 'throw()' annotation to avoid GCC warning: 'operator new' must not return NULL unless it is declared 'throw()' (or -fcheck-new is in effect)
>+        static void *operator new(size_t size, GC *gc, size_t count) GNUC_ONLY(throw())
>+        {
>+            size_t bytes = GCHeap::CheckForAllocSizeOverflow(size,
>+                                                             GCHeap::CheckForCallocSizeOverflow(count, sizeof(T)));
>+            return gc->Alloc(bytes, flags & GC::kZero);
>+        }

In light of comment 13, comment 14 and comment 16, I am suspicious of leaving this formula using the sum of size and count*sizeof(T).  There are two concerns:

1. it will over-allocate by one byte on every allocation of a LeafVector

2. it fools the reader into thinking that the size could somehow represent meaningful state being allocated (e.g. one looking at this operator new and the class definitions for LeafObject and LeafVector may think that it is reasonable to add data members or virtual methods to LeafObject/LeafVector, when of course the newly added code in attachment 550164 [details] [diff] [review] makes it clear that doing so would be completely wrong.

I suggest you leave out the size entirely from the calculation, along with a comment in operator new saying that it cannot be meaningful.

If for some reason we need to ensure that never invoke gc->Alloc with bytes==0, then you could use max(1, GCHeap::CheckForCallocSizeOverflow(count, sizeof(T)).  But still leave size (even though it ends up being 1) out of the calculation to avoid confusion.

----

I said in comment 16 that there was no way to assert the constraint that LeafObject/LeafVector carry no payload; but I now realize this is not entirely true.  In other places where we have this pattern of an object with a variable-sized payload, we mark it with a member that is a single element array, i.e.
  T data[1];

(One even sees this same pattern in the old code for attachment 550163 [details] [diff] [review].)

That might seem absurd in this case (since the payload here is the *only* member and thus there should be no need to name it.)  But if we were to do this in this case, then a static assertion can be added to check that sizeof(LeafVector<T> == sizeof(T)).  We would in effect be asserting that LeafObject/LeafVector cannot carry any *additional* payload beyond what is being added on by LeafVector::operator new().

(And also the data member signals an instance of the variable-length payload allocation pattern, so using it might ease reading the code.)

To be honest though, I don't see a significant engineering advantage to adding the data[1] member in this case.  It was just something that came to mind as I worked through the issues when writing the suggestion above.

----

Tangential Aside:

>+    class LeafObject
>+    {
...
>+        // These are noops overridden by RCObject to do the right thing.
>+        REALLY_INLINE void IncrementRef() {}
>+        REALLY_INLINE void DecrementRef() {}

This comment above these two methods confused me at first since RCObject does not extend LeafObject, but I get the point; the methods are hooks that are called from the methods of RCPtr<T>, where T can be instantiated with LeafObject or one of its subclasses.  In other words we're talking about template-polymorphism, not subclass-polymorphism or prototype-dispatch-polymorphism, and I'm just used to seeing the word "overridden" reserved for the latter.

(Of course, the same comment applies to the same three line pattern that occurred already in GCObject; in that case I always overlooked it for some reason.)

Comment 18

[Tommy Reilly]

I wanted to leave the door open for LeafVector to be the base class for a trailing T t[1] style class, I'm still working through how LeafVector will be used in the player.  Agree that a switch from Calloc to LeafVector shouldn't require any more memory than before, will fix.

Comment 19

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

Comment on attachment 550162 [details] [diff] [review]
Make BugCompatibily a leaf object instead of using Alloc

Perfect illustration of utility of this class.

Comment 20

[Tommy Reilly]

Attachment: lock down LeafVector with private ctor and factory method

Comment 21

[Tommy Reilly]

Attachment: use LeafVector for wordcode

Comment 22

[Tommy Reilly]

Attachment: Use LeafVector for secondary supertypes Traits**

Comment 23

[Steven Johnson]

Comment on attachment 550803 [details] [diff] [review]
Use LeafVector for secondary supertypes Traits**

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

::: core/avmplus.h
@@ +242,5 @@
>  #include "MMgc.h"
> +
> +namespace avmplus
> +{
> +    typedef MMgc::LeafVector<Traits*, MMgc::GC::kZero> UnscannedTraitsArray;

This seems like a really weird place to declare this type. Surely it could go somewhere else?

Comment 24

[Tommy Reilly]

(In reply to Steven Johnson from comment #23)
> This seems like a really weird place to declare this type. Surely it could
> go somewhere else?

Its used by Traits.h and AvmCore.h (for emptySupertypeList).  Would AvmCore.h be preferable?

Comment 25

[Tommy Reilly]

Note that its where it is in avmplus.h because it has to come after the MMgc.h include.

Comment 26

[Tommy Reilly]

Attachment: LeafObject and LeafVector to replace Alloc/Calloc respectively

Updated with AsArray method to eliminate ugly casting in users of LeafVector

Comment 27

[Tommy Reilly]

Attachment: Use new AsArray method instead of C style casts

Comment 28

[Tommy Reilly]

Attachment: v2: Use AsArray instead of c style casts

Comment 29

[Tommy Reilly]

Lars in case you're confused I pinged you on a re-review because I added AsArray to clean things up and remove C cast from code using LeafVector, that's the only change.

Comment 30

[Tamarin Bot]

changeset: 6516:c25df57c43d0
user:      Tommy Reilly <[e-mail]>
summary:   Bug 654946 - Make GC::Alloc and GC::Calloc private to MMgc, introduce !kContainsPointer LeafObject/LeafVector (r=fklockii,sr=lhansen)

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

Comment 31

[Tamarin Bot]

changeset: 6517:62b72549b187
user:      Tommy Reilly <[e-mail]>
summary:   Bug 654946 - Make GC::Alloc and GC::Calloc private to MMgc, use LeafObject for BugCompatability (r=fklockii,r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/62b72549b187

Comment 32

[Tamarin Bot]

changeset: 6518:f1629d5bdf84
user:      Tommy Reilly <[e-mail]>
summary:   Bug 654946 - Make GC::Alloc and GC::Calloc private to MMgc, use LeafVector for word code (r=fklockii,sr=lhansen)

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

Comment 33

[Tamarin Bot]

changeset: 6519:bfebccd78a06
user:      Tommy Reilly <[e-mail]>
summary:   Bug 654946 - Make GC::Alloc and GC::Calloc private to MMgc, use LeafVector for Traits** (r=fklockii,sr=lhansen,f=stejohns)

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

Comment 34

[Tommy Reilly]

Attachment: use LeafVector for invocationCounts

Comment 35

[Lars T Hansen]

Comment on attachment 553503 [details] [diff] [review]
LeafObject and LeafVector to replace Alloc/Calloc respectively

The comment ahead of LeafVector was already unreasonable because it's discursive and does not serve as documentation; documentation here should state what's the case and what the API is.  Worse is that with AsArray the doc block is misleading.

Obviously the API methods like AsArray need serious documentation.

Second, I would really like to not depend on the array class starting on address zero of the LeafArray object, because I fear this will lock us into bad practices quickly.  So that should not be documented as working, and in debug builds I really think we should insert some guards at the beginning to ensure that no code comes to depend on it (check it on destruction).

The documentation above LeafObject could use some work too: it needs to be clear that LeafObject can be used instead of GCObject.

I know the patch has landed, but please take the time to rework and resubmit the documentation for review, and again, strong suggestion from me that there's no equivalence between the object and the array it contains, in terms of starting address.

Comment 36

[Lars T Hansen]

Comment on attachment 553505 [details] [diff] [review]
v2: Use AsArray instead of c style casts

There are casts here from UnscannedTraitsArray* to Traits**, should those not be uses of AsArray instead?

Comment 37

[Tommy Reilly]

Attachment: Address documentation concerns and enforce AsArray usage (and fix casts in UnscannedTraits)

Comment 38

[Lars T Hansen]

Comment on attachment 554936 [details] [diff] [review]
Address documentation concerns and enforce AsArray usage (and fix casts in UnscannedTraits)

Nits in LeafVector documentation:

 - "of space needed checking for overflow" either needs a comma or "while"
   after "needed"
 - "not not"
 - "a template parameter" should instead be "the template parameter 
   MMgc::GC::kZero", as that is more helpful.
 - Inside operator new, "LeafVectors" is plural (without apostrophe as you
   write it).

Bugs in LeafVector:

 - All MMgc poison values are kept in GCHeap.h (search for "Poison values
   used by MMgc"); this should be there too.  Usually it's kept there as a
   32-bit value, if a byte is desired, as here, then just the low byte is
   used during poisoning and checking.

 - The allocation code in New does not strike me as correct.  Since LeafVector
   is not a POD-type there can be padding here and there, so the amount you
   allocate must really be sizeof(LeafVector<T,flags>) + count*eltsize.

So R- again, but chiefly for the issue around New, the rest are just things that can be mopped up on landing.

(Speaking of mopping up, it's high time we get GCObject-inlines.h.)

Comment 39

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

(In reply to Lars T Hansen from comment #38)
> Comment on attachment 554936 [details] [diff] [review]
>  - The allocation code in New does not strike me as correct.  Since
> LeafVector
>    is not a POD-type there can be padding here and there, so the amount you
>    allocate must really be sizeof(LeafVector<T,flags>) + count*eltsize.

Hmm.  I guess that is a counter to my argument for leaving the size out in comment 17.

(Would making LeafVector not extend LeafObject qualify it as POD-type?  Or do we just accept the extra unused size?)

Comment 40

[Lars T Hansen]

(In reply to Felix S Klock II from comment #39)

> Would making LeafVector not extend LeafObject qualify it as POD-type?

I doubt that very much, since the thing has methods.  POD types are quite restricted, IIRC.

It seems to me that the safe way of handling this is the way we're handling it everywhere: by having a T[] at the end of the class, and then over-allocate by (n-1)*sizeof(T); if the size of the array is zero then we waste a little data but this pretty much never happens.  That way we know the correct starting address of the array data, the padding and alignment will always be correct, and everyone will be reasonably happy.  For the DEBUG case we would overallocate by one element and return the address of element 1 from AsArray().

We should document somewhere that Size()/sizeof(T) is not an accurate way of getting the number of elements in the array.

Comment 41

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

(In reply to Lars T Hansen from comment #40)
> (In reply to Felix S Klock II from comment #39)
> 
> > Would making LeafVector not extend LeafObject qualify it as POD-type?
> 
> I doubt that very much, since the thing has methods.  POD types are quite
> restricted, IIRC.

(aside: LeafVector and LeafObject have methods, but no virtual methods.  i was more concerned about whether templates possibly interacted in some weird way with POD-ness; but I do not want to take the time right now to pore over the C++ spec.)

Comment 42

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

[update to my last note: the user-declared constructor in LeafVector would also be a problem for POD-ness.  Which is ironic, since the (private) constructor just there to prevent sub-classing.]

Comment 43

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

(In reply to Lars T Hansen from comment #40)
> It seems to me that the safe way of handling this is the way we're handling
> it everywhere: by having a T[] at the end of the class, and then
> over-allocate by (n-1)*sizeof(T); if the size of the array is zero then we
> waste a little data but this pretty much never happens.  That way we know
> the correct starting address of the array data, the padding and alignment
> will always be correct, and everyone will be reasonably happy.  For the
> DEBUG case we would overallocate by one element and return the address of
> element 1 from AsArray().

Tommy: Lars is fine with the minor wastage, so I retract my objection from comment 17.

(The only remaining concern from there was about hypothetical changes to LeafObject/LeafVector, but hopefully future maintenance won't make such mistakes.)

Comment 44

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

(I should not have implied that the wastage that I was objecting to is the same as the wastage that Lars is fine with; I think the code I was reviewing back in comment 17 had more wastage than what we would get by following the strategy that Lars outlined.  )

Comment 45

[Lars T Hansen]

For the record, any wastage is fine with me as long as it's /modest/ wastage :-)

Comment 46

[Tommy Reilly]

Yeah we've ignored size because its 1 when LeafVector has no data members and I thought adding in 1 was silly.   If we go the T elements[1] route that problem goes away.   Originally I think I thought maybe LeafVector would be subclassable and avoided the tail allocation for that reason but I think the API is better as a locked down final class so that's not a concern either.

Comment 47

[Tommy Reilly]

Attachment: Documentation cleanup and simplify implementation using tail array of 1 technique

Comment 48

[Tamarin Bot]

changeset: 6543:c2a84c8e480b
user:      Tommy Reilly <[e-mail]>
summary:   Bug 654946 - Clean up LeafObject/LeafVector documentation and enforce AsArray usage (r=lhansen)

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

Comment 49

[Tamarin Bot]

changeset: 6552:db1a0da8a387
user:      Tommy Reilly <[e-mail]>
summary:   Bug 654946 - Use LeafVector for invocationCounts uint64_t array (r=lhansen)

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

Comment 50

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

Now that LeafVector has landed, someone should go back to the original exercise of making GC::Alloc and GC::Calloc private to MMgc.

Comment 51

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 52

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.