In order to make shutdown faster, and especially to make GC's during shutdown faster (BenWa had a profile where GCs triggered by CC took 76%), could we perhaps try to manually kill JS components. I mean, kill cross-compartment wrappers and not hold JS stuff alive from C++ side.

or we could perhaps just not run CC during shutdown (opt builds)

https://tbpl.mozilla.org/?tree=Try&rev=7fab6752c9de (the patch isn't about this bug, but I just wanted to see what tryserver says about it)

Comment on attachment 689020 [details] [diff] [review] don't run CC during its shutdown Andrew, what do you think about this? We do effectively leak everything, but we're about to shutdown anyway, so it shouldn't matter. I want to keep the possibility to run CC during shutdown, if that is required for some leak hunting. Env variable should work ok for that. We need to still delete CycleCollector properly so that purple buffer gets cleared etc. Tryserver doesn't look bad. BenWa, feel free to try how this affects to shutdown performance.

(We could put the ifndef to many places, I just happened to put it there. Makes it clear that we run CC 0 times.)

Comment on attachment 689020 [details] [diff] [review] don't run CC during its shutdown Well it's certainly SUPER-FAST(tm): http://people.mozilla.com/~bgirard/cleopatra/#report=5b82674c7fdbf78e2bc200864252bc1355d1d55d We should also always run this under #ifdef DEBUG. But is this safe?

/me is scared...

Ben, may I ask why? (In reply to Benoit Girard (:BenWa) from comment #6) > Comment on attachment 689020 [details] [diff] [review] > We should also always run this under #ifdef DEBUG. I don't understand this comment. We do want to run CC when we're in DEBUG build. We must be able to detect shutdown leaks.

Well, we'd stop running a bunch of destructors. Who knows what will break.

Well, those dtors can't expect to run even now if there are leaks.

But yes, this feels rather risky, which is why we could perhaps land it to m-c and backout soon if there are some problems.

> (In reply to Benoit Girard (:BenWa) from comment #6) > > Comment on attachment 689020 [details] [diff] [review] > > We should also always run this under #ifdef DEBUG. > I don't understand this comment. We do want to run CC when we're in DEBUG > build. > We must be able to detect shutdown leaks. Right. I was saying we should always run CC if #ifdef DEBUG is defined.

(In reply to comment #10) > Well, those dtors can't expect to run even now if there are leaks. That is not a good reason to not run them ever!

Also, note, CC shutdown runs very late, way after xpcom-shutdown. And Threadmanager for example has been already shutdown, and componentmanager->FreeServices has been called. (In reply to Ehsan Akhgari [:ehsan] from comment #13) > (In reply to comment #10) > > Well, those dtors can't expect to run even now if there are leaks. > > That is not a good reason to not run them ever! The good reason to not run them ever (after CC shutdown) is that running them just takes time and if they are trying to do something sane during CC shutdown, it is very unlikely to succeed.

> We should also always run this under #ifdef DEBUG. But is this safe? The basic problem here is that we need to run it in DEBUG builds so we can detect leaks. But of course, then if skipping it causes funny business we can only detect in DEBUG builds, then we're in trouble... You could put the Collect call under the "if" as we're not going to do anything if we're not under the branch. Some ideas for less radical ways to reduce time spent in GC and CC during shutdown: - Restructure the shutdown GC->CC->checkIfWeDidAnything loop to be more like CC->check->GC, because we can probably assume we just did a GC recently. That would reduce the number of shutdown GCs we do by 1. - To reduce the amount of time spent in CCs during shutdown (I don't recall if this is a lot or not) is to make the first CC, which frees up a lot of stuff, into a compartment-merging CC. That makes CCs with a lot of JS being "freed" much faster (5x faster for TechCrunch, for instance). - Only do a followup GC after a CC if the CC "freed" JS. - If we do the previous one, we could consider only doing another CC if we triggered a GC. These changes might be doable in debug builds, too, though we may have to clean up some nastiness along the lines of bug 800392 to get that to stick, and it could introduce new random shutdown leaks, etc.

(In reply to Andrew McCreight [:mccr8] from comment #15) > - Restructure the shutdown GC->CC->checkIfWeDidAnything loop to be more like > CC->check->GC, because we can probably assume we just did a GC recently. > That would reduce the number of shutdown GCs we do by 1. > > - To reduce the amount of time spent in CCs during shutdown (I don't recall > if this is a lot or not) It is not a lot, at least based on the profiles I've seen. It is GC which is a lot. Eventually we don't want to run CC at all in opt builds during shutdown, so I'm not sure it is worth to add temporary hacks to make it faster. Why not just try the simple approach first?

The last write to disk that I found on bugzilla was bug 752642. I could not trigger a write during nsCycleCollector_shutdown locally, so I did a try push to see if it can find one: https://tbpl.mozilla.org/?tree=Try&rev=c3ac14393e56 If not, I will open a bug for moving write poisoning to that line. Unfortunately I could not find/remember what originally prevented it going there.

Tryserver results look good.

Comment on attachment 692346 [details] [diff] [review] patch Review of attachment 692346 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/nsCycleCollector.cpp @@ +2954,5 @@ > > void > nsCycleCollector::Shutdown() > { > // Here we want to run a final collection and then permanently Please fix the comment to say something about how we only run the final collection in DEBUG builds or whatnot. @@ +2958,5 @@ > // Here we want to run a final collection and then permanently > // disable the collector because the program is shutting down. > > +#ifndef DEBUG > +#ifndef DEBUG_CC Is the ifndef DEBUG_CC needed? Does non-DEBUG DEBUG_CC even compile?

Removed the useless comment as agreed on IRC. DEBUG_CC can be used without DEBUG.

It looks like there was a 30% drop in average shutdown times from this patch! See Dec 15th: http://tinyurl.com/abo6uek

(In reply to comment #23) > It looks like there was a 30% drop in average shutdown times from this patch! > See Dec 15th: http://tinyurl.com/abo6uek That is exactly what I would expect. Great!

If there is a way to test this please let me know.

Well, shutdown shouldn't crash, yet be faster than before... so I don't think there are reasonable ways to test this.

Or do what BenWa suggested in Bug 818296

Couldn't we back out the patch (maybe on some subset of builds) & then just monitor the late write Telemetry reports? That would tell us if this change causes us to skip saving data

(In reply to comment #28) > Couldn't we back out the patch (maybe on some subset of builds) & then just > monitor the late write Telemetry reports? That would tell us if this change > causes us to skip saving data We don't yet have Windows write poisoning. Perhaps this makes sense when BenWa lands those patches...

This should make it to the release note for Firefox 20. It's a big improvements to shutdown for users that are getting the 'Firefox is running' dialog.