Closed
Bug 702036
Opened 13 years ago
Closed 13 years ago
JSEventListener could have weak mTarget to reduce CC overhead
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(4 files)
8.26 KB,
patch
|
bzbarsky
:
review+
mccr8
:
feedback+
christian
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
25.06 KB,
application/pdf
|
Details | |
8.33 KB,
patch
|
Details | Diff | Splinter Review | |
8.38 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #574081 -
Flags: feedback?(continuation)
Assignee | ||
Comment 2•13 years ago
|
||
Comment 3•13 years ago
|
||
Comment on attachment 574081 [details] [diff] [review]
patch
Looks good to me, from the perspective of removing things from the CC graph. Thanks!
Attachment #574081 -
Flags: feedback?(continuation) → feedback+
Assignee | ||
Comment 4•13 years ago
|
||
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.
Attachment #574081 -
Flags: review?(peterv)
Comment 5•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
Review ping
Assignee | ||
Comment 7•13 years ago
|
||
Comment on attachment 574081 [details] [diff] [review]
patch
Boris, you've looked at this code lately...
Attachment #574081 -
Flags: review?(peterv) → review?(bzbarsky)
![]() |
||
Comment 8•13 years ago
|
||
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?
Attachment #574081 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 9•13 years ago
|
||
ELM's mTarget and event listeners are cleared when ELM's Disconnect() is called.
And there is an assertion if Disconnect() isn't called.
Assignee | ||
Comment 10•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Target Milestone: --- → mozilla11
Assignee | ||
Comment 11•13 years ago
|
||
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).
Attachment #574081 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 12•13 years ago
|
||
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•13 years ago
|
||
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.
Attachment #574081 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 14•13 years ago
|
||
Assignee | ||
Comment 15•13 years ago
|
||
Target Milestone: mozilla11 → mozilla10
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•