Last Comment Bug 681104 - Add flag to make JS_Tracers not visit WeakMap bindings
: Add flag to make JS_Tracers not visit WeakMap bindings
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Andrew McCreight [:mccr8]
:
:
Mentors:
Depends on:
Blocks: 668855
  Show dependency treegraph
 
Reported: 2011-08-22 15:34 PDT by Andrew McCreight [:mccr8]
Modified: 2011-09-20 07:49 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add flag to disable visiting weak map bindings (4.93 KB, patch)
2011-09-04 10:51 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
add flag to disable visiting weak map bindings (4.93 KB, patch)
2011-09-04 11:30 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
add flag to disable visiting weak map bindings (6.36 KB, patch)
2011-09-09 10:00 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
add flag to disable visiting weak map bindings (6.47 KB, patch)
2011-09-09 16:06 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2011-08-22 15:34:35 PDT
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.
Comment 1 Andrew McCreight [:mccr8] 2011-08-22 15:43:56 PDT
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.
Comment 2 Andrew McCreight [:mccr8] 2011-09-03 12:55:30 PDT
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.
Comment 3 Andrew McCreight [:mccr8] 2011-09-04 10:04:57 PDT
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.
Comment 4 Andrew McCreight [:mccr8] 2011-09-04 10:51:59 PDT
Created attachment 558161 [details] [diff] [review]
add flag to disable visiting weak map bindings

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.
Comment 5 Andrew McCreight [:mccr8] 2011-09-04 11:30:29 PDT
Created attachment 558171 [details] [diff] [review]
add flag to disable visiting weak map bindings

Okay, so I learned today there's a reason people are using bool instead of JSBool in jsapi.h...
Comment 6 Mozilla RelEng Bot 2011-09-04 11:50:48 PDT
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://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/amccreight@mozilla.com-18771b4ae557
Comment 7 Andrew McCreight [:mccr8] 2011-09-05 06:12:03 PDT
A second try run for the fixed patch (on top of the patch in Bug 684595) came back okay.
Comment 8 Mozilla RelEng Bot 2011-09-06 23:20:51 PDT
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://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/amccreight@mozilla.com-ab335d3e7728
Comment 9 Andrew McCreight [:mccr8] 2011-09-09 10:00:02 PDT
Created attachment 559493 [details] [diff] [review]
add flag to disable visiting weak map 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 10 Andrew McCreight [:mccr8] 2011-09-09 15:28:26 PDT
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.
Comment 11 Andrew McCreight [:mccr8] 2011-09-09 16:06:59 PDT
Created attachment 559608 [details] [diff] [review]
add flag to disable visiting weak map bindings

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 12 Bill McCloskey (:billm) 2011-09-19 11:38:54 PDT
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.
Comment 13 Andrew McCreight [:mccr8] 2011-09-19 11:55:14 PDT
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.
Comment 14 Bill McCloskey (:billm) 2011-09-19 12:00:56 PDT
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.
Comment 15 Andrew McCreight [:mccr8] 2011-09-19 14:00:27 PDT
Makes sense.  I'll change it in both places.
Comment 16 Andrew McCreight [:mccr8] 2011-09-19 17:00:26 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c64b8574e282

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