758034 - Clean up how the browser triggers GCs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: 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][[e-mail]]]

I'll try to review this tomorrow, well today EET.

Comment 2

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

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 [inactive unless it's an emergency] (:billm)]

Attachment: 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 [inactive unless it's an emergency] (:billm)]

Attachment: 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 [inactive unless it's an emergency] (:billm)]

Attachment: 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][[e-mail]]]

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][[e-mail]]]

Comment on attachment 626602 [details] [diff] [review]
use the APIs

># HG changeset patch
># Parent d2cd48411a47e228ea23964eedde67e11284d6c8
># User Bill McCloskey <[e-mail]>
>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]]

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]]

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 [inactive unless it's an emergency] (:billm)]

Attachment: stop calling through xpconnect, v2

This adds an additional incremental flag to GarbageCollectNow.

Comment 11

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

Attachment: use the APIs, v2

This fixes the problem Olli brought up.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Looks like nsDOMWindowUtils needs some tiny changes.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=38a690c59978

Comment 15

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

Attachment: 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][[e-mail]]]

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 [inactive unless it's an emergency] (:billm)]

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]]

https://hg.mozilla.org/mozilla-central/rev/3ecdf0d8b4fa

Comment 19

[IU]

Why is this "RESOLVED FIXED?"  Only one of the patches has been landed.

Comment 20

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

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 [inactive unless it's an emergency] (:billm)]

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 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa36c61ce4c8
https://hg.mozilla.org/mozilla-central/rev/7c6e0423400e
https://hg.mozilla.org/mozilla-central/rev/3b62d36c68c5

Comment 23

[Ed Morley [:emorley]]

> 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].

Comment 24

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1db0c93261d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbabd0ce3603

Hopefully we can finally close this bug!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1db0c93261d5
https://hg.mozilla.org/mozilla-central/rev/cbabd0ce3603