714642 - Figure out some way to optimize nsXPCWrappedJS traversing

Status: RESOLVED DUPLICATE of bug 720686

(Core :: XPConnect, defect)

Description

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

In some cases we traverse lots of nsXPCWrappedJS objects.
We should try to do less or be faster.

Comment 1

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

*If* my testing is correct, in many cases about 65% of WrappedJS objects are certainly alive.
Would be great to be able to skip that.

Comment 2

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

Attachment: patch

Something like this.
Didn't seem to leak at least in simple tests, but let's see what
tryserver says
https://tbpl.mozilla.org/?tree=Try&rev=44772bdb3013

Comment 3

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

Um, is the patch all wrong :/

Comment 4

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

Comment on attachment 585330 [details] [diff] [review]
patch

Ok, this doesn't make sense, but something similar could.

Comment 5

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

So, I want to be able to mark WrappedJS object certainly alive.
Fortunately nsXPCWrappedJS has plenty of spare bits left.

Comment 6

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

Attachment: WIP

On top of other stuff I have

Comment 7

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

Attachment: patch

Something like this.
DOM code, for example, can then set the generation. This can be used to
optimize for example event listener traversing.

Comment 8

[Andrew McCreight [:mccr8]]

Can we just skip these altogether?  We need to add all C++->JS edges so that we hold alive everything from JS.  To catch cross-compartment cycles, we only need either all C++->JS or all JS->C++ edges.

Comment 9

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

(In reply to Andrew McCreight [:mccr8] from comment #8)
> Can we just skip these altogether?
You mean removing the whole
for (XPCRootSetElem *e = mWrappedJSRoots; e ; e = e->GetNextRoot()) {
         nsXPCWrappedJS *wrappedJS = static_cast<nsXPCWrappedJS*>(e);

?

Comment 10

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

Perhaps. Blake, Peter?

The patch is certainly safer option, since it would be used only in those cases when
the object is certainly owned by something black.

Comment 11

[Andrew McCreight [:mccr8]]

Yes, that's what I meant.  It sounds like this is actually useful in either case, because it will squash things being held by black JS.  We can investigate the more radical option in another bug.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 586507 [details] [diff] [review]
patch

I don't understand the XPCWrappedJS stuff well enough to contribute here. Hopefully peterv can, though.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Err, that's the wrappedJS *traversal* stuff.

Comment 14

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

Comment on attachment 586507 [details] [diff] [review]
patch

I have a better plan for this.

Comment 15

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

Attachment: patch

Sorry for the -U 1, but I just manually cut the patch from a larger patch.

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp#579

Comment 16

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

Attachment: v3

Oops, previous patch was  missing the #include.

Comment 17

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

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

This got merged to the generic xpc patch