Last Comment Bug 818739 - Don't run CC during shutdown
: Don't run CC during shutdown
Status: RESOLVED FIXED
[Snappy:P1]
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla20
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
: 818737 (view as bug list)
Depends on: 834945 895381 822029 822825 888900
Blocks: shutdown-faster
  Show dependency treegraph
 
Reported: 2012-12-05 16:20 PST by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2013-07-18 05:40 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
20+


Attachments
don't run CC during its shutdown (1.08 KB, patch)
2012-12-05 16:48 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
continuation: feedback-
bgirard: feedback+
Details | Diff | Review
patch (1.21 KB, patch)
2012-12-14 10:01 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
continuation: review+
Details | Diff | Review
patch (1.25 KB, patch)
2012-12-14 10:42 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-05 16:20:55 PST
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.
Comment 1 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-05 16:21:43 PST
*** Bug 818737 has been marked as a duplicate of this bug. ***
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-05 16:37:21 PST
or we could perhaps just not run CC during shutdown (opt builds)
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-05 16:48:10 PST
Created attachment 689020 [details] [diff] [review]
don't run CC during its shutdown

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 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-06 13:39:51 PST
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.
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-06 13:52:26 PST
(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 6 Benoit Girard (:BenWa) 2012-12-06 13:54:24 PST
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?
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-12-06 13:58:45 PST
/me is scared...
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-06 14:02:03 PST
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.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-12-06 14:07:12 PST
Well, we'd stop running a bunch of destructors. Who knows what will break.
Comment 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-06 14:10:28 PST
Well, those dtors can't expect to run even now if there are leaks.
Comment 11 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-06 14:11:37 PST
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.
Comment 12 Benoit Girard (:BenWa) 2012-12-06 14:14:17 PST
> (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.
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-06 14:15:39 PST
(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!
Comment 14 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-06 14:23:01 PST
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.
Comment 15 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-12-06 14:24:11 PST
> 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.
Comment 16 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-06 14:32:08 PST
(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?
Comment 17 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-12-13 13:23:33 PST
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.
Comment 18 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-14 09:14:22 PST
Tryserver results look good.
Comment 19 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-14 10:01:59 PST
Created attachment 692346 [details] [diff] [review]
patch

https://tbpl.mozilla.org/?tree=Try&rev=b3aa58b00267
Comment 20 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-12-14 10:12:23 PST
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?
Comment 21 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-14 10:42:32 PST
Created attachment 692367 [details] [diff] [review]
patch

Removed the useless comment as agreed on IRC.
DEBUG_CC can be used without DEBUG.
Comment 22 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-12-14 12:37:09 PST
https://hg.mozilla.org/mozilla-central/rev/034a28d4d68b
Comment 23 Vladan Djeric (:vladan) 2013-01-08 12:09:13 PST
It looks like there was a 30% drop in average shutdown times from this patch! See Dec 15th: http://tinyurl.com/abo6uek
Comment 24 :Ehsan Akhgari (busy, don't ask for review please) 2013-01-08 12:17:32 PST
(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!
Comment 25 Bogdan Maris, QA [:bogdan_maris] 2013-01-11 08:04:05 PST
If there is a way to test this please let me know.
Comment 26 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2013-01-11 11:51:12 PST
Well, shutdown shouldn't crash, yet be faster than before...
so I don't think there are reasonable ways to test this.
Comment 27 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2013-01-11 11:53:42 PST
Or do what BenWa suggested in Bug 818296
Comment 28 Vladan Djeric (:vladan) 2013-01-11 13:09:51 PST
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
Comment 29 :Ehsan Akhgari (busy, don't ask for review please) 2013-01-11 13:26:32 PST
(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...
Comment 30 Benoit Girard (:BenWa) 2013-02-18 09:43:25 PST
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.

Note You need to log in before you can comment on or make changes to this bug.