Closed
Bug 802829
Opened 12 years ago
Closed 12 years ago
Don't add nsXPCWrappedJS objects to CC graph if they only point to a certainly alive object
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
Details
Attachments
(2 files, 1 obsolete file)
1.68 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
3.19 KB,
patch
|
Details | Diff | Splinter Review |
https://tbpl.mozilla.org/?tree=Try&rev=d442b1ec6280 This should affect especially to nsXPCWrappedJS (nsIDOMXULLabelElement) case
Attachment #672533 -
Flags: review?(continuation)
Assignee | ||
Comment 1•12 years ago
|
||
Comment on attachment 672533 [details] [diff] [review] patch Hmm, is this causing some odd behavior after all... investigating.
Attachment #672533 -
Flags: review?(continuation)
Assignee | ||
Comment 2•12 years ago
|
||
Or no, I'm getting the same odd behavior even without the patch... investigating.
Assignee | ||
Updated•12 years ago
|
Attachment #672533 -
Flags: review?(continuation)
Assignee | ||
Comment 3•12 years ago
|
||
with the patch I get <1000 objects in the graph in my normal setup.
Assignee | ||
Comment 4•12 years ago
|
||
...but I don't expect changes to performance, since we just remove stuff from the graph early during CC. But makes graphs easier to analyze.
Comment 5•12 years ago
|
||
Comment on attachment 672533 [details] [diff] [review] patch Review of attachment 672533 [details] [diff] [review]: ----------------------------------------------------------------- The idea here is that if a wrappedJS has an aggregated native object, then it has an additional pointer to an nsISupports native, and if we're going to skip that native, then the whole wrapped JS is not interesting? ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +465,1 @@ > nsXPCWrappedJS *wrappedJS = static_cast<nsXPCWrappedJS*>(e); Please turn this whole block into a function that takes wrappedJS as an argument and returns a bool. You could call it something like CanSkipWrappedJS. @@ +476,5 @@ > + } else { > + nsISupports* agg = wrappedJS->GetAggregatedNativeObject(); > + nsXPCOMCycleCollectionParticipant* cp = nullptr; > + CallQueryInterface(agg, &cp); > + nsISupports* canonical = nullptr; Should you canonicalize first, before you get the participant? I don't recall what is right, though I guess it shouldn't matter...
Attachment #672533 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 6•12 years ago
|
||
CallQueryInterface does normal QI, so there shouldn't be no need for canonicalizing.
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #5) > The idea here is that if a wrappedJS has an aggregated native object, then > it has an additional pointer to an nsISupports native, and if we're going to > skip that native, then the whole wrapped JS is not interesting? Yes
Assignee | ||
Comment 8•12 years ago
|
||
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #673613 -
Attachment is obsolete: true
Comment 10•12 years ago
|
||
Comment on attachment 673614 [details] [diff] [review] v3 Review of attachment 673614 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +437,5 @@ > + !wrappedJS->IsSubjectToFinalization() && > + wrappedJS->GetRootWrapper() == wrappedJS) { > + if (!wrappedJS->IsAggregatedToNative()) { > + return true; > + } else { You don't need the |else| here.
Assignee | ||
Comment 11•12 years ago
|
||
Bah, indeed. But I pushed already. https://hg.mozilla.org/mozilla-central/rev/034634c4125a
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•