Closed Bug 802829 Opened 8 years ago Closed 8 years ago

Don't add nsXPCWrappedJS objects to CC graph if they only point to a certainly alive object

Categories

(Core :: XPConnect, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch patchSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=d442b1ec6280

This should affect especially to nsXPCWrappedJS (nsIDOMXULLabelElement) case
Attachment #672533 - Flags: review?(continuation)
Comment on attachment 672533 [details] [diff] [review]
patch

Hmm, is this causing some odd behavior after all... investigating.
Attachment #672533 - Flags: review?(continuation)
Or no, I'm getting the same odd behavior even without the patch... investigating.
Attachment #672533 - Flags: review?(continuation)
with the patch I get <1000 objects in the graph in my normal setup.
...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 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+
CallQueryInterface does normal QI, so there shouldn't be no need for canonicalizing.
(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
Attached patch v2 (obsolete) — Splinter Review
Attached patch v3Splinter Review
Attachment #673613 - Attachment is obsolete: true
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.
Bah, indeed.
But I pushed already.
https://hg.mozilla.org/mozilla-central/rev/034634c4125a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.