Improve CanSkipWrappedJS

RESOLVED FIXED in mozilla21

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla21
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 698813 [details] [diff] [review]
patch

I have quite a few WrappedJS objects in my graph. They are objects
holding root WrappedJS object alive, so they are are always in pair.
We optimize out only root wrappedJS, but non-root wrappedJS brings back also the root.

I don't expect this to speed up anything, but makes graphs easier to analyze.

https://tbpl.mozilla.org/?tree=Try&rev=5487bf997839
Hmm, not quite that simple.
Created attachment 698828 [details] [diff] [review]
faster

https://tbpl.mozilla.org/?tree=Try&rev=9bc9a32fc886
Attachment #698813 - Attachment is obsolete: true
Created attachment 698910 [details] [diff] [review]
v3

https://tbpl.mozilla.org/?tree=Try&rev=c43a726861b8
Attachment #698828 - Attachment is obsolete: true
Attachment #698910 - Flags: review?(continuation)
This should get rid of about 30% of objects in my current graph.
Comment on attachment 698910 [details] [diff] [review]
v3

Review of attachment 698910 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +435,4 @@
>      if (nsCCUncollectableMarker::sGeneration &&
>          (!obj || !xpc_IsGrayGCThing(obj)) &&
>          !wrappedJS->IsSubjectToFinalization() &&
> +        (isRootWrappedJS || CanSkipWrappedJS(wrappedJS->GetRootWrapper()))) {

Are these chains of root wrappers going to be fairly short? The time to visit them all is going to be quadratic in the length of these chains, which could get ugly.

@@ +435,5 @@
>      if (nsCCUncollectableMarker::sGeneration &&
>          (!obj || !xpc_IsGrayGCThing(obj)) &&
>          !wrappedJS->IsSubjectToFinalization() &&
> +        (isRootWrappedJS || CanSkipWrappedJS(wrappedJS->GetRootWrapper()))) {
> +        if (!wrappedJS->IsAggregatedToNative() || !isRootWrappedJS) {

The additional quick case here occurs when the wrapped JS IsAggregatedToNative(), but also CanSkipWrappedJS() on its root wrapper. Why is that okay to not look at the aggregated native object? I admit to not knowing what is going on in the else case here.
(In reply to Andrew McCreight [:mccr8] from comment #5)
> Are these chains of root wrappers going to be fairly short? The time to
> visit them all is going to be quadratic in the length of these chains, which
> could get ugly.
All the chains I've seen have been short. Just one wrappedjs under root



> The additional quick case here occurs when the wrapped JS
> IsAggregatedToNative(), but also CanSkipWrappedJS() on its root wrapper. Why
> is that okay to not look at the aggregated native object? I admit to not
> knowing what is going on in the else case here.
Because aggregated native object hangs in the root
Comment on attachment 698910 [details] [diff] [review]
v3

Great! Thanks for the explanations.
Attachment #698910 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/a3effbaa10bb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.