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.

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.

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.

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.

Olli, this is your patch. Without it, we do ~15 CC_FORCED GCs before we really CC, because of the ForgetSkippable calls.

Comment on attachment 626600 [details] [diff] [review] stop calling through xpconnect > [uuid(1239b432-b835-4d28-9dc0-53063cb7f60f)] > interface nsIXPConnect : nsISupports Update the uuid

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

This adds an additional incremental flag to GarbageCollectNow.

This fixes the problem Olli brought up.

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.

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 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; }

Just landing one of these patches for now (the inter-slice timer): https://hg.mozilla.org/integration/mozilla-inbound/rev/3ecdf0d8b4fa

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

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.

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

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

> 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/integration/mozilla-inbound/rev/1db0c93261d5 https://hg.mozilla.org/integration/mozilla-inbound/rev/cbabd0ce3603 Hopefully we can finally close this bug!

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