617005 - Convert vm over to GCMember remove DWB/DWBRC

Status: RESOLVED FIXED

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

Comment 1

[Lars T Hansen]

Please don't do this without raising the discussion to the full group.  (I'm opposed until further notice, though that's based on the previous realization of this facility.)

Comment 2

[Tommy Reilly]

Can you clarify what "previous realization of this facility" means?   From my previous discussion with you, Edwin and Steven I've only gotten warm reception to the idea of doing away with DRC/DWB/DWBRC and moving to GCMember<T>.     Essentially we're just getting rid of the macros, folding WriteBarrier/WriteBarrierRC together and renaming it.     I didn't anticipate this would be controversial.

Comment 3

[Lars T Hansen]

I guarantee that I never said yes to replacing uses in the VM with GCMember et al.  What I meant by "previous realization of this facility" was that raw pointers were not available to the code; you /had/ to use GCRefs.  I did not, nor do I now, want the VM to be restricted in that way.  The current setup with magic interconversion from/to GCMember should work fine, but what would happen with the VM the day we decide to lock this down and restrict it so that GCRefs must be used?

Comment 4

[Tommy Reilly]

I agree, not talking about GCRef here, just DRC/DWB/DRCWB.    If we ever lock down GCMember to not accept raw pointers that can easily be made to only apply to client code.   Steven's lockdown work will give us a nice way to have enable this via ifdefs.

Comment 5

[Lars T Hansen]

I have a patch for this that I'm currently vetting; my inclination is to land it as soon as it's ready.

Comment 6

[Lars T Hansen]

Attachment: Use GCMember everywhere

This replaces all uses of DRC, DWB, and DRCWB in the Tamarin source and comments with comparable uses of GCMember<>.  The patch also marks those macros as deprecated in the source; we should not be using them.

This patch does not introduce any GCRefs, it's all about the write barrier definitions.

The new GCMember<> smart pointers are equivalent to the smart pointers underlying the macros, so there's little risk here.

Comment 7

[Steven Johnson]

Comment on attachment 520209 [details] [diff] [review]
Use GCMember everywhere

for AbcEnv::m_invocationCounts, why not use DataList<uint64_t>?

Comment 8

[Ruchi Lohani]

Reviewed partially. Should do the rest first thing in the morning.

Q: why GCFinalizedObject::GCMember in FieldName's name member? GCMember wunt work?

Comment 9

[Lars T Hansen]

(In reply to comment #8)
> Reviewed partially. Should do the rest first thing in the morning.
> 
> Q: why GCFinalizedObject::GCMember in FieldName's name member? GCMember wunt
> work?

GCMember is always an internal class of some GC class (eg GCObject), so derived classes of the GC class can use GCMember.  But FieldName is not a subclass of any of the GC classes, nor can it be - it's an array element, and the array itself is the subclass of the GC class.

Arguably GCMember breaks down in this circumstance, as do many of the other GC facilities - arrays are tricky.

Anyway, by using the one from GCFinalizedObject I'm trying to work around it in the same way that DRCWB was used before even though FieldName wasn't a subclass of any GC class then either.

Comment 10

[Ruchi Lohani]

Few comments:
1. We probably wont need the cast if this patch goes after the GCMemberbase copy ctor patch lands from bug #642455 ? 

2. in traits.h write barriers are missing for these: 
public:     PoolObject* const       GC_POINTER(pool);  
public:     Traits*                 GC_POINTER(itraits); 
Do we know why?

Rest all looks fine to me.

Comment 11

[Steven Johnson]

(2) not necessary as the lifespan of self is >= both of those. but not harmful either, probably better to add them to avoid copy-paste errors

Comment 12

[Lars T Hansen]

(In reply to comment #10)
> Few comments:
> 1. We probably wont need the cast if this patch goes after the GCMemberbase
> copy ctor patch lands from bug #642455 ? 

That's right.

> 2. in traits.h write barriers are missing for these: 
> public:     PoolObject* const       GC_POINTER(pool);  
> public:     Traits*                 GC_POINTER(itraits); 
> Do we know why?

I don't, but I will investigate and add comments re my findings.  (Every "missing" write barrier or smart pointer should have a comment explaining why.  It's possible every missing write barrier should be a GCMemberNoWB<> smart pointer where the second argument to the template is a string describing why, on the other hand, that could be pretty annoying :-)

Comment 13

[Steven Johnson]

(In reply to comment #12)

> It's possible every missing write barrier should be a GCMemberNoWB<> smart
> pointer where the second argument to the template is a string describing why,
> on the other hand, that could be pretty annoying :-)

Good, it *should* be annoying to do such a thing. (Not sure I vote for that particular mechanism but you get the point...)

Comment 14

[Tamarin Bot]

changeset: 6127:d33ac05c5d93
user:      Lars T Hansen <[e-mail]>
summary:   Fix 617005 - Convert vm over to GCMember remove DWB/DWBRC (r=rulohani, r=stejohns)

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

Comment 15

[Steven Johnson]

Attachment: Back out the nativegen.py changes for now

We have to back out this portion of the change for now; the problem is that in some contexts, GCMember<> requires a full declaration of the class, not merely a forward declaration, and the Flash headers haven't been properly scrubbed to allow for this. Rather than stall the merge while this potentially lengthy task completes, let's defer this one till next time.

Comment 16

[Steven Johnson]

Comment on attachment 522793 [details] [diff] [review]
Back out the nativegen.py changes for now

pushed to TR in anticipation of R+ as 6134:f67494deaa6f, to unblock the merge

Comment 17

[Lars T Hansen]

Comment on attachment 522793 [details] [diff] [review]
Back out the nativegen.py changes for now

SR+ because it's necessary, but this makes me unhappy.

Comment 18

[Steven Johnson]

I'm not happy about it either, but untangling the Flash header business will be a lot simpler if done as a standalone task.

Comment 19

[Lars T Hansen]

Moved the revert issue and the patch to bug #647972, closing this one.