827471 - Improve CanSkipWrappedJS

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Hmm, not quite that simple.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: faster

https://tbpl.mozilla.org/?tree=Try&rev=9bc9a32fc886

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

https://tbpl.mozilla.org/?tree=Try&rev=c43a726861b8

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

This should get rid of about 30% of objects in my current graph.

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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

[Andrew McCreight [:mccr8]]

Comment on attachment 698910 [details] [diff] [review]
v3

Great! Thanks for the explanations.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/a3effbaa10bb