785493 - Mark the script of live nsXULPrototypeScript black during GC

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Andrew McCreight [:mccr8]]

When not much is going on, the CC graph has a ton of JS, being held alive by nsXULPrototypeScripts. That JS shouldn't create cycles with the XUL elements, so we should just root and unroot it manually instead of using the usual JS holder interface.  This will make them black all the time, and thus they will never be seen by the cycle collector.  This was Olli's idea.

Comment 1

[Andrew McCreight [:mccr8]]

In my local profile, this reduces the number of GC objects in an idle CC from 3800 to a few hundred, reducing the over all size of the CC graph by about 2/3rds.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 1: preparatory clean up

The intermediate script holding structure isn't useful any more. Additionally, we make the script pointer private to make it a little safer when we later want to start requiring rooting and unrooting.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 2: add XPC interface to root and unroot JSScript

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 3: manually root/unroot nsXULPrototypeScript's script instead of using the CC

Comment 5

[Andrew McCreight [:mccr8]]

Basic try run: https://tbpl.mozilla.org/?tree=Try&rev=145f2b30188c

Comment 6

[Andrew McCreight [:mccr8]]

It leaks 122MB during Mochitest Chrome, so that's not good.  Seems okay otherwise.

Comment 7

[Andrew McCreight [:mccr8]]

I can reproduce a shutdown leak with these two tests:

MOCHITEST_CHROME_FILES = \
		test_bug437844.xul \
		bug451286_window.xul \
		test_bug451286.xul \
		RegisterUnregisterChrome.js

The first one disables and reenables the prototype cache, but change it so that it leaves the cache alone doesn't seem to fix it.

It looks like there's a missing edge for a few XUL documents that are in turn holding the XUL prototype documents alive.  Presumably it is from the JS.  I'll have to figure out how to dig that out.

Comment 8

[Andrew McCreight [:mccr8]]

It looks like there is a cycle. The initial script is being held alive by an nsXULPrototypeScript. With my patch here, it is rooted until the death of the prototype script node.

0x1290520e8 [script chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1]
    --[objects[43]]-> 0x12906be80 [Function]
    --[script]-> 0x12906d8c8 [script chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:936]
    --[objects[0]]-> 0x12905b3a0 [RegExp 0]
    --[shape]-> 0x12906ab78 [shape]
    --[parent]-> 0x12906ab28 [shape]
    ... more parent edges through shapes ...
    --[base]-> 0x129059510 [base_shape]
    --[parent]-> 0x12904e060 [Window 1280983c0]
    --[document]-> 0x12904f1c0 [XULDocument 128098ac0]

Separately, I've seen that the nsXULPrototypeScript is being held alive by a XULDocument, which I assume is the same one here, though I'm not entirely sure, as JS formatting of addresses is weird, and 128098ac0 doesn't correspond to anything in the CC log.

I'll try looking at a more "conventional" approach, using the liveness of XUL documents to guide marking these script roots as black during GC marking.

Comment 9

[Andrew McCreight [:mccr8]]

It isn't clear to me how I might iterate over the set of live XUL documents.

Comment 10

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

MarkContentViewer should do it. nsIDocument has IsXUL method.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: unmark gray scripts held by XUL elements held by live XUL documents

Hopefully less leaky than the other approach.

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

Comment 12

[Andrew McCreight [:mccr8]]

Also, the mPrototypes looks a little weird to me. nsXULDocument::PrepareToWalk shoves mCurrentPrototype onto it, but it is otherwise completely unused outside of CC traversal. I guess if we ever walk a prototype we can somehow potentially reference things in it without otherwise keeping them alive?

Comment 13

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

You're looking ancient code on top of which CC stuff have been added.
Expect some weirdness :)

Comment 14

[Andrew McCreight [:mccr8]]

I was getting times of 1-3ms in a debug build for unmarking some XULDocuments, which is really terrible, but now when I am trying it again I'm only getting no worse than 0.05ms. I'm not sure what happened.

Comment 15

[Andrew McCreight [:mccr8]]

Okay, on my own profile it is in fact taking 2ms to mark one of the documents, so I think the decreased time was because I switched over to a cleaner profile that is newer and has less addons.  Back to investigating why it is so slow!

Comment 16

[Andrew McCreight [:mccr8]]

At least 90% of the time is being taking up in the actual UnmarkGrayScript call, which is as I'd expect, so I'll try marking these documents as black during the GC black marking phase. That will eliminate that overhead.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: part 1: add JS_CALL_SCRIPT_TRACER

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: part 2: add proto scripts of live docs as black roots during GC

Still need to make an opt build to see if this is too slow.

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

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: part 2: add proto scripts of live docs as black roots during GC

Forgot a null check. Maybe this will work better.
https://tbpl.mozilla.org/?tree=Try&rev=f2cca4a74145

Comment 20

[Andrew McCreight [:mccr8]]

Testing locally in an opt non-debug build, the total time spent iterating over windows in mozilla::dom::TraceBlackJS was on the order of 0.15ms or so, on my default profile, so it seems like speed isn't a problem for this approach.

