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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

7 years ago
Posted patch patchSplinter 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

7 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

7 years ago
Or no, I'm getting the same odd behavior even without the patch... investigating.
Assignee

Updated

7 years ago
Attachment #672533 - Flags: review?(continuation)
Assignee

Comment 3

7 years ago
with the patch I get <1000 objects in the graph in my normal setup.
Assignee

Comment 4

7 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 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

7 years ago
CallQueryInterface does normal QI, so there shouldn't be no need for canonicalizing.
Assignee

Comment 7

7 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

7 years ago
Posted patch v2 (obsolete) — Splinter Review
Assignee

Comment 9

7 years ago
Posted 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.
Assignee

Comment 11

7 years ago
Bah, indeed.
But I pushed already.
https://hg.mozilla.org/mozilla-central/rev/034634c4125a
Assignee

Updated

7 years ago
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.