Last Comment Bug 684595 - Don't visit WeakMap keys from the WeakMap in markEntry
: Don't visit WeakMap keys from the WeakMap in markEntry
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla9
Assigned To: Andrew McCreight [:mccr8]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 685841 687843
  Show dependency treegraph
Reported: 2011-09-04 10:01 PDT by Andrew McCreight [:mccr8]
Modified: 2012-07-09 14:42 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

don't visit keys in markEntry (4.11 KB, patch)
2011-09-04 10:27 PDT, Andrew McCreight [:mccr8]
jorendorff: review+
Details | Diff | Splinter Review

Description User image Andrew McCreight [:mccr8] 2011-09-04 10:01:04 PDT
During non-GC marking, we visit keys and values in a WeakMap.  Visiting values is nice because it means that the reachability relation implied by JS_TraceChildren is a strict subset of the real reachability relation, without the overhead of actually marking keys to get the true reachability relation.  However, I can't really see any justification for visiting keys, as there's never any path from a WeakMap to the key that it contains, so let's get rid of that.

See Bug 681104 for a list of non-GCmarking clients that use this.  It looks to me like they should all be okay with this change.
Comment 1 User image Andrew McCreight [:mccr8] 2011-09-04 10:27:34 PDT
Created attachment 558160 [details] [diff] [review]
don't visit keys in markEntry
Comment 2 User image Andrew McCreight [:mccr8] 2011-09-05 06:25:10 PDT
Try run of this came back looking okay.
Comment 3 User image Mozilla RelEng Bot 2011-09-06 22:20:53 PDT
Try run for 59444eacb69a is complete.
Detailed breakdown of the results available here:
Results (out of 152 total builds):
    exception: 1
    success: 143
    warnings: 7
    failure: 1
Builds available at
Comment 4 User image Jason Orendorff [:jorendorff] 2011-09-07 09:54:35 PDT
Comment on attachment 558160 [details] [diff] [review]
don't visit keys in markEntry

OK, looks good. While you're in here, would you please fix the comment about markEntryIfLive, just above the part you modified? It says:

//        A typical definition of markIteratively would be:

but the method name has changed; it's not markIteratively anymore.
Comment 5 User image Andrew McCreight [:mccr8] 2011-09-07 09:55:43 PDT
Will do, thanks!
Comment 6 User image Andrew McCreight [:mccr8] 2011-09-07 10:58:41 PDT
Comment 8 User image Terrence Cole [:terrence] 2012-07-09 14:38:47 PDT
For the post-verifier, we really need the keys to be visited as well.

Bug 772229
Comment 9 User image Andrew McCreight [:mccr8] 2012-07-09 14:42:50 PDT
In the interest of completeness, I should say that I have no idea why I decided we should not visit keys.  But I was quite convinced of it at the time...

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