1512045 - Simplify the way resetIncrementalGC() works

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

As mentioned in bug 1510145, I think it's possible to simplify the way resetIncrementalGC() works so that it doesn't call incrementalSlice() directly, but arranges for the following call in gcCycle to do the appropriate work (finishing the current GC).  This should result in a single use of AutoGCSession and a single call to incrementalSlice().

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1512045-simplify-reset

Patch to simplify resetIncrementalGC().  This no longer finishes the GC itself.  Instead this happens in gcCycle(), removing the early return.

I removed the unecessary return value from incrementalSlice().  I added an extra GC state 'Finish' so that aborting during marking could go to this state.  I removed the restoring of isCompacting which isn't used after the GC is reset.

The call to unmarkPreMarkedFreeCells() got moved to getNextSweepGroup() and I improved the assertions a little.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9030086 [details] [diff] [review]
bug1512045-simplify-reset

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

Nice refactoring!

::: js/src/gc/GC.cpp
@@ +165,3 @@
>   * the mark state, this just stops marking, but if we have started sweeping
>   * already, we continue until we have swept the current sweep group. Following a
>   * reset, a new non-incremental collection is started.

Rereading this comment makes me wonder if this is always a good idea. If we add a zone to an ongoing incremental GC, for example, it seems like we'd want to bail out of the current GC but then start a new incremental GC, not a nonincremental one.

It doesn't really matter now that we're down to like 2% nonincremental GCs, though I do kind of wonder what percentage of our long pauses can be blamed on nonincremental GCs?

Oh wait, never mind. I looked at nonincremental reasons. 71% of them are for GCBytesTrigger, and 0% of them are for ZoneChange. (Really? We're that good at not adding in zones now?)

@@ +6991,5 @@
>        MOZ_FALLTHROUGH;
>  
> +    case State::Finalize:
> +
> +    {

Did clang-format enforce this odd whitespacing? There's a blank line between the case and the open '{'. But perhaps that's what it does?

@@ +7369,1 @@
>        // State::MarkRoots can't ever happen here.

We don't really need the comment anymore.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> >   * the mark state, this just stops marking, but if we have started sweeping
> >   * already, we continue until we have swept the current sweep group. Following a
> >   * reset, a new non-incremental collection is started.
> 
> Rereading this comment makes me wonder if this is always a good idea. If we
> add a zone to an ongoing incremental GC, for example, it seems like we'd
> want to bail out of the current GC but then start a new incremental GC, not
> a nonincremental one

Oh wait, this was intentional?  I assumed that was a bug and fixed this with this patch (by making gcCycle no longer take a reference to the budget, so the second call gets the original budget again).  I agree this is undesirable.  I'll update the comment to reflect the new behaviour.

> Did clang-format enforce this odd whitespacing? There's a blank line between
> the case and the open '{'. But perhaps that's what it does?

I added this.  We've got a freestanding block to enforce the lifetime of an AutoPhase, and clang-format doesn't understand this and formats it as if the block is there for the statements following the case label, even though it ends well before the next case.  This looks confusing.  I'll try formatting it manually to what I think makes sense but I guess clang-format will overwrite this again.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b995059a83d2
Simplify GC resets r=sfink

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to Jon Coppeard (:jonco) from comment #3)
> (In reply to Steve Fink [:sfink] [:s:] from comment #2)
> > Did clang-format enforce this odd whitespacing? There's a blank line between
> > the case and the open '{'. But perhaps that's what it does?
> 
> I added this.  We've got a freestanding block to enforce the lifetime of an
> AutoPhase, and clang-format doesn't understand this and formats it as if the
> block is there for the statements following the case label, even though it
> ends well before the next case.  This looks confusing.  I'll try formatting
> it manually to what I think makes sense but I guess clang-format will
> overwrite this again.

Oh! Sorry, I didn't bother reading forward and just assumed that the bracket covered the entire case body. Given that, I'm fine with whatever. (I wonder if doing case: { { RAII; } ...code...; } would be most clear? Anyway, it sounds like you have good reasons for whatever indentation you pick.)

Comment 6

[Cristina Coroiu [:ccoroiu]]

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