Comment 21

[Andrew McCreight [:mccr8]]

Comment on attachment 696065 [details] [diff] [review]
part 1: add JS_CALL_SCRIPT_TRACER

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

Let me know if there's a better way I should be tracing scripts.

Comment 22

[Andrew McCreight [:mccr8]]

Attachment: part 2: add proto scripts of live docs as black roots during GC

Comment 23

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

Comment on attachment 696374 [details] [diff] [review]
part 2: add proto scripts of live docs as black roots during GC

> TraceActiveWindowGlobal(const uint64_t& aId, nsGlobalWindow*& aWindow, void* aClosure)
> {
>   if (aWindow->GetDocShell() && aWindow->IsOuterWindow()) {
>+    JSTracer* trc = static_cast<JSTracer *>(aClosure);
>     if (JSObject* global = aWindow->FastGetGlobalJSObject()) {
>-      JSTracer* trc = static_cast<JSTracer *>(aClosure);
>       JS_CALL_OBJECT_TRACER(trc, global, "active window global");
>     }
>+#ifdef MOZ_XUL
>+    nsIDocument* doc = aWindow->GetExtantDoc();
>+    if (doc && doc->IsXUL()) {
>+      static_cast<nsXULDocument*>(doc)->TraceProtos(trc);
>+    }
>+#endif
I think we should pass CC generation to TraceProtos and store generation in protodocument so that
we don't end up traversing the same prototree several times.
(Separate browser windows share the same prototree.)


>+nsXULPrototypeElement::TraceScripts(JSTracer* trc)
>+{
>+    for (uint32_t i = 0; i < mChildren.Length(); ++i) {
>+        nsXULPrototypeNode* child = mChildren[i];
>+        if (child->mType == nsXULPrototypeNode::eType_Element) {
>+            static_cast<nsXULPrototypeElement*>(child)->TraceScripts(trc);
>+        } else if (child->mType == nsXULPrototypeNode::eType_Script) {
>+            JSScript* script = static_cast<nsXULPrototypeScript*>(child)->GetScriptObject();
>+            if (script) {
>+                JS_CALL_SCRIPT_TRACER(trc, script, "active window XUL prototype script");
>+            }
>+        }
>+    }
>+}

aTrc

>+    void TraceScripts(JSTracer* trc);
aTrc

>+nsXULDocument::TraceProtos(JSTracer* trc)
>+{
>+    uint32_t i, count = mPrototypes.Length();
>+    for (i = 0; i < count; ++i) {
>+        mPrototypes[i]->TraceProtos(trc);
>+    }
>+}
aTrc

>@@ -166,16 +167,19 @@ public:
>     MatchAttribute(nsIContent* aContent,
>                    int32_t aNameSpaceID,
>                    nsIAtom* aAttrName,
>                    void* aData);
> 
>     NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(nsXULDocument, nsXMLDocument)
> 
>     virtual nsXPCClassInfo* GetClassInfo();
>+
>+    void TraceProtos(JSTracer* trc);
aTrc


>+void
>+nsXULPrototypeDocument::TraceProtos(JSTracer* trc)
>+{
>+  mRoot->TraceScripts(trc);
>+}
aTrc

>@@ -113,16 +114,18 @@ public:
>     void MarkInCCGeneration(uint32_t aCCGeneration)
>     {
>         mCCGeneration = aCCGeneration;
>     }
> 
>     NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(nsXULPrototypeDocument,
>                                              nsIScriptGlobalObjectOwner)
> 
>+    void TraceProtos(JSTracer* trc);
aTrc


r- because I'd like to avoid duplicate work in case there are several windows open.

Comment 24

[Andrew McCreight [:mccr8]]

> I think we should pass CC generation to TraceProtos and store generation in
> protodocument so that we don't end up traversing the same prototree several times.
> (Separate browser windows share the same prototree.)

Ah, yes, good point! I'll work on fixing that.

> aTrc

Oops! Writing browser-style code that is dealing with JS stuff is always confusing to me...

Comment 25

[Andrew McCreight [:mccr8]]

> I think we should pass CC generation to TraceProtos

One thing I realized is that we can't use the CC generation, because this marking is being done per GC, not CC. But a similar mechanism should work.

Comment 26

[Andrew McCreight [:mccr8]]

Attachment: part 2: add proto scripts of live docs as black roots during GC

This uses the GC number as the "GC generation". The GC number is incremented at the very start of a GC, so initializing the cached GC number to zero should work as expected. It is mostly the same, but I figured this could use a re-review for the bits that are different.

Try run just in case: https://tbpl.mozilla.org/?tree=Try&rev=7056dc81f3a7

Comment 27

[Andrew McCreight [:mccr8]]

Oh, right, you r-'d it before, so it needs a review in any event...

Comment 28

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3b007280f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/877d78ac7d4f

As I said before, in local testing this reduced the size of the "steady state" cycle collector graph by about 80%.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8f3b007280f4
https://hg.mozilla.org/mozilla-central/rev/877d78ac7d4f