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

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: mccr8, Assigned: smaug)

Tracking

(Blocks 1 bug)

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
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.
Blocks: 716598
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.
Assignee: nobody → continuation
Attachment #577003 - Attachment is obsolete: true
No longer blocks: 702032
Blocks: 712170
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.
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.
Depends on: 718634
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.
Attachment #588438 - Attachment is obsolete: true
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
Attachment #589507 - Attachment is obsolete: true
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.
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.
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.
Cool. I never did figure out why my patches were crashy.
Assignee: continuation → nobody
Bah, try doesn't like that patch either. Don't understand...
Oh, null pointer...
So, I think the order matters here. The code assumes that JSContext is unlinked before nsJSContext.
Posted patch patch (obsolete) — Splinter Review
This works, but I'll do still a bit safer patch which shouldn't change the
behavior.
Posted patch saferSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=a902010d8c0c

Not too pretty.
Assignee: nobody → bugs
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.
Attachment #698427 - Flags: review?(continuation)
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 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. :)
Attachment #698427 - Flags: review?(continuation) → feedback+
(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 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?
Attachment #698427 - Flags: feedback+ → review+
Attachment #589525 - Attachment is obsolete: true
Attachment #698307 - Attachment is obsolete: true
Attachment #698422 - Attachment is obsolete: true
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
Sounds reasonable to me.
https://hg.mozilla.org/mozilla-central/rev/eaf8b4f2eaa5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.