Closed Bug 758034 Opened 8 years ago Closed 8 years ago

Clean up how the browser triggers GCs

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: billm, Assigned: billm)

References

Details

(Whiteboard: [js:t])

Attachments

(5 files, 2 obsolete files)

Attached patch stop calling through xpconnect (obsolete) — Splinter Review
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.
Attachment #626600 - Flags: review?(bugs)
I'll try to review this tomorrow, well today EET.
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.
Attachment #626600 - Attachment description: patch → stop calling through xpconnect
Attached patch extra JS APIsSplinter Review
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.
Attachment #626601 - Flags: review?(terrence)
Attached patch use the APIs (obsolete) — Splinter Review
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.
Attachment #626602 - Flags: review?(bugs)
Attached patch fix forced CCsSplinter Review
Olli, this is your patch. Without it, we do ~15 CC_FORCED GCs before we really CC, because of the ForgetSkippable calls.
Attachment #626604 - Flags: review?(bugs)
Attachment #626604 - Flags: review?(bugs) → review+
Comment on attachment 626600 [details] [diff] [review]
stop calling through xpconnect


> [uuid(1239b432-b835-4d28-9dc0-53063cb7f60f)]
> interface nsIXPConnect : nsISupports
Update the uuid
Attachment #626600 - Flags: review?(bugs) → review+
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?
Attachment #626602 - Flags: review?(bugs) → review-
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 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?
Attachment #626601 - Flags: review?(terrence) → review+
This adds an additional incremental flag to GarbageCollectNow.
Attachment #626600 - Attachment is obsolete: true
Attachment #628063 - Flags: review?(bugs)
Attached patch use the APIs, v2Splinter Review
This fixes the problem Olli brought up.
Attachment #626602 - Attachment is obsolete: true
Attachment #628065 - Flags: review?(bugs)
Attachment #628063 - Flags: review?(bugs) → review+
Attachment #628065 - Flags: review?(bugs) → review+
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.)
Looks like nsDOMWindowUtils needs some tiny changes.
Blocks: 761739
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.
Attachment #631781 - Flags: review?(bugs)
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;
}
Attachment #631781 - Flags: review?(bugs) → review+
Just landing one of these patches for now (the inter-slice timer):
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ecdf0d8b4fa
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/3ecdf0d8b4fa
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Why is this "RESOLVED FIXED?"  Only one of the patches has been landed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Whiteboard: [leave-open] → [leave-open][js:t]
Whiteboard: [leave-open][js:t] → [leave open][js:t]
Depends on: 770072
> 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].
https://hg.mozilla.org/mozilla-central/rev/1db0c93261d5
https://hg.mozilla.org/mozilla-central/rev/cbabd0ce3603
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 774201
You need to log in before you can comment on or make changes to this bug.