624402 - Allow tracers for substructures to be generated

Status: RESOLVED FIXED

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

Description

[Lars T Hansen]

It wouldn't be very hard to support generation of tracers for in-line structures, like LookupCache in MethodEnv.h, with a structure along the lines of "GC_CPP_STRUCT(name)".  Presumably more of these structures will be found in the Flash Player than in Tamarin, where they are few.

Comment 1

[Tommy Reilly]

Attachment: GC_CPP_EXACT_STRUCT

Comment 2

[Tommy Reilly]

Attachment: GC_CPP_EXACT_STRUCT

Comment 3

[Tommy Reilly]

So if we just make the cursor argument to gcTrace optional then we don't need GC_CPP_EXACT_STRUCT as GC_STRUCTURE's gcTrace(gc) call will work.   But the structure would have to subclass GCTraceableObject but I think we want them to subclass GCTraceableBase (just need to add GCTraceableBase to the non-interesting baseclass list).   Thoughts?   We'd needlessly be adding a virtual but the only way to avoid it is to split GC_DATA_BEGIN up into separate macros.

These structures typically need to subclass something to get GCMember, usually that's GCObject.

Comment 4

[Tommy Reilly]

Attachment: Absolute minimum needed for sub structure trace gen

This works, should we bother to do more?

Comment 5

[Tommy Reilly]

Here's a sleazy thought, the virtual keyword isn't required in subclasses, we could drop it from the decl introduced by GC_DATA_BEGIN macro and introduce a new sub structure base class to introduce the GCMember stuff but no vtable.

Comment 6

[Tommy Reilly]

Attachment: GCStructure

Separate this out, we want it regardless of how we decide to to exact tracing of structures.

Comment 7

[Tommy Reilly]

Attachment: include GCStructure in noUseful tracer and relax gcTrace virtual requirement

If you combine this patch with the others you end up with a nice way to exactly trace structures maximizing re-use of our existing facilities.

Comment 8

[Tommy Reilly]

Attachment: All patches rolled together

Comment 9

[Tommy Reilly]

In some cases a class is used as a structure and sometimes its heap allocated.   This patch allows us to use GC_STRUCTURE and GC_POINTER to handle both cases without having to refactor anything.

Comment 10

[Lars T Hansen]

Comment on attachment 515617 [details] [diff] [review]
All patches rolled together

No way is this all of it, so R- just for that.

I'm not real happy about removing "virtual", though I understand the reasoning so I probably won't put up too much of a fuss.  There needs to be adequate commenting around GC_DECLARE_EXACT_METHODS obviously.

I don't understand the need for GCStructure at all.  All I thought was necessary was a simple GC_SUBCLASS_EXACT or similar annotation which together with GC_DATA_BEGIN/GC_DATA_END would do the trick.

Comment 11

[Lars T Hansen]

I also don't like the optionality of the cursor argument; that argument is not optional nor is the status flag return optional in a reasonable way.  The more I think about it this strikes me as a bundle of hacks just to avoid introducing GC_SUBCLASS_EXACT.

Comment 12

[Tommy Reilly]

This is all of it, what's missing?    I thought it was rather elegant in its simplicity and reuse.

I started out with a new GC_CPP_EXACT_STRUCT tag but realized later it added no value.   This tag just signifiies that we should generate the tracer for the class, I don't see why structures need there own tag.  It wouldn't help with solving the virtual or optional cursor issues since its the GC_DATA_BEGIN macro that outputs the function decl.

GCStructure is independent of tracing and solves this bug https://bugzilla.mozilla.org/show_bug.cgi?id=622815, basically it allows us to distinguish between to expose GCMember to structures.   Drive by bug fixing, I can split it out if you'd like.   The class I needed this for uses GCMember and subclasses GCObject at the moment.

I thought my comments added to GC_DECLARE_EXACT_METHODS were adequate, can you help me understand why they aren't?

I thought hard about the optional argument thing, I didn't like it either.  Couldn't think of a way to eliminate it.

