Remove the "foundBlackToGrayEdges" flag from GCRuntime

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox50 affected, firefox51 affected, firefox52 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8691615 [details] [diff] [review]
remove_setFoundBlackGrayEdges-v0.diff

The comment that sets it in ShouldMarkCrossCompartment claims that this can happen because of conservative scanning. If set, we later memset the mark bitmap. There is, of course, a comment telling us that it's totes safe, but without going into any substantial detail about how or why.

We no longer have conservative scanning, so I think we can just assert here.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5571c83b80a4
Attachment #8691615 - Flags: review?(jcoppeard)
(Assignee)

Comment 1

3 years ago
Comment on attachment 8691615 [details] [diff] [review]
remove_setFoundBlackGrayEdges-v0.diff

Uh oh. Try is not green. It seems the flimsy rationalizations in the comments were not the complete story!
Attachment #8691615 - Flags: review?(jcoppeard)
(In reply to Terrence Cole [:terrence] from comment #1)
Heh.  My guess is that a write barrier can mark the source black.  This comment is old and possibly predates incremental marking.
(Assignee)

Comment 3

3 years ago
(In reply to Jon Coppeard (:jonco) from comment #2)
> (In reply to Terrence Cole [:terrence] from comment #1)
> Heh.  My guess is that a write barrier can mark the source black.  This
> comment is old and possibly predates incremental marking.

And quite possibly compartmental GC as well!

It's not a barrier actually. What's going on is:
  * GC[n] marks |src| and |target| GREY
  * GC[n+1] is a compartmental GC containing the compartment of |src|, but not containing the compartment of |target|
  * GC[n+1] finds the cross compartment edge from |src| to |target| via tracing
  * |src| is BLACK because it was reset to WHITE at the start of GC[n+1] and subsequently traced
  * |target| is still |GREY| because it is not part of the GC
  *** KABOOM ***

How we solve this right now is to clobber the mark bits and force the CC to re-do an all-compartments collection before it can run again. This worries me: we don't actually solve the problem so we're likely to get right back into the same situation again. I think a better solution would be to actually enforce the invariants.

One way to do this would be to exposeToActiveJS somehow on the extra-gc |target|. The downside here is that we (probably) would get to grey marking and would proceed to re-mark this edge grey if it were in the GC. It would stay black and not be collectable by the CC until another GC refreshed the state.

Andrew, do you have thoughts?
Flags: needinfo?(continuation)
Can we just clobber the mark bits on all compartments we aren't collecting? Do we already do that? (I assume we treat compartments we aren't collecting as black for the purposes of holding onto things in the compartments we are collecting?) Then we could run the CC safely without collecting all compartments; we just won't collect anything in the non-collected compartments.
Flags: needinfo?(continuation)
(In reply to Andrew McCreight [:mccr8] from comment #4)
> Can we just clobber the mark bits on all compartments we aren't collecting?
> Do we already do that? 

That's effectively what we do already, if we find that this situation has occured: in endSweepPhase() we clear all the mark bits for uncollected compartments if we found and black to gray edges.

(In reply to Terrence Cole (PTO from 26 Nov to 7 Dec) from comment #3)
> One way to do this would be to exposeToActiveJS somehow on the extra-gc
> |target|. 

That sounds like it would work but would involve doing an unbounded amount of marking non-incrementally outside the confines of the selected zones we are collecting.  Instead, could we have a separate fixup operation invoked by the CC that updates the marking state by recursively marking black from all black to gray cross compartment edges?
(In reply to Jon Coppeard (:jonco) from comment #5)
> Instead, could we have a separate fixup operation invoked
> by the CC that updates the marking state by recursively marking black from
> all black to gray cross compartment edges?

The reason we currently trigger a full GC is that once there is a black-gray edge anywhere potentially somebody could read that gray pointer out of the black object, then write it into some other black object, creating new black-gray edges elsewhere at unknown locations. Maybe a write barrier could deal with that.
(Assignee)

Comment 8

2 years ago
Created attachment 8793005 [details] [diff] [review]
expose_black_to_gray_edges_instead_of_resetting_everything-v1.diff

This patch wasn't too difficult, although I'll wait on the try run before proclaiming anything specific about its other merits.
Attachment #8691615 - Attachment is obsolete: true
Attachment #8793005 - Flags: review?(jcoppeard)
(Assignee)

Comment 9

2 years ago
Looks like we're missing an "explicit" and one of our assertions is over-zealous. We can't actually assert at the end of the cycle that the black->gray target is still gray as it might have been marked later by something else.
(Assignee)

Comment 11

2 years ago
Created attachment 8793037 [details] [diff] [review]
expose_black_to_gray_edges_instead_of_resetting_everything-v2.diff

Fixing the initial round of errors turned up by the try run.
Attachment #8793005 - Attachment is obsolete: true
Attachment #8793005 - Flags: review?(jcoppeard)
Attachment #8793037 - Flags: review?(jcoppeard)
Comment on attachment 8793037 [details] [diff] [review]
expose_black_to_gray_edges_instead_of_resetting_everything-v2.diff

Review of attachment 8793037 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8793037 - Flags: review?(jcoppeard) → review+

Comment 13

2 years ago
Pushed by tcole@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cbb0c32eebf
Expose black to gray cross compartment edges instead of resetting the GC; r=jonco

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5cbb0c32eebf
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
status-firefox45: affected → ---
status-firefox50: --- → affected
status-firefox51: --- → affected
You need to log in before you can comment on or make changes to this bug.