Last Comment Bug 758034 - Clean up how the browser triggers GCs
: Clean up how the browser triggers GCs
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla16
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on: 770072 774201
Blocks: 761739
  Show dependency treegraph
 
Reported: 2012-05-23 15:12 PDT by Bill McCloskey (:billm)
Modified: 2012-07-16 01:37 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stop calling through xpconnect (13.91 KB, patch)
2012-05-23 15:12 PDT, Bill McCloskey (:billm)
bugs: review+
Details | Diff | Splinter Review
extra JS APIs (4.62 KB, patch)
2012-05-23 15:23 PDT, Bill McCloskey (:billm)
terrence: review+
Details | Diff | Splinter Review
use the APIs (2.90 KB, patch)
2012-05-23 15:28 PDT, Bill McCloskey (:billm)
bugs: review-
Details | Diff | Splinter Review
fix forced CCs (3.01 KB, patch)
2012-05-23 15:30 PDT, Bill McCloskey (:billm)
bugs: review+
Details | Diff | Splinter Review
stop calling through xpconnect, v2 (14.37 KB, patch)
2012-05-29 12:41 PDT, Bill McCloskey (:billm)
bugs: review+
Details | Diff | Splinter Review
use the APIs, v2 (2.83 KB, patch)
2012-05-29 12:42 PDT, Bill McCloskey (:billm)
bugs: review+
Details | Diff | Splinter Review
create separate inter-slice timer (4.15 KB, patch)
2012-06-10 16:16 PDT, Bill McCloskey (:billm)
bugs: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2012-05-23 15:12:31 PDT
Created attachment 626600 [details] [diff] [review]
stop calling through xpconnect

The main change here is to make nsJSContext invoke the GC directly, via jsfriendapi.h, rather than going through xpconnect. Going through xpconnect is really annoying, because every time we want to add a new GC mode, we have to change the IDL file and a few other things.

With this change, we can simplify nsXPConnect::Collect. It no longer needs to handle incremental GC.

I also added some enums for GC options in nsJSContext. I think this is a little easier to read.
Comment 1 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-05-23 15:17:27 PDT
I'll try to review this tomorrow, well today EET.
Comment 2 Bill McCloskey (:billm) 2012-05-23 15:17:38 PDT
Oh yeah, this patch also makes all GCs triggered from nsJSContext incremental. Olli, I'm pretty sure the instability you were seeing before was from bug 754989.
Comment 3 Bill McCloskey (:billm) 2012-05-23 15:23:37 PDT
Created attachment 626601 [details] [diff] [review]
extra JS APIs

This patch adds some extra jsfriendapi.h stuff that allows us to:

1. Select for GC all compartments that are being collected incrementally.
2. Finish off an already running incremental GC.
Comment 4 Bill McCloskey (:billm) 2012-05-23 15:28:11 PDT
Created attachment 626602 [details] [diff] [review]
use the APIs

This patch uses the APIs above to:
1. During an incremental GC, collect only the compartments that were scheduled for collection in previous slices.
2. Use the APIs to finish an already running incremental GC. Previously we would cancel the current one and do a new non-incremental GC.
Comment 5 Bill McCloskey (:billm) 2012-05-23 15:30:53 PDT
Created attachment 626604 [details] [diff] [review]
fix forced CCs

Olli, this is your patch. Without it, we do ~15 CC_FORCED GCs before we really CC, because of the ForgetSkippable calls.
Comment 6 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-05-24 02:42:08 PDT
Comment on attachment 626600 [details] [diff] [review]
stop calling through xpconnect


> [uuid(1239b432-b835-4d28-9dc0-53063cb7f60f)]
> interface nsIXPConnect : nsISupports
Update the uuid
Comment 7 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-05-24 03:01:48 PDT
Comment on attachment 626602 [details] [diff] [review]
use the APIs

># HG changeset patch
># Parent d2cd48411a47e228ea23964eedde67e11284d6c8
># User Bill McCloskey <wmccloskey@mozilla.com>
>diff --git a/dom/base/nsJSEnvironment.cpp b/dom/base/nsJSEnvironment.cpp
>--- a/dom/base/nsJSEnvironment.cpp
>+++ b/dom/base/nsJSEnvironment.cpp
>@@ -2940,22 +2940,29 @@ nsJSContext::GarbageCollectNow(js::gcrea
>   // loading and move on as if they weren't.
>   sPendingLoadCount = 0;
>   sLoadingInProgress = false;
> 
>   if (!nsContentUtils::XPConnect()) {
>     return;
>   }
> 
>+  if (sCCLockedOut) {
>+    // We're in the middle of incremental GC. Do another slice.
>+    js::PrepareForIncrementalGC(nsJSRuntime::sRuntime);
>+    js::IncrementalGC(nsJSRuntime::sRuntime, aReason);
>+    return;
>+  }
>+
But there are cases when we really want to run full GC here, not depending
on sCCLockedOut state. For example tools using DOMWindowUtils::GarbageCollect want
that collection to happen immediately, and do the full GC.
So, I think it should be something like
if (sCCLockedOut && aShrinking != ShrinkingGC && aCompartment != NonCompartmentGC)
  // We're in the middle of incremental GC. Do another slice.
  js::PrepareForIncrementalGC(nsJSRuntime::sRuntime);
  js::IncrementalGC(nsJSRuntime::sRuntime, aReason);
  return;
}
Or do we need to keep the aGlobal parameter in the method?
Comment 8 Andrew McCreight [:mccr8] 2012-05-24 10:08:48 PDT
Comment on attachment 626600 [details] [diff] [review]
stop calling through xpconnect

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

Nice cleanup!

