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] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks: 702032
  Show dependency treegraph
 
Reported: 2011-11-12 12:00 PST by Olli Pettay [:smaug] (high review load, please consider other reviewers)
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] (high review load, please consider other reviewers)
bzbarsky: review+
continuation: feedback+
christian: approval‑mozilla‑beta+
Details | Diff | 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] (high review load, please consider other reviewers)
no flags Details | Diff | Review
beta patch with comment change (8.38 KB, patch)
2011-12-29 15:22 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-11-12 12:00:29 PST

    
Comment 1 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-11-12 12:26:29 PST
Created attachment 574081 [details] [diff] [review]
patch
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 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] (high review load, please consider other reviewers) 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] (high review load, please consider other reviewers) 2011-11-18 09:22:25 PST
Review ping
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 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] (Out June 25-July 6) 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] (high review load, please consider other reviewers) 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 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-11-29 01:48:19 PST
Created attachment 577522 [details] [diff] [review]
patch

https://hg.mozilla.org/mozilla-central/rev/e3c8a14cd60b
Comment 11 Olli Pettay [:smaug] (high review load, please consider other reviewers) 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] (high review load, please consider other reviewers) 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] (high review load, please consider other reviewers) 2011-12-29 15:22:07 PST
Created attachment 584870 [details] [diff] [review]
beta patch with comment change
Comment 15 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-12-30 02:27:51 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/09bc6f7ed00e

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