The default bug view has changed. See this FAQ.

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

NEW
Assigned to

Status

Tamarin
Garbage Collection (mmGC)
P2
normal
6 years ago
5 years ago

People

(Reporter: Lars T Hansen, Assigned: Tommy Reilly)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
Q1 12 - Brannan
x86
Mac OS X
Dependency tree / graph
Bug Flags:
flashplayer-qrb +

Details

(Whiteboard: loose-end)

Attachments

(6 attachments, 10 obsolete attachments)

1.00 KB, patch
pnkfelix
: review+
Lars T Hansen
: superreview+
Steven Johnson
: feedback+
Details | Diff | Splinter Review
4.52 KB, patch
Lars T Hansen
: review-
Details | Diff | Splinter Review
2.10 KB, patch
Lars T Hansen
: review+
Details | Diff | Splinter Review
10.08 KB, patch
Lars T Hansen
: review-
Details | Diff | Splinter Review
2.43 KB, patch
Lars T Hansen
: review+
Details | Diff | Splinter Review
7.93 KB, patch
Lars T Hansen
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Depends on: 654934
(Reporter)

Updated

6 years ago
Depends on: 654932
(Assignee)

Comment 1

6 years ago
Created attachment 548458 [details] [diff] [review]
use DataList<uint64_t> for invocation count
Assignee: nobody → treilly
Status: NEW → ASSIGNED
Attachment #548458 - Flags: review?(stejohns)
(Assignee)

Comment 2

6 years ago
Created attachment 548462 [details] [diff] [review]
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

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

Review of attachment 548458 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #548458 - Flags: review?(stejohns) → review+

Comment 4

6 years ago
changeset: 6488:528a23c1afdf
user:      Tommy Reilly <treilly@adobe.com>
summary:   Bug 654946 - Remove GC::Calloc call and use DataList for invocation counts (r=stejohns)

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

Comment 5

6 years ago
Created attachment 550160 [details] [diff] [review]
LeafObject and LeafVector to allow Alloc/Calloc removal
Attachment #550160 - Flags: superreview?(lhansen)
Attachment #550160 - Flags: review?(fklockii)
(Assignee)

Comment 6

6 years ago
Created attachment 550162 [details] [diff] [review]
Make BugCompatibily a leaf object instead of using Alloc
(Assignee)

Comment 7

6 years ago
Created attachment 550163 [details] [diff] [review]
Have TranscodedCode use LeafVector instead of raw Alloc
Attachment #550163 - Flags: review?(lhansen)
(Assignee)

Comment 8

6 years ago
Created attachment 550164 [details] [diff] [review]
Have unscanned Traits** use LeafVector instead of Alloc
Attachment #550164 - Flags: review?(fklockii)
(Assignee)

Updated

6 years ago
Attachment #550162 - Flags: review?(fklockii)
(Assignee)

Updated

6 years ago
Attachment #550162 - Flags: feedback?(stejohns)
(Assignee)

Updated

6 years ago
Attachment #550164 - Flags: feedback?(stejohns)
(Assignee)

Comment 9

6 years ago
Motivation for terminology choice: http://www.memorymanagement.org/glossary/l.html#leaf.object
(Assignee)

Comment 10

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

Updated

6 years ago
Attachment #550162 - Flags: feedback?(stejohns) → feedback+

Updated

6 years ago
Attachment #550164 - Flags: feedback?(stejohns) → feedback+
(Assignee)

Comment 11

6 years ago
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
Attachment #550160 - Flags: superreview?(lhansen)
Attachment #550160 - Flags: review?(fklockii)
(Assignee)

Comment 12

6 years ago
Created attachment 550174 [details] [diff] [review]
LeafObject and LeafVector to replace Alloc/Calloc respectively
Attachment #550160 - Attachment is obsolete: true
Attachment #550174 - Flags: review?
Attachment #550174 - Flags: review? → review?(fklockii)
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-.)
Attachment #550174 - Flags: review?(fklockii) → review-
(Assignee)

Comment 14

6 years ago
CheckForAllocSizeOverflow checks addition
CheckForCallocSizeOverflow checks product
(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?
Attachment #550174 - Flags: review- → review?
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...)
Attachment #550164 - Flags: review?(fklockii) → review+
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.)
Attachment #550174 - Flags: review? → review+
(Assignee)

Comment 18

6 years ago
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 on attachment 550162 [details] [diff] [review]
Make BugCompatibily a leaf object instead of using Alloc

Perfect illustration of utility of this class.
Attachment #550162 - Flags: review?(fklockii) → review+
(Assignee)

Comment 20

6 years ago
Created attachment 550801 [details] [diff] [review]
lock down LeafVector with private ctor and factory method
Attachment #550174 - Attachment is obsolete: true
Attachment #550801 - Flags: review?(fklockii)
(Assignee)

Comment 21

6 years ago
Created attachment 550802 [details] [diff] [review]
use LeafVector for wordcode
Attachment #550163 - Attachment is obsolete: true
Attachment #550163 - Flags: review?(lhansen)
Attachment #550802 - Flags: review?(fklockii)
(Assignee)

Comment 22

6 years ago
Created attachment 550803 [details] [diff] [review]
Use LeafVector for secondary supertypes Traits**
Attachment #550164 - Attachment is obsolete: true
Attachment #550803 - Flags: review?(fklockii)
Attachment #550803 - Flags: feedback?(stejohns)

