607393 - Remove global new (GC*) operator

Status: RESOLVED FIXED

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

Description

[Tommy Reilly]

At this point we've committed to using GCObject and GCFinalizedObject as GC baseclasses.  These provide new operators so the default one can go away.   Also the global one and the GCObject/GCFinalizedOBject ones differ in their args WRT to extra and flags, so we'll probably fix some bugs/avoid future bugs by removing the global one.

Comment 1

[Tommy Reilly]

Attachment: remove global new (GC*)

Hmm, that was easy, we don't actually use it!    I suspect the player does, investigating...

Comment 2

[Tommy Reilly]

Well slap me sideways and paint me orange, the player doesn't use it either.  DUC!

Comment 3

[Tommy Reilly]

AIR builds too, I think we must have gone through the motions of removing this before and just never got around actually removing it.

Comment 4

[Tommy Reilly]

Comment on attachment 486115 [details] [diff] [review]
remove global new (GC*)

lars is out till Friday possibly and Ed loves DUC bugs...

Comment 5

[Edwin Smith]

Comment on attachment 486115 [details] [diff] [review]
remove global new (GC*)

I like deleting dead code, but I like an empty review queue more, and i'm not close enough to know answers to the obvious questions:

* has this passed all flash sandbox tests and ours too?
* are we really sure?  (not my gig).  
* are we 100% certian that GCObject or one of the base classes will be required for all gc allocations, forever?

this can wait for Lars to get around to it.

Comment 6

[Tommy Reilly]

(In reply to comment #5)

> I like deleting dead code, but I like an empty review queue more, and i'm not
> close enough to know answers to the obvious questions:

Well asking questions Lar's might ask anyways will speed things along so its not for nothing.

> * has this passed all flash sandbox tests and ours too?

No its just compiles

> * are we really sure?  (not my gig).  

Are we really sure its dead code?  Its not like you could have some code that was using the global new operator magically work after removing it, the class'es new operators take precedence over global new.

I guess to really prove it you could leave off the impl but leave in the decl in and you'd get link errors .... trying that.   Oops!  My bad!  I should have known this was too good to be true.

Turns out we use it in one place in tamarin in MethodEnv b/c ActivationMethodTablePair doesn't inherit from GCObject, need to fix that first.    I guess placement new took over for new (GC*) with this change.  Oops, that's not what we want ;-)  In fact we probably want to leave in new (GC*) forever and just not define it so people get compile errors if they use it.

> * are we 100% certian that GCObject or one of the base classes will be required
> for all gc allocations, forever?

No this is silly, GC::Alloc and friends are available and in use.   But yes I think we want GCObject and GCFinalizedObject to provide all new operations.    We can now add the possibility for new (gc*) to get mixed up with the global placement new operator to the list of reasons it should go ;-)

Comment 7

[Edwin Smith]

> > * are we 100% certian that GCObject or one of the base classes will be required
> > for all gc allocations, forever?
> 
> No this is silly, GC::Alloc and friends are available and in use.   But yes I
> think we want GCObject and GCFinalizedObject to provide all new operations.   
> We can now add the possibility for new (gc*) to get mixed up with the global
> placement new operator to the list of reasons it should go ;-)

Yep, thats what I mean: all gc "new" operations.  And I was thinking about
examples like ActivationMethodTablePair, I just wasn't sure how many there
were.  (hmm, dehydra...).  I had expected we'd see an increasing number of
cases like this if we deprecated GCObject in general.

Comment 8

[Tommy Reilly]

607340 is needed for a player refactoring that fixes a use of global new in the player so its a blocker.

Comment 9

[Tommy Reilly]

(In reply to comment #7)

> Yep, thats what I mean: all gc "new" operations.  And I was thinking about
> examples like ActivationMethodTablePair, I just wasn't sure how many there
> were.  (hmm, dehydra...).  I had expected we'd see an increasing number of
> cases like this if we deprecated GCObject in general.

In tamarin the only other case is bug 607648.   With safegc and exact tracing I think the ship has sailed on GCObject being optional.

Comment 10

[Lars T Hansen]

I notice I'm not being asked for a review but I'm in favor, R+.

That said:

My main concern with removing it is that I'm afraid there will be a chance of a mix-up if somebody believes it exists: suppose gc is a GC*, and C is /not/ derived from GCObject or GCFinalizedObject, what happens here?

  new (gc) C(...)

Will this cast the GC* to a void* and try to use the GC's memory for the new object, ie, will it invoke the global placement new operator?  If experimentation shows that this could be a problem then the proper initial fix might be to keep the global operators but have them call GCHeap::Abort() after suitably asserting a message about the problem.

In the longer term I suspect that operator new/delete on GCObject and GCFinalizedObject will also be removed, or at least, their use will decrease drastically.  (Once RC is gone, operator delete will likely be turn into a no-op that asserts initially, maybe indefinitely.)  With exact tracing, and optional finalization, the only sane allocation style will be a factory method, which requires no 'new' operator because it relies on GC::Alloc and placement new.

But that has no bearing on this patch :-)

Comment 11

[Tommy Reilly]

Attachment: decl but don't impl global new

Declaring but not implementing global new seems effective for finding uses, obviously this can't land until the uses are nuked (tamarin is clean now, there's like 3 in the player).

Comment 12

[Tommy Reilly]

Doh I have an improved comment for this that is actually a complete sentence :-)

Comment 13

[Steven Johnson]

(In reply to comment #11)
> Declaring but not implementing global new seems effective for finding uses,
> obviously this can't land until the uses are nuked (tamarin is clean now,
> there's like 3 in the player).

+1 for this approach. (Be sure to have a comment in the header explaining it, so offenders can figure out the link error cause)

Comment 14

[Tamarin Bot]

changeset: 5635:c04c6c4c7aaf
user:      Tommy Reilly <[e-mail]>
summary:   Bug 607393 - Remove global new (GC*) operator (r=lhansen)

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