The patch changes also nsIDOMWindowUtils so that garbageCollect and cycleCollect methods take some more (optional) parameters. I'd really like to try the compartment GC. It makes also most of the CCs significantly faster.

(In reply to Olli Pettay [:smaug] from comment #0) > I'd really like to try the compartment GC. It makes also most of the CCs > significantly faster. How hard would it be to split out the compartmental GC stuff? Bill should be the one who takes a look at that, and if it is separate he could more easily test it and see what its effect on the GC is. Compartmental GCs are done only rarely right now, so it may have some weird effect on things. The reason that doing more compartmental GCs probably make the CCs much faster is that doing a compartmental GC clears the bits in all of the other compartments, so they will all be non-gray, which will allow your CC optimizations to ignore a lot of it. In essence, we're getting compartmental CCs: only C++ held by one compartment (or by none at all) are being traversed. That's good, but we have to be careful to schedule a CC whenever there is a global GC, and before the next compartmental GC, to catch any cycles that are reachable from multiple compartments.

(In reply to Andrew McCreight [:mccr8] from comment #1) > How hard would it be to split out the compartmental GC stuff? Not too hard. > The reason that doing more compartmental GCs probably make the CCs much > faster is that doing a compartmental GC clears the bits in all of the other > compartments, so they will all be non-gray, which will allow your CC > optimizations to ignore a lot of it. In essence, we're getting > compartmental CCs: Exactly! that is a very good thing.

I'll put the compartment gc thing to bug 716014

Thanks! Sorry I didn't finish many reviews yesterday.

(In reply to Andrew McCreight [:mccr8] from comment #1) > That's good, but we have to be careful to schedule a CC > whenever there is a global GC, and before the next compartmental GC, to > catch any cycles that are reachable from multiple compartments. DOMGCFinishedCallback already handles that.

Comment on attachment 592103 [details] [diff] [review] patch, without compartment GC Review of attachment 592103 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. This is a pretty cool little mechanism. r- because I'd like to see the "garbage held while idle" problem addressed. It sounds kind of silly, but I'm concerned about future MemShrink bug where somebody has a giant heap that mysteriously shrinks when they press the CC button. Though really, loading about:memory is probably enough to jar the CC into action. ;) ::: dom/base/nsJSEnvironment.cpp @@ +3316,5 @@ > sFirstCollectionTime = now; > } > > + NS_NAMED_MULTILINE_LITERAL_STRING(kFmt, > + NS_LL("CC(T+%.1f) collected: %lu (%lu waiting for GC), suspected: %lu, duration: %llu ms.\n") Could you maybe move duration to before collected? I think that's the thing people usually care about. Also, maybe remove the waiting for GC stat. It seems like an odd heuristic, and it can be computed by looking at the error console if somebody cares. @@ +3401,5 @@ > + } else { > + sPreviousSuspectedCount = 0; > + nsJSContext::KillCCTimer(); > + if (nsCycleCollector_suspectedCount() > 500) { > + nsJSContext::CycleCollectNow(); This isn't aggressive enough. There was a problem with the GC in Fx4 (that was fixed in Fx7) where it wasn't running when you left it overnight, which caused fragmentation and the browser running out of memory. I guess if the suspected count is growing slowly or not at all, you probably aren't going to blow up memory, but we still want to make sure any garbage gets freed in at least a somewhat timely fashion, say, 5 or 10 minutes. There are two problems I can think of that should be addressed. First, memory could be held alive by suspected nodes*, so we want to make sure that if there are any suspected nodes, we should run the CC every 5-10 minutes. Second, a GC could create a garbage cycle on the JS side, so you want a similar mechanism to ensure that a CC is run at least once in the 5-10 minutes after a GC. These mechanisms will ensure that we free things every so often, but will still not trigger if the browser is truly idle. For the first one, once we run the suspected count down to 0, if the browser isn't doing anything, it will stop running. For the second one, it will only trigger when the GC triggers, so we're not really idle. *In the Bacon CC paper, they found that in one benchmark the CC only freed a handful of cycles, but because those nodes held alive giant arrays, the benchmark would OOM without the CC. @@ +3501,5 @@ > + sCCTimerFireCount = 0; > + CallCreateInstance("@mozilla.org/timer;1", &sCCTimer); > + sCCTimer->InitWithFuncCallback(CCTimerFired, nsnull, > + NS_CC_SKIPPABLE_DELAY, > + nsITimer::TYPE_REPEATING_SLACK); What is a repeating slack timer? It keeps firing until you tell it to stop, and it isn't too picky about how precise its firing is? ::: dom/base/nsJSEnvironment.h @@ +198,5 @@ > static void KillCCTimer(); > > virtual void GC(js::gcreason::Reason aReason); > > + static bool PreviousCollectionWasGC(); This isn't a very good name, as it tracks ForgetSkippable and not just CCs. Maybe LastGCCleanedup or CleanupSinceLastGC? (for this function and the variable in the cpp file) LastGCForgotten? ;) Okay that last one would be confusing.

Note to myself, this can't land before Bug 721548 or there will be leaks.

(In reply to Olli Pettay [:smaug] from comment #7) > Note to myself, this can't land before Bug 721548 or there will be leaks. That's surprising. Why is that?

(In reply to Andrew McCreight [:mccr8] from comment #6) > ::: dom/base/nsJSEnvironment.cpp > @@ +3316,5 @@ > > sFirstCollectionTime = now; > > } > > > > + NS_NAMED_MULTILINE_LITERAL_STRING(kFmt, > > + NS_LL("CC(T+%.1f) collected: %lu (%lu waiting for GC), suspected: %lu, duration: %llu ms.\n") > > Could you maybe move duration to before collected? Well, I'm not changing that. Better to do changes in a different bug. > I think that's the thing > people usually care about. Also, maybe remove the waiting for GC stat. I actually look at gc stat all the time > @@ +3401,5 @@ > > + } else { > > + sPreviousSuspectedCount = 0; > > + nsJSContext::KillCCTimer(); > > + if (nsCycleCollector_suspectedCount() > 500) { > > + nsJSContext::CycleCollectNow(); > > This isn't aggressive enough. There was a problem with the GC in Fx4 (that > was fixed in Fx7) where it wasn't running when you left it overnight, which > caused fragmentation and the browser running out of memory. Bug #? > @@ +3501,5 @@ > > + sCCTimerFireCount = 0; > > + CallCreateInstance("@mozilla.org/timer;1", &sCCTimer); > > + sCCTimer->InitWithFuncCallback(CCTimerFired, nsnull, > > + NS_CC_SKIPPABLE_DELAY, > > + nsITimer::TYPE_REPEATING_SLACK); > > What is a repeating slack timer? It keeps firing until you tell it to stop, > and it isn't too picky about how precise its firing is? Yeah. > This isn't a very good name, as it tracks ForgetSkippable and not just CCs. > Maybe LastGCCleanedup or CleanupSinceLastGC? I'll use CleanupSinceLastGC

(In reply to Andrew McCreight [:mccr8] from comment #8) > (In reply to Olli Pettay [:smaug] from comment #7) > > Note to myself, this can't land before Bug 721548 or there will be leaks. > > That's surprising. Why is that? Because documents would have wrong CCGeneration.

(In reply to Olli Pettay [:smaug] from comment #9) > > Could you maybe move duration to before collected? > Well, I'm not changing that. Better to do changes in a different bug. okay, sure. > > I think that's the thing > > people usually care about. Also, maybe remove the waiting for GC stat. > I actually look at gc stat all the time Ok, we should leave it in then. In the future we may want to think about tuning this. In a very recent Nightly, you can now see why the GC is running, and for me, I'm seeing a lot of GCs due to the CC. Ideally, we'd track the amount of JS stuff that is freed. > > @@ +3401,5 @@ > > > + } else { > > > + sPreviousSuspectedCount = 0; > > > + nsJSContext::KillCCTimer(); > > > + if (nsCycleCollector_suspectedCount() > 500) { > > > + nsJSContext::CycleCollectNow(); > > > > This isn't aggressive enough. There was a problem with the GC in Fx4 (that > > was fixed in Fx7) where it wasn't running when you left it overnight, which > > caused fragmentation and the browser running out of memory. > Bug #? I'll poke around to find this.

Comment on attachment 592141 [details] [diff] [review] patch Review of attachment 592141 [details] [diff] [review]: ----------------------------------------------------------------- So you just always run the CC every 5 minutes? Sounds reasonable to me.