Closed
Bug 693527
Opened 13 years ago
Closed 12 years ago
add Watchpoint support to the cycle collector
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: jruderman, Assigned: mccr8)
References
Details
(Keywords: memory-leak, testcase, Whiteboard: [MemShrink:P2])
Attachments
(3 files, 3 obsolete files)
Bug 639235 mentioned that watchpoints have something in common with WeakMap. Could this bug be related to the WeakMap leaks?
Assignee | ||
Comment 1•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
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
Assignee | ||
Comment 3•13 years ago
|
||
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...
Assignee: general → continuation
Assignee | ||
Updated•13 years ago
|
Depends on: 668855
OS: Mac OS X → All
Hardware: x86_64 → All
Summary: Leak nsDocument and nsGlobalWindow with Object.prototype.watch → add Watchpoint support to the cycle collector
Whiteboard: [MemShrink]
Updated•13 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 4•13 years ago
|
||
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;
Assignee | ||
Comment 5•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
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.
Attachment #566574 -
Attachment is obsolete: true
Assignee | ||
Comment 7•13 years ago
|
||
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.
Attachment #566598 -
Attachment is obsolete: true
Comment 8•12 years ago
|
||
mccr8, sounds like this is close to being ready for review?
Assignee | ||
Comment 9•12 years ago
|
||
Yeah, sorry, I should refresh this and get it done. I'll do that this week.
Assignee | ||
Comment 10•12 years ago
|
||
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
Attachment #576790 -
Attachment is obsolete: true
Attachment #591850 -
Flags: review?(jorendorff)
Assignee | ||
Comment 11•12 years ago
|
||
try looks reasonable.
Comment 12•12 years ago
|
||
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>.
Attachment #591850 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 13•12 years ago
|
||
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.
Target Milestone: --- → mozilla12
Comment 14•12 years ago
|
||
mccr8, can you briefly summarize what this patch did? Did watchpoints cause some leaks? (What are watchpoints?)
Assignee | ||
Comment 15•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9801e9475d3b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•