Closed Bug 1335117 Opened 3 years ago Closed 3 years ago

Don't report that cells are gray in uncollected zones during incremental GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

At the moment we unmark gray cells that are the target of a black cross compartment wrapper when they are in an uncollected zone (see ShouldTraceCrossCompartment / GCRuntime::foundBlackGrayEdges / AutoExposeLiveCrossZoneEdges).

The problem with this is that if a cross compartment wrapper is marked due to a barrier then this will not happen until the next GC slice runs and the mutator can observe gray to black edges.  This is OK since we only need to prevent the CC from observing such edges, but it does mean we can get spurious assertion failures related to gray marking.

This patch causes us to not report cells as gray in this situation.  Hopefully this will reduce the number of gray marking assertion failures we see.

I'm also working on adding a full heap trace to verify there are no black->gray edges that will run just before the CC.
Attachment #8831760 - Flags: review?(sphink)
Comment on attachment 8831760 [details] [diff] [review]
relax-gray-asserts

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

I had to think through this for a while, but it makes sense.
Attachment #8831760 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efe0d03a98f1
Don't report that cells are gray in uncollected zones during incremental GC r=sfink
https://hg.mozilla.org/mozilla-central/rev/efe0d03a98f1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(jcoppeard)
Comment on attachment 8831760 [details] [diff] [review]
relax-gray-asserts

Approval Request Comment
[Feature/Bug causing the regression]: Incremental GC.
[User impact if declined]: No user impact, but possible spurious assertion failures in testing
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a simple change to report GC things as gray less often.  
[String changes made/needed]: None.
Flags: needinfo?(jcoppeard)
Attachment #8831760 - Flags: approval-mozilla-beta?
Attachment #8831760 - Flags: approval-mozilla-aurora?
Comment on attachment 8831760 [details] [diff] [review]
relax-gray-asserts

Fix an issue related to GC. Aurora53+.
Attachment #8831760 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
See Also: → 1336630
Comment on attachment 8831760 [details] [diff] [review]
relax-gray-asserts

gc fix for beta52
Attachment #8831760 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1346417
Depends on: 1352373
See Also: → 1359231
You need to log in before you can comment on or make changes to this bug.