Closed Bug 1323083 Opened 4 years ago Closed 3 years ago

Change mark bit interpretation to allow incremental gray marking

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: billm, Assigned: jonco)

References

Details

(Keywords: perf, Whiteboard: [qf:p3])

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(jcoppeard)
Flags: needinfo?(jcoppeard)
Attached patch marking patch (obsolete) — Splinter Review
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.
(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 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.
Attachment #8852494 - Flags: feedback+
Updated patch.

I've put this through try and it works.
Assignee: nobody → jcoppeard
Attachment #8852494 - Attachment is obsolete: true
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.
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.
Whiteboard: [qf:p1]
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05908613d1b
Change representation of GC mark state to enable incremental gray marking r=jonco
Keywords: leave-open
Are we waiting on bug 1167452 to land before we close this?
Flags: needinfo?(jcoppeard)
(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.
Flags: needinfo?(jcoppeard)
Whiteboard: [qf:p1] → [qf:p2]
Keywords: perf
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..
Flags: needinfo?(jcoppeard)
Never mind.  Moving to p1 since we can't leave it at p2 anyway, and p3 is not appropriate.
Flags: needinfo?(jcoppeard)
Whiteboard: [qf:p2] → [qf:p1]
Moving to p3 after IRC discussion with djvj.  This a fairly large chunk of work blocked behind other work (e.g. bug 1167452).
Depends on: é
No longer depends on: 1167452
Whiteboard: [qf:p1] → [qf:p3]
Depends on: 1167452
No longer depends on: é
Priority: -- → P3
Blocks: 1463462
I'm going to close this bug and split off the main part of the work into bug 1463462.
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Summary: Incrementalize gray marking → Change mark bit interpretation to allow incremental gray marking
This was fixed in FF 55.
Depends on: 1459568
You need to log in before you can comment on or make changes to this bug.