1337450 - Simplify the implementation of GC resets

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: simplify-abort-incremental

At the moment we call GCRuntime::resetIncrementalGC from several places.  This patch changes things so that this always happens from budgetIncrementalGC.  The implementation of abortGC now just starts a collection with the ABORT_GC reason which that method checks for.

This is needed for running the GC from its own thread because it means that resets now always happen on the GC thread.

Comment 1

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

Comment on attachment 8834504 [details] [diff] [review]
simplify-abort-incremental

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

::: js/src/jsgc.cpp
@@ +6224,1 @@
>      if (prevState != State::NotActive && !isIncrementalGCInProgress())

Would it be possible to eliminate this test? It feels like it's inferring what budgetIncrementalGC did, instead of having it just tell it directly. Maybe:

  budgetIncrementalGC(nonincrementalByAPI, reason, budget, &resetGC, session.lock);
  if (resetGC)
    return true;

Unless there's some nuance I'm missing.

Or, given that we're already using a boolean return from gcCycle to mean wasReset, I suppose we could do the same here. Or better, change both of them to an enum { GCWasReset, GCWasNotReset }. Ugh, those names are pretty bad. enum class GCResult { Ok, Reset }?

@@ -6471,5 @@
>      checkCanCallAPI();
>      MOZ_ASSERT(!TlsContext.get()->suppressGC);
>  
> -    AutoStopVerifyingBarriers av(rt, false);
> -    AutoEnqueuePendingParseTasksAfterGC aept(*this);

Heh, seems we had another batch of these nested inside collect(). Yay for getting rid of lots of redundant goop here.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #1)
> Or, given that we're already using a boolean return from gcCycle to mean
> wasReset, I suppose we could do the same here. Or better, change both of
> them to an enum { GCWasReset, GCWasNotReset }. Ugh, those names are pretty
> bad. enum class GCResult { Ok, Reset }?

That is a great idea, I'll do that.

Comment 3

[Pulsebot]

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

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/67160e6118d1