Remove global new (GC*) operator

RESOLVED FIXED in Q3 11 - Serrano

Status

Tamarin
Garbage Collection (mmGC)
P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Tommy Reilly, Assigned: Tommy Reilly)

Tracking

(Blocks: 1 bug)

unspecified
Q3 11 - Serrano
x86
All
Dependency tree / graph
Bug Flags:
flashplayer-bug -

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Updated

7 years ago
Blocks: 565664
(Assignee)

Comment 1

7 years ago
Created attachment 486115 [details] [diff] [review]
remove global new (GC*)

Hmm, that was easy, we don't actually use it!    I suspect the player does, investigating...
Attachment #486115 - Flags: review?(lhansen)
(Assignee)

Comment 2

7 years ago
Well slap me sideways and paint me orange, the player doesn't use it either.  DUC!
(Assignee)

Comment 3

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

Comment 4

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

lars is out till Friday possibly and Ed loves DUC bugs...
Attachment #486115 - Flags: review?(lhansen) → review?(edwsmith)

Comment 5

7 years ago
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.
Attachment #486115 - Flags: review?(edwsmith)
(Assignee)

Updated

7 years ago
Attachment #486115 - Flags: review?(lhansen)
(Assignee)

Comment 6

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

Updated

7 years ago
Attachment #486115 - Flags: review?(lhansen)
(Assignee)

Updated

7 years ago
Depends on: 607647
(Assignee)

Updated

7 years ago
Depends on: 607648

Comment 7

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

Updated

7 years ago
Depends on: 607340
(Assignee)

Comment 8

7 years ago
607340 is needed for a player refactoring that fixes a use of global new in the player so its a blocker.
Blocks: 607340
No longer depends on: 607340
(Assignee)

Comment 9

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

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

Comment 11

7 years ago
Created attachment 486601 [details] [diff] [review]
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).
Attachment #486115 - Attachment is obsolete: true
Attachment #486601 - Flags: review?(lhansen)

Updated

7 years ago
Attachment #486601 - Flags: review?(lhansen) → review+
(Assignee)

Comment 12

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

Comment 13

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

Updated

7 years ago
Blocks: 607651
(Assignee)

Updated

7 years ago
No longer blocks: 607651

Comment 14

7 years ago
changeset: 5635:c04c6c4c7aaf
user:      Tommy Reilly <treilly@adobe.com>
summary:   Bug 607393 - Remove global new (GC*) operator (r=lhansen)

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

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Flags: flashplayer-bug-
You need to log in before you can comment on or make changes to this bug.