As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 768282 - GCCycle may be run multiple times for any CC_FORCED GC
: GCCycle may be run multiple times for any CC_FORCED GC
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bill McCloskey (:billm)
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-06-25 17:11 PDT by Bill McCloskey (:billm)
Modified: 2012-06-27 03:36 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (6.30 KB, patch)
2012-06-25 17:11 PDT, Bill McCloskey (:billm)
no flags Details | Diff | Splinter Review
patch v2 (3.70 KB, patch)
2012-06-26 12:07 PDT, Bill McCloskey (:billm)
continuation: review+
Details | Diff | Splinter Review

Description User image Bill McCloskey (:billm) 2012-06-25 17:11:49 PDT
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.
Comment 1 User image Andrew McCreight [:mccr8] 2012-06-25 17:26:44 PDT
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.
Comment 2 User image Bill McCloskey (:billm) 2012-06-26 12:07:03 PDT
Created attachment 636822 [details] [diff] [review]
patch v2

Here's the updated patch.
Comment 4 User image Ed Morley [:emorley] 2012-06-27 03:36:01 PDT

Note You need to log in before you can comment on or make changes to this bug.