1466020 - Try always yielding before sweeping in incremental collections

Status: NEW

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

Description

[Paul Bone [:pbone] (PTO until 14/8/2025)]

jonco suggested always yeilding between marking and sweeping when we're doing incremental collection.  The previous behavior was to yield if we'd already yeidled once already (not-tiny collection).

Comment 1

[Paul Bone [:pbone] (PTO until 14/8/2025)]

Attachment: Bug 1466020 - Always yeild before sweeping in incremental collections

This gives the two slice zeal modes an extra slice.  I have the feeling that
this was intended all along but the "two slice" was just a bad name.

If that's the case I can add another patch to rename them.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8982405 [details] [diff] [review]
Bug 1466020 - Always yeild before sweeping in incremental collections

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

The name is correct: two slice zeal modes are only intended to have two slices.  This is so that you can test what happens if we yield at a specific point in the collection.

I was thinking some more about the avoiding nursery collection change and wondered why we have to do this extra yield.  As I understand it, we currently yield before sweeping if marking took more than one slice.  If we haven't yielded yet then we start sweeping straight away.  If this is the case, then we don't actually need to yield here to avoid nursery collection because the one we did at the start of the collection will keep us safe.

Comment 3

[Paul Bone [:pbone] (PTO until 14/8/2025)]

(In reply to Jon Coppeard (:jonco) from comment #2)
> Comment on attachment 8982405 [details] [diff] [review]
> Bug 1466020 - Always yeild before sweeping in incremental collections
> 
> Review of attachment 8982405 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The name is correct: two slice zeal modes are only intended to have two
> slices.  This is so that you can test what happens if we yield at a specific
> point in the collection.

Okay, I'll re-check the logic in this patch.

> I was thinking some more about the avoiding nursery collection change and
> wondered why we have to do this extra yield.  As I understand it, we
> currently yield before sweeping if marking took more than one slice.  If we
> haven't yielded yet then we start sweeping straight away.  If this is the
> case, then we don't actually need to yield here to avoid nursery collection
> because the one we did at the start of the collection will keep us safe.

I think you're right.  I'll re check the logic there also and make sure that because of this, it can remain simple.

Comment 4

[Paul Bone [:pbone] (PTO until 14/8/2025)]

(In reply to Paul Bone [:pbone] from comment #3)
> (In reply to Jon Coppeard (:jonco) from comment #2)
> > I was thinking some more about the avoiding nursery collection change and
> > wondered why we have to do this extra yield.  As I understand it, we
> > currently yield before sweeping if marking took more than one slice.  If we
> > haven't yielded yet then we start sweeping straight away.  If this is the
> > case, then we don't actually need to yield here to avoid nursery collection
> > because the one we did at the start of the collection will keep us safe.
> 
> I think you're right.  I'll re check the logic there also and make sure that
> because of this, it can remain simple.

I have thought further about this (and checked my own notes).

If we start an incremental collection (and therefore don't collect the nursery at the beginning).  Then yield part way through and the mutator allocates some things into the nusery, then continue the collection and part-way through decide to collect it non-incrementally, then the nursery not be empty and yet this condition won't fire because we're in a non-incremental mode.