Last Comment Bug 684595 - Don't visit WeakMap keys from the WeakMap in markEntry
: Don't visit WeakMap keys from the WeakMap in markEntry
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: 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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description 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 Andrew McCreight [:mccr8] 2011-09-04 10:27:34 PDT
Created attachment 558160 [details] [diff] [review]
don't visit keys in markEntry
Comment 2 Andrew McCreight [:mccr8] 2011-09-05 06:25:10 PDT
Try run of this came back looking okay.
Comment 3 Mozilla RelEng Bot 2011-09-06 22:20:53 PDT
Try run for 59444eacb69a is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=59444eacb69a
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-59444eacb69a
Comment 4 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 Andrew McCreight [:mccr8] 2011-09-07 09:55:43 PDT
Will do, thanks!
Comment 6 Andrew McCreight [:mccr8] 2011-09-07 10:58:41 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/1c432b6fdbdb
Comment 7 :Ehsan Akhgari (out sick) 2011-09-08 10:27:12 PDT
http://hg.mozilla.org/mozilla-central/rev/1c432b6fdbdb
Comment 8 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 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.