Enable incremental weakmap marking in the browser
Categories
(Core :: JavaScript: GC, defect, P1)
Tracking
()
People
(Reporter: sfink, Assigned: sfink)
References
(Blocks 2 open bugs, Regression)
Details
(Keywords: regression, sec-other, Whiteboard: [post-critsmash-triage])
Attachments
(4 files, 1 obsolete file)
As I was horrified to discover, bug 1633176 did not enable incremental weakmap marking in the browser after all. It only enabled it in the shell. I did not update the browser's prefs file, so because it's a boolean pref it defaulted to false.
I also found one bug when enabling it in the browser.
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
Also weaken an overly strong assertion.
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
Assignee | ||
Comment 3•4 years ago
|
||
Also note that with this bug and bug 1667912 fixed, I don't get any more errors when swapping the order of weak/gray marking, whether with incremental or non-incremental weakmap marking.
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
It seems safest to land bug 1667912 first, to fix the current non-incremental weakmap marking. At any point thereafter, the two patches here might then be landed either separately or together. One fixes incremental weakmap marking, the other enables it, but I believe there is still at least one other bug still remaining in the incremental version (bug 1658654).
Comment 5•4 years ago
|
||
There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:sfink, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 6•4 years ago
|
||
I think this should probably be a sec-other. The current situation, where weakmap marking is still nonincremental, is a bug but it's safer and currently more correct. At the same time, I would rather not have it exposed that this is what's going on.
Before landing this bug, any remaining known issues in the incremental mode of weakmap marking should be resolved.
I'll try to set dependencies.
Updated•3 years ago
|
Comment 7•3 years ago
|
||
Landed: https://hg.mozilla.org/integration/autoland/rev/089c068b554189074be97335d577a0a747f79866
Backed out for causing assertion failures related to Marking.cpp
Failure log: https://treeherder.mozilla.org/logviewer?job_id=354676787&repo=autoland
Backout link: https://hg.mozilla.org/integration/autoland/rev/26f5cb3f972d61429a91b7aa9c3cca13603c36bc
Assignee | ||
Comment 8•3 years ago
|
||
Assignee | ||
Comment 9•3 years ago
|
||
The assertion failure I looked at turned out to be kind of nasty. (But I could reproduce in Pernosco, which really helped.)
We mark a map in zone 1 and find an unmarked CCW key to a delegate in zone 2, so add a <weakmap, delegate> -> key edge to the ephemeron table. Later in the same incremental GC, zone 1 gets nuked. Then we enter sweeping, and compute sweep groups. Zone 1 is put into sweep group 3, zone 2 is sweep group 4, because at this point in time there's no edge between the zones since the CCW was nuked. We finish sweep group 3, then when marking the delegate in sweep group 4 we try to mark the key while its zone is no longer marking.
Because nuking a CCW will conservatively mark the key already, these ephemeron edges are unneeded. I thought they were harmless, but this shows that they are at least going to cause a DEBUG assertion failure. It's pretty easy to erase them, though.
Comment 10•3 years ago
|
||
Landed:
https://hg.mozilla.org/integration/autoland/rev/00cc848acd05d3fd0a92afc78702c9324394b8e8
https://hg.mozilla.org/integration/autoland/rev/1688c63ebe62daa652befa06c54897301273fcf3
Backed out for causing assertion failures on Marking.cpp:
https://hg.mozilla.org/integration/autoland/rev/61ecccbc80e6cfc6e83e172a557efeed4c7d6405
Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=JqwLedjdTlOz5Kc9Ror2fA.0&resultStatus=usercancel%2Ctestfailed%2Cbusted%2Cexception%2Cretry&revision=feea84165bf0c8622c415064d405d8fd5f7a1a7d
Failure log: https://treeherder.mozilla.org/logviewer?job_id=355509136&repo=autoland
Assertion failure: !key->zone()->gcEphemeronEdges(key).has(key) (non-collecting zone should not have populated gcEphemeronEdges), at /builds/worker/checkouts/gecko/js/src/gc/Marking.cpp:882
Assignee | ||
Comment 11•3 years ago
|
||
Assignee | ||
Comment 12•3 years ago
|
||
Comment 13•3 years ago
|
||
remove ephemeron edges that are invalidated by nuking r=jonco
https://hg.mozilla.org/integration/autoland/rev/ea077c9e7d5acdef357957e8f427206e072464bd
https://hg.mozilla.org/mozilla-central/rev/ea077c9e7d5a
discard ephemeron table when resetting incremental GC r=jonco
https://hg.mozilla.org/integration/autoland/rev/d7c2c27749b7e585c578153d2e6e7e466cbac973
https://hg.mozilla.org/mozilla-central/rev/d7c2c27749b7
Enable incremental weakmap marking in the browser r=jonco
https://hg.mozilla.org/integration/autoland/rev/e29de9bd3d07e46b03bd7486bfbb34690d93446d
https://hg.mozilla.org/mozilla-central/rev/e29de9bd3d07
Weaken assert so that it need not hold when un-nuking a weakmap key's delegate. r=jonco
https://hg.mozilla.org/integration/autoland/rev/26cdb4726136737670d0cb97c5bf059bf7496bb3
https://hg.mozilla.org/mozilla-central/rev/26cdb4726136
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•2 years ago
|
Description
•