1536061 - Investigate incrementally marking gray roots

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P5)

Description

[Jon Coppeard (:jonco)]

JS::Heap<T> is used for roots in the browser C++ heap and doesn't have a pre-barrier (these are the gray roots). This means that we have to copy these roots at the start of an incremental GC to enforce the snapshot at the beginning invariant (so we take an actual snapshot).

This happens non-incrementally and one of the main sources of budget overruns. Instead we may be able to add a pre-barrier and just trace these incrementally as necessary.

Comment 1

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #0)

For this to work we would need a way of only tracing the gray roots for specific zones. This is not trivial to implement as the browser doesn't have much concept of which roots belong to which zones.

Comment 2

[Andrew McCreight [:mccr8]]

The trickiness here is that the gray roots are C++ objects which don't have zones. A C++ object is created, then it is added as a gray root, then later a JS object might be stored in the C++ object. Only when the last step happens does the gray root really have a zone. This may have to deal with the case when the JS object held by a C++ gray root is replaced with a JS object from another zone, if that can happen. Writes are rare enough that this can probably be done by a write barrier (in fact, we don't really need to add something as a gray root until it contains a JS pointer), but the barrier needs to know the C++ object, so it feels like you'd need to change the type of setting a Heap<> pointer to take the C++ object?

Also, C++ objects might be able to hold JS objects from multiple zones, though I'd expect that this is rare enough that we can keep around a multizone gray root set and just always trace that.

Terrence did a similar thing a while back for some XPConnect black roots in bug 1214961.

Anyways, it seems doable, but yeah it would be a bit of work.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #0)

JS::Heap<T> is used for roots in the browser C++ heap and doesn't have a pre-barrier (these are the gray roots).

Although they don't have a pre-barrier they do have a read barrier. I think the read barrier will be sufficient to allow us to trace these incrementally after the first slice.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1536061 - Part 1: Change the gray root trace hook to allow gray roots to be marked incrementally r=sfink,mccr8

This adds a slice budget parameter and boolean return value to indicate whether tracing has finished.

Depends on D125431

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1536061 - Part 2: Support tracing gray roots incrementally in the cycle collector r=mccr8

This stores the iterator state between slices to allow us to trace these
incrementally.

Depends on D125558

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1536061 - Part 3: Trace gray roots incrementally r=sfink

This patch adds a new sweep action and passes the budget down through the trace hook.

Depends on D125559

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08cdd990b675
Change the gray root trace hook to allow gray roots to be marked incrementally r=sfink,mccr8
https://hg.mozilla.org/integration/autoland/rev/8c9135ac1d10
Support tracing gray roots incrementally in the cycle collector r=mccr8
https://hg.mozilla.org/integration/autoland/rev/248ec4a96a70
Trace gray roots incrementally r=sfink

Comment 8

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/08cdd990b675
https://hg.mozilla.org/mozilla-central/rev/8c9135ac1d10
https://hg.mozilla.org/mozilla-central/rev/248ec4a96a70

Comment 9

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

Backed out for conflicts with backout of Bug 1730534:
https://hg.mozilla.org/mozilla-central/rev/ae626497124eec384358afadbe6f34a6a182ebc4

Comment 10

[Jon Coppeard (:jonco)]

I'll reland this in the next cycle.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: Bug 1536061 - Part 4: Add some tests for incremental iteration of the holder map r?mccr8

Depends on D125560

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5cf84d1aab5
Part 1: Change the gray root trace hook to allow gray roots to be marked incrementally r=sfink,mccr8
https://hg.mozilla.org/integration/autoland/rev/777ffd0511c0
Part 2: Support tracing gray roots incrementally in the cycle collector r=mccr8
https://hg.mozilla.org/integration/autoland/rev/15a056766c44
Part 3: Trace gray roots incrementally r=sfink
https://hg.mozilla.org/integration/autoland/rev/f276bad53497
Part 4: Add some tests for incremental iteration of the holder map r=mccr8

Comment 13

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/b5cf84d1aab5
https://hg.mozilla.org/mozilla-central/rev/777ffd0511c0
https://hg.mozilla.org/mozilla-central/rev/15a056766c44
https://hg.mozilla.org/mozilla-central/rev/f276bad53497