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)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(4 files)

No description provided.
Attached patch patchSplinter Review
Attachment #574081 - Flags: feedback?(continuation)
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+
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)
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.
Review ping
Comment on attachment 574081 [details] [diff] [review] patch Boris, you've looked at this code lately...
Attachment #574081 - Flags: review?(peterv) → review?(bzbarsky)
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+
ELM's mTarget and event listeners are cleared when ELM's Disconnect() is called. And there is an assertion if Disconnect() isn't called.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
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?
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 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+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: