1667913 - Enable incremental weakmap marking in the browser

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

[Steve Fink [:sfink] [:s:]]

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.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1667913 - After restoring a delegate to a key, only update gcWeakKeys if the delegate zone needs barriers.

Also weaken an overly strong assertion.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1667913 - Enable incremental weakmap marking in the browser

Comment 3

[Steve Fink [:sfink] [:s:]]

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.

Comment 4

[Steve Fink [:sfink] [:s:]]

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

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 6

[Steve Fink [:sfink] [:s:]]

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.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/089c068b554189074be97335d577a0a747f79866

Backed out for causing assertion failures related to Marking.cpp

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel&fromchange=089c068b554189074be97335d577a0a747f79866&tochange=26f5cb3f972d61429a91b7aa9c3cca13603c36bc&selectedTaskRun=aoUUKYhnT5K-o8LX-33WNQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=354676787&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/26f5cb3f972d61429a91b7aa9c3cca13603c36bc

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1667913 - remove ephemeron edges that are invalidated by nuking

Comment 9

[Steve Fink [:sfink] [:s:]]

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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 11

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1667913 - discard ephemeron table when resetting incremental GC

Comment 12

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1667913 - Weaken assert so that it need not hold when un-nuking a weakmap key's delegate.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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