Closed Bug 785493 Opened 9 years ago Closed 9 years ago

Mark the script of live nsXULPrototypeScript black during GC


(Core :: XUL, defect)

Not set





(Reporter: mccr8, Assigned: mccr8)


(Blocks 2 open bugs)


(Whiteboard: [Snappy])


(2 files, 7 obsolete files)

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.
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.
Attached patch part 1: preparatory clean up (obsolete) — Splinter Review
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.
Assignee: nobody → continuation
Blocks: 785532
It leaks 122MB during Mochitest Chrome, so that's not good.  Seems okay otherwise.
I can reproduce a shutdown leak with these two tests:

		test_bug437844.xul \
		bug451286_window.xul \
		test_bug451286.xul \

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.
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.
It isn't clear to me how I might iterate over the set of live XUL documents.
MarkContentViewer should do it. nsIDocument has IsXUL method.
Hopefully less leaky than the other approach.
Attachment #655145 - Attachment is obsolete: true
Attachment #655153 - Attachment is obsolete: true
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?
You're looking ancient code on top of which CC stuff have been added.
Expect some weirdness :)
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.
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!
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.
Depends on: 824958
Attachment #655123 - Attachment is obsolete: true
Attachment #659582 - Attachment description: part 2: unmark gray scripts held by XUL elements held by live XUL documents → unmark gray scripts held by XUL elements held by live XUL documents
Attachment #659582 - Attachment is obsolete: true
Still need to make an opt build to see if this is too slow.
Attachment #696065 - Attachment is patch: true
Forgot a null check. Maybe this will work better.
Attachment #696066 - Attachment is obsolete: true
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.
Summary: Make nsXULPrototypeScript root its script black, instead of gray → Mark the script of live nsXULPrototypeScript black during GC
Version: 17 Branch → Trunk
Whiteboard: [Snappy]
Comment on attachment 696065 [details] [diff] [review]

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

Let me know if there's a better way I should be tracing scripts.
Attachment #696065 - Flags: review?(wmccloskey)
Attachment #696109 - Attachment is obsolete: true
Attachment #696374 - Flags: review?(bugs)
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);
>+    }
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");
>+            }
>+        }
>+    }


>+    void TraceScripts(JSTracer* trc);

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

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

>+nsXULPrototypeDocument::TraceProtos(JSTracer* trc)
>+  mRoot->TraceScripts(trc);

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

r- because I'd like to avoid duplicate work in case there are several windows open.
Attachment #696374 - Flags: review?(bugs) → review-
> 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...
Attachment #696065 - Flags: review?(wmccloskey) → review+
> 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.
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:
Attachment #696374 - Attachment is obsolete: true
Attachment #696744 - Flags: review?(bugs)
Oh, right, you r-'d it before, so it needs a review in any event...
Attachment #696744 - Flags: review?(bugs) → review+

As I said before, in local testing this reduced the size of the "steady state" cycle collector graph by about 80%.
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 827020
You need to log in before you can comment on or make changes to this bug.