Last Comment Bug 693527 - add Watchpoint support to the cycle collector
: add Watchpoint support to the cycle collector
Status: RESOLVED FIXED
[MemShrink:P2]
: mlk, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on: 668855
Blocks: 326633
  Show dependency treegraph
 
Reported: 2011-10-10 22:22 PDT by Jesse Ruderman
Modified: 2012-01-28 19:01 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (344 bytes, text/html)
2011-10-10 22:22 PDT, Jesse Ruderman
no flags Details
test case 2 (279 bytes, text/plain)
2011-10-11 15:16 PDT, Andrew McCreight [:mccr8]
no flags Details
tell the cycle collector about watchpoints (2.88 KB, patch)
2011-10-12 09:58 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Review
tell the cycle collector about watchpoints (6.03 KB, patch)
2011-10-12 11:49 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Review
tell the cycle collector about watchpoints (6.13 KB, patch)
2011-11-24 09:47 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Review
tell the cycle collector about watchpoints (6.10 KB, patch)
2012-01-26 10:34 PST, Andrew McCreight [:mccr8]
jorendorff: review+
Details | Diff | Review

Description Jesse Ruderman 2011-10-10 22:22:40 PDT
Created attachment 566125 [details]
testcase

Bug 639235 mentioned that watchpoints have something in common with WeakMap. Could this bug be related to the WeakMap leaks?
Comment 1 Andrew McCreight [:mccr8] 2011-10-11 10:03:59 PDT
It is probably something similar.  I figured they probably leaked in the same way, but hadn't seen any reports.  I was hoping this would be fixed by Bug 653248, which changes watchpoints so they are marked gray when reachable only from gray roots, but unfortunately it does not.  Maybe we need to tell the cycle collector explicitly about these edges.
Comment 2 Andrew McCreight [:mccr8] 2011-10-11 10:18:09 PDT
Hmm.  The window and document are being kept alive by an nsNodeInfoManager.  That is kind of odd.

