The default bug view has changed. See this FAQ.

Don't run CC during shutdown

RESOLVED FIXED in mozilla20

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 20+)

Details

(Whiteboard: [Snappy:P1])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 818737

Updated

4 years ago
Blocks: 810156
(Assignee)

Comment 2

4 years ago
or we could perhaps just not run CC during shutdown (opt builds)
(Assignee)

Comment 3

4 years ago
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)

Updated

4 years ago
No longer blocks: 810156

Updated

4 years ago
Blocks: 819063
(Assignee)

Comment 4

4 years ago
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.
Attachment #689020 - Flags: feedback?(continuation)
Attachment #689020 - Flags: feedback?(bgirard)
(Assignee)

Comment 5

4 years ago
(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?
Attachment #689020 - Flags: feedback?(bgirard) → feedback+
/me is scared...
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Updated

4 years ago
Summary: Try to kill chrome JS more aggressively during shutdown → Try to (a) kill chrome JS more aggressively or (b) not run CC at all during shutdown
Well, we'd stop running a bunch of destructors. Who knows what will break.
(Assignee)

Comment 10

4 years ago
Well, those dtors can't expect to run even now if there are leaks.
(Assignee)

Comment 11

4 years ago
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!
(Assignee)

Comment 14

4 years ago
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.
(Assignee)

Comment 16

4 years ago
(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?
Attachment #689020 - Flags: feedback?(continuation) → feedback-
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.
(Assignee)

Comment 18

4 years ago
Tryserver results look good.
(Assignee)

Comment 19

4 years ago
Created attachment 692346 [details] [diff] [review]
patch

https://tbpl.mozilla.org/?tree=Try&rev=b3aa58b00267
Attachment #689020 - Attachment is obsolete: true
Attachment #692346 - Flags: review?(continuation)
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?
Attachment #692346 - Flags: review?(continuation) → review+
(Assignee)

Comment 21

4 years ago
Created attachment 692367 [details] [diff] [review]
patch

Removed the useless comment as agreed on IRC.
DEBUG_CC can be used without DEBUG.
Assignee: nobody → bugs
Attachment #692346 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
https://hg.mozilla.org/mozilla-central/rev/034a28d4d68b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy:P1]
Depends on: 822029

Updated

4 years ago
OS: Linux → All
Hardware: x86 → All
Summary: Try to (a) kill chrome JS more aggressively or (b) not run CC at all during shutdown → Don't run CC during shutdown
Target Milestone: --- → mozilla20
Depends on: 822825
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.
(Assignee)

Comment 26

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

Comment 27

4 years ago
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...
Depends on: 834945
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.
relnote-firefox: --- → ?
relnote-firefox: ? → 20+

Updated

4 years ago
Depends on: 888900, 895381
You need to log in before you can comment on or make changes to this bug.