Closed Bug 1084421 Opened 5 years ago Closed 5 years ago

Only Events with a wrapper should trigger GC more likely

Categories

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

34 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Attached patch wipSplinter Review
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 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().
Attachment #8506965 - Flags: review?(continuation)
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.
Attachment #8506965 - Flags: review?(continuation) → review+
(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
(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.
https://hg.mozilla.org/mozilla-central/rev/cde524d5ce64
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.