0x11850e620 [nsNodeInfoManager]
    --[mDocument]-> 0x1185c7000 [nsDocument (xhtml) https://bug693527.bugzilla.mozilla.org/attachment.cgi?id=566125]
    --[mChildren[i]]-> 0x119ea75e0 [nsGenericElement (xhtml) html]
    --[mAttrsAndChildren[i]]-> 0x1188a5500 [nsGenericElement (xhtml) p]
    --[mSlots->mChildrenList]-> 0x1188a5980 [nsBaseContentList]
    --[Expando object]-> 0x1180653a0 [JS Object (DOM proxy binding expando object) (global=118059060)]
    --[parent, x]-> 0x1180568c8 [JS Object (HTMLParagraphElement) (global=118059060)]
    --[type_proto, XPCWrappedNativeProto::mJSProtoObject]-> 0x1180566b8 [JS Object (XPC_WN_ModsAllowed_NoCall_Proto_JSClass - HTMLParagraphEleme (global=118059060)]
    --[type_proto]-> 0x118056768 [JS Object (XPC_WN_ModsAllowed_NoCall_Proto_JSClass - HTMLElement) (global=118059060)]
    --[type_proto]-> 0x118056818 [JS Object (XPC_WN_ModsAllowed_NoCall_Proto_JSClass - Element) (global=118059060)]
    --[parent, XPCWrappedNativeScope::mGlobalJSObject]-> 0x118059060 [JS Object (Window) (global=118059060)]
    --[xpc_GetJSPrivate(obj)]-> 0x11867ce80 [XPCWrappedNative (Window)]
    --[mIdentity]-> 0x11a1dd878 [nsGlobalWindow]
    --[mOuterWindow]-> 0x12151bc78 [nsGlobalWindow]

0x11850e620 [nsNodeInfoManager]
    --[mDocument]-> 0x1185c7000 [nsDocument (xhtml) https://bug693527.bugzilla.mozilla.org/attachment.cgi?id=566125]

    Root 0x11850e620 is a ref counted object with 1 unknown edge(s).
    known edges:
       0x129844b48 [nsNodeInfo (xhtml) head] --[mOwnerManager]-> 0x11850e620
       0x129845098 [nsNodeInfo (xhtml) p] --[mOwnerManager]-> 0x11850e620
       0x129844ac0 [nsNodeInfo (xhtml) html] --[mOwnerManager]-> 0x11850e620
       0x129844ce0 [nsNodeInfo (xhtml) body] --[mOwnerManager]-> 0x11850e620
       0x129844a38 [nsNodeInfo ([none]) #document-type] --[mOwnerManager]-> 0x11850e620
       0x129844c58 [nsNodeInfo ([none]) #text] --[mOwnerManager]-> 0x11850e620
       0x1298449b0 [nsNodeInfo ([none]) #document] --[mOwnerManager]-> 0x11850e620
       0x129844bd0 [nsNodeInfo (xhtml) script] --[mOwnerManager]-> 0x11850e620
Comment 3 Andrew McCreight [:mccr8] 2011-10-11 12:03:56 PDT
When you remove the "fuzzerisms" the code looks like this:

    var p = document.createElement("p");
    document.documentElement.appendChild(p);
    p.children.x = p;
    var f = function() { };
    p.watch("y", f);
    f.z = document.createElement("div");

Removing any of these lines eliminates the leak.  It looks like that non-GCMarkers are never told about watchpoints, so the cycle collector won't see the edge from p to f, so I think it won't see the div element in f.z.  Johnny said that the new div element that goes in f.z will have a strong reference to the nsNodeInfoManager, which would explain the unknown edge causing the leak.  IE there is a cycle from the document to p to f to f.x to the node info manager to the document, and the CC doesn't see the edge from p to f.

To fix this, we have to tell the cycle collector about watchpoint edges.  I think this can be fixed by treating them as fake weak map edges, where the "map" is marked black.  Should be straightforward...
Comment 4 Andrew McCreight [:mccr8] 2011-10-11 15:16:34 PDT
Created attachment 566368 [details]
test case 2

Here's a variation of Jesse's test case where the cycle does not involve nsNodeInfoManager (which I think may be sensitive to some specifics of whether the info manager reference is strong or weak).  The cycle is p -> f -> d -> p.

    var p = document.createElement("p");
    p.children.x = p;
    var f = function() { };
    p.watch("y", f);
    var d = document.createElement("div");
    d.appendChild(p);
    f.loop = d;
Comment 5 Andrew McCreight [:mccr8] 2011-10-12 09:58:45 PDT
Created attachment 566574 [details] [diff] [review]
tell the cycle collector about watchpoints

Fairly straightforward.  I confirmed that this patch fixes the leak in test case 2.  I should still add a test for this.

The code mimics the GC marking code for watchpoints.  It is kind of a gross fix because it requires that the cycle collector iterate over compartments, even if there are no watchpoints.
Comment 6 Andrew McCreight [:mccr8] 2011-10-12 11:49:19 PDT
Created attachment 566598 [details] [diff] [review]
tell the cycle collector about watchpoints

Now with test!  I couldn't use Jesse's original test, because there the watchpoint stays alive as long as the document itself.  I confirmed that this current test leaks without my patch, and doesn't leak with the test.  Trying to observe the callback directly made the weak go away, which is weird, but whatever.

This patch is on top of my stack of patches for weak map cycle collector integration.
Comment 7 Andrew McCreight [:mccr8] 2011-11-24 09:47:41 PST
Created attachment 576790 [details] [diff] [review]
tell the cycle collector about watchpoints

Updated the patch to deal with write barriers.  Now that WeakMap-CC integration has landed, I'll hopefully get around to testing this in the next week or so.
Comment 8 Nicholas Nethercote [:njn] 2012-01-18 03:26:55 PST
mccr8, sounds like this is close to being ready for review?
Comment 9 Andrew McCreight [:mccr8] 2012-01-18 05:16:08 PST
Yeah, sorry, I should refresh this and get it done.  I'll do that this week.
Comment 10 Andrew McCreight [:mccr8] 2012-01-26 10:34:36 PST
Created attachment 591850 [details] [diff] [review]
tell the cycle collector about watchpoints

I confirmed that the attached test case fails without the patch (and leaks a lot...), and passes with it.  This patch refresh is just some minor unbitrotting.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ffb1f7901bd5
Comment 11 Andrew McCreight [:mccr8] 2012-01-26 15:16:05 PST
try looks reasonable.
Comment 12 Jason Orendorff [:jorendorff] 2012-01-26 16:35:18 PST
Comment on attachment 591850 [details] [diff] [review]
tell the cycle collector about watchpoints

OK.

Someday we'll change the implementation of watchpoints to use something like
WeakMap<JSObject *, ObjectWatchpoints> where class ObjectWatchpoints contains a
HashMap<jsid, Watchpoint>.
Comment 13 Andrew McCreight [:mccr8] 2012-01-27 17:15:17 PST
Thanks.

(In reply to Jason Orendorff [:jorendorff] from comment #12)
> Comment on attachment 591850 [details] [diff] [review]
> Someday we'll change the implementation of watchpoints to use something like
> WeakMap<JSObject *, ObjectWatchpoints> where class ObjectWatchpoints
> contains a HashMap<jsid, Watchpoint>.

That would be nice.  I also have a long-term plan to move JS marking by the CC more into the JS engine, so we can keep the CC in the dark about things like that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9801e9475d3b

Thanks for the reminder, njn, I just lost track of this.
Comment 14 Nicholas Nethercote [:njn] 2012-01-27 17:40:46 PST
mccr8, can you briefly summarize what this patch did?  Did watchpoints cause some leaks?  (What are watchpoints?)
Comment 15 Andrew McCreight [:mccr8] 2012-01-27 17:49:05 PST
Here's some documentation for watchpoints: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/watch  "These two methods are implemented only in Gecko, and they're intended primarily for debugging use. In addition, using watchpoints has a serious negative impact on performance, which is especially true when used on global objects, such as window."

In general, cycles between C++ and JS are never broken unless the cycle collector knows about them.  Watchpoints are a form of weak reference, so they require special handling in the GC, and nobody ever told the CC about them. :(  But with the weak map + CC patch I landed before, we can view a watchpoint as a degenerate form of weak map with a single binding, where the map is alive.  As jorendorff said, watchpoints could be reimplemented using WeakMaps.

So yeah, if you make a watchpoint watch itself, or something like that, via a DOM node, then the GC would just keep it alive forever.  I don't know how common they are.  Jesse fuzzed this up.
Comment 16 Joe Drew (not getting mail) 2012-01-28 19:01:00 PST
https://hg.mozilla.org/mozilla-central/rev/9801e9475d3b

Note You need to log in before you can comment on or make changes to this bug.