GCCycle may be run multiple times for any CC_FORCED GC

RESOLVED FIXED in mozilla16



JavaScript Engine
5 years ago
5 years ago


(Reporter: billm, Assigned: billm)



Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



5 years ago
Created 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. 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.
Attachment #636554 - Flags: review?(continuation)
Comment on attachment 636554 [details] [diff] [review]

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.
Attachment #636554 - Flags: review?(continuation)

Comment 2

5 years ago
Created attachment 636822 [details] [diff] [review]
patch v2

Here's the updated patch.
Attachment #636554 - Attachment is obsolete: true
Attachment #636822 - Flags: review?(continuation)
Attachment #636822 - Flags: review?(continuation) → review+

Comment 3

5 years ago
Target Milestone: --- → mozilla16

Comment 4

5 years ago
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.