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
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 Bill McCloskey (:billm) 2012-06-25 17:11:49 PDT
Created attachment 636554 [details] [diff] [review]
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] 2012-06-25 17:26:44 PDT
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 (:billm) 2012-06-26 12:07:03 PDT
Created attachment 636822 [details] [diff] [review]
patch v2

Here's the updated patch.
Comment 4 Ed Morley [:emorley] 2012-06-27 03:36:01 PDT
https://hg.mozilla.org/mozilla-central/rev/df43767956ba

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