I considered putting together a patch that introduces a GC_CPP_EXACT_STRUCT in conjunction with a GC_DATA_BEGIN_STRUCT and that introduces a void gcTrace(GC*) decl that matches what GC_STRUCTURE calls but it seemed like making the user interface more complicated for a slightly cleaner implementation was a bad compromise, the hackiness of dropping the virtual and having an optional cursor argument seemed worth keeping the user facing stuff simple.   No one ever calls or declares gcTrace directly.

And really the thing that clinched it for me was that a couple classes in the player are used as structures and as individual heap allocations and keeping a unified gcTrace protocol made that possible.   Splitting the protocol will force refactorings or the use of custom tracers.

Comment 13

[Tommy Reilly]

Attachment: All existing mechanism to work on structures

With this patch you can just do GC_CPP_EXACT(Foo, GCStructure) and use GC_STRUCTURE to decorate instances in the class containing the structure and that's it.   Broke out the GCStructure bit into another bug.   Note that this unifies the gcTrace prototcol so that you could also have GC_CPP_EXACT(Bar, GCTraceableObject) and Bar could be used as a structure inlined and as a separate heap allocation.

Comment 14

[Lars T Hansen]

(In reply to comment #12)
> And really the thing that clinched it for me was that a couple classes in the
> player are used as structures and as individual heap allocations and keeping a
> unified gcTrace protocol made that possible.   Splitting the protocol will
> force refactorings or the use of custom tracers.

As a general remark (unrelated to this patch) that simply has to be fixed; we want to be on the path toward a state where if you're allocating something on the GC'd heap then that thing is subclassed from GCObject/GCFinalizedObject/etc, and if you allocate it elsewhere then it never is so subclassed.  The current situation where GCObject is optional is acceptable only because it's necessary for conservatively scanned array allocations.

Comment 15

[Lars T Hansen]

Comment on attachment 516595 [details] [diff] [review]
All existing mechanism to work on structures

I am extremely apprehensive about this - I do not like this kind of punning in fundamental matters, I much prefer to expend additional information on writing out what it is that I mean - but I'm going to let it pass.  So consider this an R+ for the code.

I'm however going to R- the patch because there are no documentation updates to go into the cookbook or manual.

Comment 16

[Tommy Reilly]

(In reply to comment #14)
> As a general remark (unrelated to this patch) that simply has to be fixed; we
> want to be on the path toward a state where if you're allocating something on
> the GC'd heap then that thing is subclassed from
> GCObject/GCFinalizedObject/etc, and if you allocate it elsewhere then it never
> is so subclassed.  The current situation where GCObject is optional is
> acceptable only because it's necessary for conservatively scanned array
> allocations.

+1 I'll log bugs for these when I come across them.   GCSubStructure prevents new usage, is there a way to prevent GCObject and friends being used as structures?

Comment 17

[Tommy Reilly]

Attachment: doc updates, will fold before pushing

Comment 18

[Lars T Hansen]

exactgc-manual.html:

Ungrammatical: "If an object contains GC member ...".

Even if it said "a GC member ..." I don't know what it would mean; needs clarification.

End of same paragraph, I don't understand what you're trying to say: " ...  and where it must be decorated with the GC_STRUCTURE annotation."

Comment 19

[Tamarin Bot]

changeset: 6040:f82148513a32
user:      Tommy Reilly <[e-mail]>
summary:   Bug 624402 - Allow tracers for substructures to be generated (r=lhansen)

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

Comment 20

[Tommy Reilly]

I went with:

"If a class contains GC pointers and isn't allocated directly"

Better?

I also fixed the bottom and added an anchor to link to the example.

Comment 21

[Lars T Hansen]

(In reply to comment #20)
> I went with:
> 
> "If a class contains GC pointers and isn't allocated directly"
> 
> Better?

Sounds good.

Comment 22

[Lars T Hansen]

Can this be closed then?  Yay.

Comment 23

[Tommy Reilly]

I left it open for you to r+ or offer more feedback.

Comment 24

[Lars T Hansen]

Alright then.

Comment 25

[Tommy Reilly]

Attachment: s/GCSubStructure/GCInlineObject

sits on top of hashtable atoms patch

Comment 26

[Tamarin Bot]

changeset: 6052:9ad4c4423ad4
user:      Tommy Reilly <[e-mail]>
summary:   Bug 624402 - naming upgrade s/GCSubStructure/GCInlineObject (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/9ad4c4423ad4