768282 - GCCycle may be run multiple times for any CC_FORCED GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

People with lots of tabs open are now getting a lot of CC_FORCED GCs, which is a problem that needs to be fixed. However, when these GCs happen, the code in bug 754588 sets the flag to clean up everything. That means that we keep re-doing the GC until the gcPoke flag doesn't get set. This means we may be running the GC over and over many times. There's no point in doing this.

This patch adds a separate GC reason for shutdown CCs, as suggesting in bug 754588.

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 636554 [details] [diff] [review]
patch

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

> People with lots of tabs open are now getting a lot of CC_FORCED GCs, which is a problem that needs to be fixed.
Ouch.  Do you have any idea why that is?  I should look at telemetry...

This mostly looks good, I think you just don't need to add the extra flag to GCIfNeeded.

::: js/src/jsgc.cpp
@@ +3816,4 @@
>  {
>      // During shutdown, we must clean everything up, for the sake of leak
>      // detection. When a runtime has no contexts, or we're doing a forced GC,
>      // those are strong indications that we're shutting down.

nit: Update the above comment, please, as a forced CC no longer indicates we're shutting down.

::: xpcom/base/nsCycleCollector.cpp
@@ +2578,5 @@
>  // this CC. It returns true on startup (before the mark bits have been set),
>  // and also when UnmarkGray has run out of stack.  We also force GCs on shut 
>  // down to collect cycles involving both DOM and JS.
>  void
> +nsCycleCollector::GCIfNeeded(bool aForceGC, bool aForShutdown)

I think this change is unnecessary.  The only forced GCs that aren't at shutdown are due to wantAllTraces, which should only happen when we're dumping the CC heap, and even then, we rarely use wantAllTraces.

@@ +2604,5 @@
>  
>      // mJSRuntime->Collect() must be called from the main thread,
>      // because it invokes XPCJSRuntime::GCCallback(cx, JSGC_BEGIN)
>      // which returns false if not in the main thread.
> +    mJSRuntime->Collect(aForShutdown ? js::gcreason::SHUTDOWN_CC : js::gcreason::CC_FORCED,

With my above suggested change, if aForceGC is true, then the reason should be SHUTDOWN_CC, otherwise it is CC_FORCED.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

Here's the updated patch.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df43767956ba

Comment 4

[Ed Morley [:emorley]]

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