::: dom/base/nsDOMWindowUtils.cpp
@@ +971,5 @@
>  #endif
>  
> +  nsJSContext::GarbageCollectNow(js::gcreason::DOM_UTILS,
> +                                 nsJSContext::NonCompartmentGC,
> +                                 nsJSContext::NonShrinkingGC);

You can drop the last two args.

::: dom/base/nsJSEnvironment.cpp
@@ +2916,5 @@
>  
>    uintptr_t reason = reinterpret_cast<uintptr_t>(aClosure);
>    nsJSContext::GarbageCollectNow(static_cast<js::gcreason::Reason>(reason),
> +                                 nsJSContext::NonCompartmentGC,
> +                                 nsJSContext::NonShrinkingGC);

You can drop these last two args.

Another thing I've noticed is that you are passing a reason via the closure here, but isn't the reason always going to be FULL_GC_TIMER?  So you could simplify that a bit.  Not a big deal, and not really in scope for this bug.

@@ +2997,5 @@
>    if (sCCLockedOut) {
>      // We're in the middle of an incremental GC; finish it first
> +    nsJSContext::GarbageCollectNow(js::gcreason::CC_FORCED,
> +                                   nsJSContext::NonCompartmentGC,
> +                                   nsJSContext::NonShrinkingGC);

You can drop the last two args.
Comment 9 Andrew McCreight [:mccr8] 2012-05-24 10:18:40 PDT
Comment on attachment 626601 [details] [diff] [review]
extra JS APIs

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

::: js/src/jsfriendapi.cpp
@@ +117,5 @@
>  
> +JS_FRIEND_API(void)
> +js::PrepareForIncrementalGC(JSRuntime *rt)
> +{
> +    if (rt->gcIncrementalState == gc::NO_INCREMENTAL)

I don't know what your philosophy on assertions here is, but it seems to me that this should only be called when we can do an incremental GC, so maybe there should be an assert before the dynamic check, so we can see if somebody is using the API incorrectly?

::: js/src/jsfriendapi.h
@@ +611,5 @@
>  extern JS_FRIEND_API(void)
>  PrepareForFullGC(JSRuntime *rt);
>  
> +extern JS_FRIEND_API(void)
> +PrepareForIncrementalGC(JSRuntime *rt);

Judging from the other patches in the bug, you could expose a smaller API here by just having a function that calls PrepForIGC followed by FinishIGC, but maybe it may be useful elsewhere?
Comment 10 Bill McCloskey (:billm) 2012-05-29 12:41:18 PDT
Created attachment 628063 [details] [diff] [review]
stop calling through xpconnect, v2

This adds an additional incremental flag to GarbageCollectNow.
Comment 11 Bill McCloskey (:billm) 2012-05-29 12:42:31 PDT
Created attachment 628065 [details] [diff] [review]
use the APIs, v2

This fixes the problem Olli brought up.
Comment 12 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-06-02 14:10:22 PDT
I'm testing whether these patches still compile and if so, push to try.
(I'd really like to see iGC to be used more often. Right now I get non-incremental GC almost all the time.)
Comment 13 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-06-02 14:30:14 PDT
Looks like nsDOMWindowUtils needs some tiny changes.
Comment 14 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-06-02 14:47:02 PDT
https://tbpl.mozilla.org/?tree=Try&rev=38a690c59978
Comment 15 Bill McCloskey (:billm) 2012-06-10 16:16:33 PDT
Created attachment 631781 [details] [diff] [review]
create separate inter-slice timer

One more patch. This one creates a separate inter-slice GC timer. With this patch, if the JS engine does a GC on its own, it won't cancel an existing timer-triggered GC. This is useful because GCs from the JS engine are usually for a single compartment, so they often don't clean up enough.
Comment 16 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-06-10 16:26:22 PDT
Comment on attachment 631781 [details] [diff] [review]
create separate inter-slice timer


> GCTimerFired(nsITimer *aTimer, void *aClosure)
> {
>-  NS_RELEASE(sGCTimer);
>+  if (aTimer == sGCTimer)
>+    NS_RELEASE(sGCTimer);
>+  else
>+    NS_RELEASE(sInterSliceGCTimer);
if (expr) {
  stmt;
} else {
  stmt;
}
Comment 17 Bill McCloskey (:billm) 2012-06-10 21:30:24 PDT
Just landing one of these patches for now (the inter-slice timer):
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ecdf0d8b4fa
Comment 18 Graeme McCutcheon [:graememcc] 2012-06-12 03:07:49 PDT
https://hg.mozilla.org/mozilla-central/rev/3ecdf0d8b4fa
Comment 19 IU 2012-06-12 04:48:01 PDT
Why is this "RESOLVED FIXED?"  Only one of the patches has been landed.
Comment 20 Bill McCloskey (:billm) 2012-06-15 16:06:26 PDT
Sadly, the rest of these patches seem to cause leaks. I'll see if I can find some subset of them that don't cause leaks. And then when I have time I'll debug the leak.
Comment 21 Bill McCloskey (:billm) 2012-06-30 14:20:04 PDT
Landing some of this. For the rest, I need to straighten out some issues that are causing reftests to take a lot longer.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa36c61ce4c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c6e0423400e
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b62d36c68c5
Comment 23 Ed Morley [:emorley] 2012-07-02 01:29:19 PDT
> Why is this "RESOLVED FIXED?"  Only one of the patches has been landed.

Because [leave-open] was used instead of [leave open]. Issue has been filed (https://bitbucket.org/graememcc/m-cmerge/issue/9/ryanvm-watch-for-leave-open-variant), but until resolved/deployed, please use [leave open].

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