1084421 - Only Events with a wrapper should trigger GC more likely

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

We could possibly trigger GC a bit less often if we didn't call
LikelyShortLivingObjectCreated for every Event.

(In the patch I explicitly decided to change only Event classes, not Touch.)

Still doing some testing...

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=00022e3a9e2b

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8506965 [details] [diff] [review]
wip

Oh, this compiles even on b2g.

So the idea is that we call nsJSContext::LikelyShortLivingObjectCreated();
only when a wrapper is about to be created.
(It is a very rare case that a wrapper is GCed and recreated for an Event.)
Since I want LikelyShortLivingObjectCreated() call to live only in one place,
WrapObject() is implemented in the Event class which is the base class for all the events, and other classes are supposed to implement WrapObjectInternal().

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8506965 [details] [diff] [review]
wip

Review of attachment 8506965 [details] [diff] [review]:
-----------------------------------------------------------------

Neat!

"Since I want LikelyShortLivingObjectCreated() call to live only in one place"
Out of curiosity, why is that?

::: dom/bindings/Codegen.py
@@ +12810,5 @@
>  class CGBindingImplClass(CGClass):
>      """
>      Common codegen for generating a C++ implementation of a WebIDL interface
>      """
> +    def __init__(self, descriptor, cgMethod, cgGetter, cgSetter, wantGetParent=True, wrapMethodPostFix=""):

Could you change wrapMethodPostFix to be wrapMethodName with a default of "WrapObject"?  I think it is a little harder to figure out what is going on to split it up like that, and it feels to me like other possible users of thing might want to come up with their own name, and not necessarily one that starts with Wrap.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> "Since I want LikelyShortLivingObjectCreated() call to live only in one
> place"
> Out of curiosity, why is that?
I don't want people implementing new *Event classes to be forced to remember that
one should call LikelyShortLivingObjectCreated() in WrapObject..
The MOZ_* annotations make sure that one has to implement WrapObjectInternal

> Could you change wrapMethodPostFix to be wrapMethodName with a default of
> "WrapObject"? 
Sure

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #4)
> I don't want people implementing new *Event classes to be forced to remember
> that one should call LikelyShortLivingObjectCreated() in WrapObject..
> The MOZ_* annotations make sure that one has to implement WrapObjectInternal
Ah, that's a good idea.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cde524d5ce64

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cde524d5ce64