Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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.