Closed Bug 681104 Opened 11 years ago Closed 11 years ago
Add flag to make JS
_Tracers not visit Weak Map bindings
Currently, JS_TraceChildren tells non-GCmarker JSmarkers that there's an edge from a weak map to its key, and to its children, which is wrong. With the addition of JS_TraceWeakMaps from Bug 668855, we should be able to fix these clients so they see the proper heap graph. Though I'm not sure exactly how that should be set up for all of these cases. Maybe I'll split these into different bugs when I get down to it. Here are the uses of JS_TraceChildren: - jsapi: JS_DumpHeap - shell/js: CountHeap (this just counts the number of each object, it looks like, so the current behavior should be okay) - jsheaptools: reverseHeap - xpconnect: UnmarkGrayChildren/UnmarkGrayChildrenRecursive: these are a little weird. Consider where m, k and v are all gray. I think this is okay right now, because the value can only be accessed when both the map and key are handed out. By the time that is done, the value will be ungrayed (map ungrays value), so we're okay. But if we only hand out the map, then the key and value get ungreyed, which we don't need to do in general. But being more precise might require the same sort of annoying MarkAllIteratively crud we do right now, so it may be annoying. Things that are okay as is: - jsgc: MarkDelayedChildren: this is actually a GCMarker, so the type of trc should be updated to reflect that. - xpconnect: NoteJSRoot, NoteJSChildren: only called with non-JSTRACE_OBJECT, so it won't ever be called on WeakMaps. nsXPConnect::Traverse: this is the place it is used with the cycle collector, so I will fix that in Bug 668855.
re: UnmarkGrayChildren/UnmarkGrayChildrenRecursive: Also, values are currently always marked black (not gray), so for the current state of things, it doesn't matter what JS_TraceChildren does with WeakMaps. This seems to be the only critical use of JS_TraceChildren, aside from the cycle collector, so that's good.
To update UnmarkGrayChildren, we don't want to actually scan every WeakMap when this is called, so JS_TraceWeakMaps is not really the right interface for it. The simplest thing to do may be to unmark gray all values contained in a WeakMap when the weak map is unmark grayed, but I'm not sure how to implement that.
I think the easiest way to fix this is to add a new flag to JSTracers called something like visitWeakMappings. If this is true, then the tracer behavior is like it currently is: visit each binding as though it was a real child. By default, this would be true. For the cycle collector tracer, we would set this to false. I could land the JSTracer changes separately in this bug, leaving visitWeakMappings to True for all clients, then flip the bit in the WeakMap CC integration bug, or just land all of it as part of that bug. On the one hand, it would be kind of weird to leave vestigial code like that, but on the other hand the integration patch is pretty huge already. Separately, I think we don't ever need to visit WeakMap keys from the WeakMap. I filed Bug 684595 for this.
Assignee: general → continuation
I'm considering calling WeakMap bindings "weak mappings". Calling them bindings is confusing with regards to DOM bindings, and "weak map mappings" is rather ugly.
Okay, so I learned today there's a reason people are using bool instead of JSBool in jsapi.h...
Attachment #558161 - Attachment is obsolete: true
Try run for 18771b4ae557 is complete. Detailed breakdown of the results available here: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=18771b4ae557 Results (out of 18 total builds): exception: 11 failure: 7 Builds available at http://email@example.com
A second try run for the fixed patch (on top of the patch in Bug 684595) came back okay.
Try run for ab335d3e7728 is complete. Detailed breakdown of the results available here: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=ab335d3e7728 Results (out of 152 total builds): exception: 1 success: 143 warnings: 7 failure: 1 Builds available at http://firstname.lastname@example.org
Summary: Fix non-GCmarker, non-CC clients of JS_TraceChildren wrt weak maps → Add flag to make JS_Tracers not visit WeakMap bindings
This patch adds a flag to disable eagerly visiting WeakMap bindings. Most tracers (like UnmarkGrayChildren and DumpHeap) just use the liveness of entries computed by the GC. When the cycle collector is properly integrated with weak maps, then we need to disable this behavior, and instead separately track weak map bindings. The patch here should not change the actual behavior. Instead, it puts the framework in place to change the CC tracer behavior. I don't want to change the behavior of the CC before landing the rest of the WeakMap integration. In addition, I have a minor bit of cleanup, where I change the type of MarkDelayedChildren to take a GCMarker instead of a JSTracer, because it is only called with GCmarkers, and this makes that more clear.
Comment on attachment 559493 [details] [diff] [review] add flag to disable visiting weak map bindings Removing my review request for now, this doesn't seem to be doing what I want, somehow.
Okay, the problem was not with this patch per se, but with how I was trying to set the flag in a later patch. I was setting it to false, then JS_TRACER_INIT was setting it back to true... Sorry about that.
Comment on attachment 559608 [details] [diff] [review] add flag to disable visiting weak map bindings Looks good. I think we want JS_FALSE in place of false/PR_FALSE.
Attachment #559608 - Flags: review?(wmccloskey) → review+
I only used JSBool because bool is not defined in C. jsgc.cpp only uses false, not JS_FALSE, so I think I should use that there. XPConnect is pretty all over the place, so I'm find with switching to JS_FALSE there. There's some kind of project to eliminate the million bool types that should be landing soon, so I may have to adjust my patch depending on when I get around to this.
I think the version of false we use should correspond to the type. So if it's JSBool, we should use JS_TRUE/JS_FALSE. Although PRBool is disappearing, I don't think JSBool is going to go away soon. We don't have any plans to move away from a C JSAPI AFAIK.
Makes sense. I'll change it in both places.
Target Milestone: --- → mozilla9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.