818739 - Don't run CC during shutdown

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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 2

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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][[e-mail]]]

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][[e-mail]]]

(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)]

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!)]

/me is scared...

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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!)]

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

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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)]

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

[(no longer active)]

(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][[e-mail]]]

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 [:mccr8]]

> 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][[e-mail]]]

(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)]

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][[e-mail]]]

Tryserver results look good.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=b3aa58b00267

Comment 20

[Andrew McCreight [:mccr8]]

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][[e-mail]]]

Attachment: patch

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

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/034a28d4d68b

Comment 23

[Vladan Djeric (:vladan)]

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

Comment 24

[(no longer active)]

(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, Desktop QA]

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

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Or do what BenWa suggested in Bug 818296

Comment 28

[Vladan Djeric (:vladan)]

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

[(no longer active)]

(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)]

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.