705371 - [CC] don't add JSContexts that have no children to the cycle collector graph

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

JSContexts can only ever have one child, globalObject.  If that's null, we hopefully don't need to add it to the graph.  In some examples I've seen, that can save 30-50 nodes, at the price of some null checks.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: skip JSContexts with null globalObject (WIP)

One thing I don't really like about this patch is that it makes the correctness of some code in XPCJSRuntime depend on an invariant in nsXPConnect, eg that JSContext's Traverse function only ever visits a single child.  I added a comment near Traverse.  I could also create a new function in nsXPConnect taking a JSContext that says when it will have no Traverse children.  There would still be no way to enforce that Traverse actually agrees with that function, but at least it could be near Traverse.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: don't add childless JSContexts to the CC graph

Take two.  This adds a new function to check if a JSContext should be added to the CC graph, so the reasoning is kept in one place.

Comment 3

[Andrew McCreight [:mccr8]]

The above patch removes very few JSContexts.  The reason is that the common reason for JSContexts to have no children because their child is marked black by the GC, not because it is null.  I have a changed version that does that check.

Comment 4

[Andrew McCreight [:mccr8]]

It might make sense to implement this using Smaug's new framework in bug 716518, instead of implementing my own one-off version of it.  Unfortunately, the JSContextListener is a nsCycleCollectionParticipant, so the Skip callbacks would have to be lifted up to its super-super-class, and I don't know if it is worth the added complexity for this one silly case.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: don't add childless JSContexts to the CC graph

This is much easier to do now, thanks to bug 716518 and the followup that moved the callbacks to cycle collection participant.  I haven't tested or even tried to compile this patch yet.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: don't add childless JSContexts to the CC graph

Oops, I made a method called CanSkipInThis instead of CanSkipThis, which built but caused errors when run, and didn't prune anything.  Normally we use the macros so that won't be a danger.

I confirmed that with this version JSContext without children aren't added to the graph.

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

Comment 7

[Andrew McCreight [:mccr8]]

Weird, judging from the first few results this is really crashy.  Not sure what the deal is there, as try server apparently isn't showing crash stacks again.

Comment 8

[Andrew McCreight [:mccr8]]

It isn't crashy when you always return false from the CanSkip* functions, as you'd expect.  Only 2/6 Mochitests crashed when I returned a real value for CanSkipThisReal, and false for the other two.

Comment 9

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

I'm just compiling a different kind of patch for this where we bypass adding
JSContext to the graph in XPCJSRuntime::AddXPConnectRoots if the context doesn't have gray global.
Simpler and keeps the XPConnect optimizations in one place. And seems to be effective.
(currently 20% of the objects in my CC graph are dummy JSContexts)

Need to get tryserver results.

Comment 10

[Andrew McCreight [:mccr8]]

Cool. I never did figure out why my patches were crashy.

Comment 11

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

Attachment: don't add useless contexts to the graph

Comment 12

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

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

Comment 13

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

Bah, try doesn't like that patch either. Don't understand...

Comment 14

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

Oh, null pointer...

Comment 15

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

So, I think the order matters here. The code assumes that JSContext is unlinked before nsJSContext.

Comment 16

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

Attachment: patch

This works, but I'll do still a bit safer patch which shouldn't change the
behavior.

Comment 17

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

Attachment: safer

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

Not too pretty.

Comment 18

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

Comment on attachment 698427 [details] [diff] [review]
safer

Not sure if I should ask review or feedback...

This is not pretty but luckily we don't have cycle collected JSContext members
too often.
Because of JSContextParticipant traverse making artificial refcount, this
doesn't affect to those JSContext's which aren't used as cycle collectable member
variables.

Comment 19

[Andrew McCreight [:mccr8]]

If I understand correctly, what is happening here is that we have two kinds of things that own JSContext, nsJSContext and this script executor. We only actually unlink the JSContext in the owning thing. Interesting.

Are JSContexts always going to have a subset of the lifetime of one of these two kinds of owning structures?

A more consistent approach would be to eliminate JSContextParticipant. In XPCJSRuntime::AddXPConnectRoots we would add the global to the graph, not the JSContext. Then, add functions like JSContextTraverse and JSContextUnlink, and call them in the traverse and unlink functions of nsJSContext and script executors. Then JSContexts would act like a wholly owned substructure of whatever is enclosing them.

One thing this won't handle is that js::ContextHasOutstandingRequests(cx) weirdness, but maybe we could add a pass before we build the graph where we unmark gray any JSContext where we are executing code. In fact, I wonder if we should assert if we find any executing context with gray globals, as the various UnmarkGray things should probably be catching that.

My suggested approach here would require more code to write, but at least it would clear up a few bits of outstanding ugliness in the context traversal code.

Comment 20

[Andrew McCreight [:mccr8]]

Comment on attachment 698427 [details] [diff] [review]
safer

If you don't want to deal with that, I think the slightly hackier patch here is probably okay for now. :)

Comment 21

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

(In reply to Andrew McCreight [:mccr8] from comment #19)
> If I understand correctly, what is happening here is that we have two kinds
> of things that own JSContext, nsJSContext and this script executor.

We have also JSContexts which aren't owned by anything cycle collected

> Are JSContexts always going to have a subset of the lifetime of one of these
> two kinds of owning structures?


> 
> A more consistent approach would be to eliminate JSContextParticipant. In
> XPCJSRuntime::AddXPConnectRoots we would add the global to the graph, not
> the JSContext.
I tried that, but we need to make sure to keep non-cc-owned contexts alive.

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 698427 [details] [diff] [review]
safer

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

Thanks for fixing this. Hopefully in the future we can do something prettier...

::: content/base/src/nsFrameMessageManager.cpp
@@ +1024,5 @@
> +nsFrameScriptExecutor::Unlink(nsFrameScriptExecutor* aTmp)
> +{
> +  if (aTmp->mCx) {
> +    JSAutoRequest ar(aTmp->mCx);
> +    JS_SetGlobalObject(aTmp->mCx, nullptr);

Could you maybe make these two lines into a function in XPC called something like UnlinkJSContext, and then use it here and in nsJSEnvironment?

Comment 23

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

Well, only two cases when that setglobalobject is called, at least for now.
I'll file a followup to clean up this mess. nsJSContext and Messagemanager etc should have some
sane way to handle JSContexts

Comment 24

[Andrew McCreight [:mccr8]]

Sounds reasonable to me.

Comment 25

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

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