Comment 23

6 years ago
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?
Attachment #550803 - Flags: feedback?(stejohns) → feedback+
(Assignee)

Comment 24

6 years ago
(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?
(Assignee)

Comment 25

6 years ago
Note that its where it is in avmplus.h because it has to come after the MMgc.h include.
(Assignee)

Updated

6 years ago
Attachment #550162 - Flags: superreview?(lhansen)
(Assignee)

Updated

6 years ago
Attachment #550801 - Flags: superreview?(lhansen)
(Assignee)

Updated

6 years ago
Attachment #550802 - Flags: superreview?(lhansen)
(Assignee)

Updated

6 years ago
Attachment #550803 - Flags: superreview?(lhansen)
(Reporter)

Updated

6 years ago
Attachment #550802 - Flags: superreview?(lhansen) → superreview+
(Reporter)

Updated

6 years ago
Attachment #550162 - Flags: superreview?(lhansen) → superreview+
(Assignee)

Updated

6 years ago
Priority: -- → P2
Target Milestone: --- → Q1 12 - Brannan
(Assignee)

Comment 26

6 years ago
Created attachment 553503 [details] [diff] [review]
LeafObject and LeafVector to replace Alloc/Calloc respectively

Updated with AsArray method to eliminate ugly casting in users of LeafVector
Attachment #550801 - Attachment is obsolete: true
Attachment #550801 - Flags: superreview?(lhansen)
Attachment #550801 - Flags: review?(fklockii)
Attachment #553503 - Flags: review?(lhansen)
(Assignee)

Comment 27

6 years ago
Created attachment 553504 [details] [diff] [review]
Use new AsArray method instead of C style casts
Attachment #550802 - Attachment is obsolete: true
Attachment #550802 - Flags: review?(fklockii)
Attachment #553504 - Flags: review?(lhansen)
(Assignee)

Comment 28

6 years ago
Created attachment 553505 [details] [diff] [review]
v2: Use AsArray instead of c style casts
Attachment #550803 - Attachment is obsolete: true
Attachment #550803 - Flags: superreview?(lhansen)
Attachment #550803 - Flags: review?(fklockii)
Attachment #553505 - Flags: review?(lhansen)
(Assignee)

Comment 29

6 years ago
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

6 years ago
changeset: 6516:c25df57c43d0
user:      Tommy Reilly <treilly@adobe.com>
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

6 years ago
changeset: 6517:62b72549b187
user:      Tommy Reilly <treilly@adobe.com>
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

6 years ago
changeset: 6518:f1629d5bdf84
user:      Tommy Reilly <treilly@adobe.com>
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

6 years ago
changeset: 6519:bfebccd78a06
user:      Tommy Reilly <treilly@adobe.com>
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
(Assignee)

Comment 34

6 years ago
Created attachment 553783 [details] [diff] [review]
use LeafVector for invocationCounts
Attachment #548458 - Attachment is obsolete: true
Attachment #548462 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #553783 - Flags: review?(lhansen)
(Reporter)

Comment 35

6 years ago
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.
Attachment #553503 - Flags: review?(lhansen) → review-
(Reporter)

Updated

6 years ago
Attachment #553504 - Flags: review?(lhansen) → review+
(Reporter)

Comment 36

6 years ago
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?
Attachment #553505 - Flags: review?(lhansen) → review-
(Reporter)

Updated

6 years ago
Attachment #553783 - Flags: review?(lhansen) → review+
(Assignee)

Comment 37

6 years ago
Created attachment 554936 [details] [diff] [review]
Address documentation concerns and enforce AsArray usage (and fix casts in UnscannedTraits)
Attachment #554936 - Flags: review?(lhansen)
(Reporter)

Comment 38

6 years ago
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.)
Attachment #554936 - Flags: review?(lhansen) → review-
(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?)
(Reporter)

Comment 40

6 years ago
(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.
(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.)
[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.]
(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.)
(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.  )
(Reporter)

Comment 45

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

Comment 46

6 years ago
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.
(Assignee)

Comment 47

6 years ago
Created attachment 555717 [details] [diff] [review]
Documentation cleanup and simplify implementation using tail array of 1 technique
Attachment #554936 - Attachment is obsolete: true
Attachment #555717 - Flags: review?(lhansen)
(Assignee)

Updated

6 years ago
Blocks: 682013
(Reporter)

Updated

6 years ago
Attachment #555717 - Flags: review?(lhansen) → review+

Comment 48

6 years ago
changeset: 6543:c2a84c8e480b
user:      Tommy Reilly <treilly@adobe.com>
summary:   Bug 654946 - Clean up LeafObject/LeafVector documentation and enforce AsArray usage (r=lhansen)

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

Comment 49

6 years ago
changeset: 6552:db1a0da8a387
user:      Tommy Reilly <treilly@adobe.com>
summary:   Bug 654946 - Use LeafVector for invocationCounts uint64_t array (r=lhansen)

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

Updated

6 years ago
Blocks: 565664

Updated

6 years ago
Flags: flashplayer-qrb+
Now that LeafVector has landed, someone should go back to the original exercise of making GC::Alloc and GC::Calloc private to MMgc.
Status: ASSIGNED → NEW
Whiteboard: loose-end
You need to log in before you can comment on or make changes to this bug.