Don't visit WeakMap keys from the WeakMap in markEntry

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: general → continuation
(Assignee)

Comment 1

6 years ago
Created attachment 558160 [details] [diff] [review]
don't visit keys in markEntry
(Assignee)

Comment 2

6 years ago
Try run of this came back looking okay.
(Assignee)

Updated

6 years ago
Attachment #558160 - Flags: review?(jorendorff)

Comment 3

6 years ago
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 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.
Attachment #558160 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 5

6 years ago
Will do, thanks!
Status: NEW → ASSIGNED
(Assignee)

Comment 6

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/1c432b6fdbdb
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/1c432b6fdbdb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 685841
Blocks: 687843
For the post-verifier, we really need the keys to be visited as well.

Bug 772229
(Assignee)

Comment 9

5 years ago
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...
You need to log in before you can comment on or make changes to this bug.