1323083 - Change mark bit interpretation to allow incremental gray marking

Status: RESOLVED FIXED

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

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Gray marking seems to be taking a lot of time according to telemetry. Incrementalizing this phase could be tricky: once we've marked something gray, we don't have an easy way to mark it black again. That's something we would need to do if we mark an object gray and then go back to mutator, where it could access the object via a read barrier, turning it black.

To make this work, I think we need to somehow ensure that:
1. isMarked should return false on a gray cell if we're marking black.
2. markIfUnmarked should return true if a gray cell is being marked black (and it should change change the color to black).
3. unmark needs to continue to convert gray cells to black.

If we could arrange all that (perhaps by changing the bit patterns for the different colors), then I suspect we could just make gray marking incremental. Jon, does this sound feasible to you?

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: marking patch

I talked to Steve about this today. This patch changes the way the mark bits work so that we can mark things black after they've already been marked gray (black takes precedence).

Once we do this, and once bug 1167452 is done, I think we can incrementalize everything in endMarkingSweepGroup. We would need to ensure that we have a single atomic run of it at the end to ensure that we got everything. But we could run it incrementally as much as we wanted before that.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Bill McCloskey (:billm) from comment #0)
Sorry for the lack of reply to your needinfo.  Yes, that does all sound fine.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8852494 [details] [diff] [review]
marking patch

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

These changes look good to me.

::: js/src/gc/Heap.h
@@ +917,5 @@
>  
>      MOZ_ALWAYS_INLINE void unmark(const Cell* cell, uint32_t color) {
>          uintptr_t* word, mask;
> +        getMarkWordAndMask(cell, ColorBit::BlackBit, &word, &mask);
> +        *word |= mask;

Previously you could call this method with color == BLACK and it would unset the black bit, although that is never used.  This isn't true any more.

Maybe we should remove the argument and call this method unmarkGray, or just markBlack.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1323083-mark-bits v2

Updated patch.

I've put this through try and it works.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I have a possibly stupid question...

I have a stress test that shows us janking due to this TimeoutManager::UnmarkGrayTimers() loop when we have insane numbers of timers:

https://dxr.mozilla.org/mozilla-central/source/dom/base/TimeoutManager.cpp#1255

Is this something that could be incrementalized as part of these changes?

I haven't spent too much time looking into it since its not clear we should optimize for "insane numbers of timers".  If its something that could be achieved as a byproduct of this, though, it might be nice.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

That code is part of cycle collection. We might be able to incrementalize it, but I don't think this bug would help with that.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05908613d1b
Change representation of GC mark state to enable incremental gray marking r=jonco

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e05908613d1b

Comment 9

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/projects/cedar/rev/e05908613d1bc235d91a5a13b19df6c61bb1eef1
Bug 1323083 - Change representation of GC mark state to enable incremental gray marking r=jonco

Comment 10

[Andrew Overholt [:overholt]]

Are we waiting on bug 1167452 to land before we close this?

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Andrew Overholt [:overholt] from comment #10)
> Are we waiting on bug 1167452 to land before we close this?

Yes but there's more work to be done in this bug too when that one lands.

Comment 12

[Kannan Vijayan [:djvj]]

Hey Jon, can you give an short update on status on this bug?  Trying to figure out if we should triage it to qf:p1 or qf:p2, depending on what the timeline..

Comment 13

[Kannan Vijayan [:djvj]]

Never mind.  Moving to p1 since we can't leave it at p2 anyway, and p3 is not appropriate.

Comment 14

[Jon Coppeard (:jonco)]

Moving to p3 after IRC discussion with djvj.  This a fairly large chunk of work blocked behind other work (e.g. bug 1167452).

Comment 15

[Jon Coppeard (:jonco)]

I'm going to close this bug and split off the main part of the work into bug 1463462.

Comment 16

[Jon Coppeard (:jonco)]

This was fixed in FF 55.