Last Comment Bug 702036 - JSEventListener could have weak mTarget to reduce CC overhead
: JSEventListener could have weak mTarget to reduce CC overhead
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 All
: -- normal (vote)
: mozilla10
Assigned To: Olli Pettay [:smaug]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 702032
  Show dependency treegraph
 
Reported: 2011-11-12 12:00 PST by Olli Pettay [:smaug]
Modified: 2011-12-30 02:27 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.26 KB, patch)
2011-11-12 12:26 PST, Olli Pettay [:smaug]
bzbarsky: review+
continuation: feedback+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review
nsJSEventListener after patch (25.06 KB, application/pdf)
2011-11-12 16:05 PST, Andrew McCreight [:mccr8]
no flags Details
patch (8.33 KB, patch)
2011-11-29 01:48 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
beta patch with comment change (8.38 KB, patch)
2011-12-29 15:22 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2011-11-12 12:00:29 PST

    
Comment 1 Olli Pettay [:smaug] 2011-11-12 12:26:29 PST
Created attachment 574081 [details] [diff] [review]
patch
Comment 2 Olli Pettay [:smaug] 2011-11-12 12:28:12 PST
https://tbpl.mozilla.org/?tree=Try&rev=8111e9aff59e
Comment 3 Andrew McCreight [:mccr8] 2011-11-12 12:42:41 PST
Comment on attachment 574081 [details] [diff] [review]
patch

Looks good to me, from the perspective of removing things from the CC graph.  Thanks!
Comment 4 Olli Pettay [:smaug] 2011-11-12 12:46:35 PST
Comment on attachment 574081 [details] [diff] [review]
patch

nsIJSEventListener ctor change makes sure we get the right nsISupports*.
That is, AFAIK, needed only because nsGlobalWindow's nsISupports isn't
nsIDOMEventTarget.

We could perhaps remove the whole QI, but I'd like to investigate that in
a different bug.
Comment 5 Andrew McCreight [:mccr8] 2011-11-12 16:05:27 PST
Created attachment 574095 [details]
nsJSEventListener after patch

The patch nicely gets rid of a bunch of nodes.  The attached diagram is the equivalent of the "bird of prey" I attached to bug 702032, and it now lacks that extra outer ring.  That's a reduction of 200 nodes from just this one part.
Comment 6 Olli Pettay [:smaug] 2011-11-18 09:22:25 PST
Review ping
Comment 7 Olli Pettay [:smaug] 2011-11-25 13:28:29 PST
Comment on attachment 574081 [details] [diff] [review]
patch

Boris, you've looked at this code lately...
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-11-28 12:36:04 PST
Comment on attachment 574081 [details] [diff] [review]
patch

Add some comments about mTarget being a weak reference and explicit Disconnect() calls being needed if it goes away, and r=me.

We're pretty sure that the ELM can't outlive its target?
Comment 9 Olli Pettay [:smaug] 2011-11-28 12:39:26 PST
ELM's mTarget and event listeners are cleared when ELM's Disconnect() is called.
And there is an assertion if Disconnect() isn't called.
Comment 11 Olli Pettay [:smaug] 2011-12-29 11:45:14 PST
Comment on attachment 574081 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): N/A
 
User impact if declined: Maybe longer CC pauses

Testing completed (on m-c, etc.):
Landed m-c 2011-11-29 so this is already in Aurora. Haven't heard of
any regressions. Comment 5 nicely visualizes what kind of impact this has.

Risk to taking this patch (and alternatives if risky):
Some binary addon could in theory use nsIJSEventListener and rely on
it to keep mTarget alive.
This is still, IMO, on the safe side of possible cycle collection changes for 
FF10 (which is why I'm asking for approval).
Comment 12 Olli Pettay [:smaug] 2011-12-29 11:46:32 PST
If the patch gets approved, comment
+// Note, mTarget is a raw pointer and the owner of the nsIJSEventListener object
+// is expected to call Disconnect()!
needs to be copied from the patch for trunk.
Comment 13 christian 2011-12-29 14:52:26 PST
Comment on attachment 574081 [details] [diff] [review]
patch

[triage comment]
Approved for beta. We're hoping this helps with the CC pause issues we've been seeing.
Comment 14 Olli Pettay [:smaug] 2011-12-29 15:22:07 PST
Created attachment 584870 [details] [diff] [review]
beta patch with comment change

Note You need to log in before you can comment on or make changes to this bug.