816656 - [Shutdown] Startup cache slows shutdown with < 10 sec uptime

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benoit Girard (:BenWa)]

The startup cache will flush entries on the main thread if it hasn't saved. This occurs when restarting the browser within 10 seconds of up time.

Taras suggest that we simply don't update the startup cache in this case and shutdown fast.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

Without the patch
http://people.mozilla.com/~bgirard/cleopatra/?report=9521230f49a1829bf53bce8a30f7f6a52f7c7c55

With the patch:
http://people.mozilla.com/~bgirard/cleopatra/?report=e757b21be50284d5360b1c65dff67e73a0532bbc

This saves one third of shutdowns within 10 seconds.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 686742 [details] [diff] [review]
patch

hg log says Taras is a good reviewer for this file.

Comment 3

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc20093cfc9d

Comment 4

[Ed Morley [:emorley]]

Backed out for build failures during prepare-package:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=bc20093cfc9d

eg
https://tbpl.mozilla.org/php/getParsedLog.php?id=17561466&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ce0bb43518

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch 2

The package step needs a startup cache. We could do something fancy like have a way to signal the startup cache to save when we run firefox for the package step. This approach is simpler. We'll just avoid saving the file if it exists which should be always for users.

Comment 6

[(no longer active)]

Perhaps mwu is a better reviewer here?

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 688885 [details] [diff] [review]
patch 2

To mwu as Ehsan suggests

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 688885 [details] [diff] [review]
patch 2

Review of attachment 688885 [details] [diff] [review]:
-----------------------------------------------------------------

::: startupcache/StartupCache.cpp
@@ +142,5 @@
> +
> +  // If we shutdown quickly timer wont have fired. Instead of writing
> +  // it on the main thread and block the shutdown we simply wont update
> +  // the startup cache. Always do this if the file doesn't exist since
> +  // we use it part of the page step.

s/page/package

Comment 9

[Benoit Girard (:BenWa)]

review ping

Comment 10

[Benoit Girard (:BenWa)]

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

We need to get it landed because it significantly skews our windows shutdown statistics since this take 50% of shutdown time.

Comment 11

[Michael Wu [:mwu]]

Comment on attachment 688885 [details] [diff] [review]
patch 2

Review of attachment 688885 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about the review delay - it kept falling through the cracks.

r=me with the comments considered. From what I remember and some skimming of the code we should be able to just check mArchive to decide whether we should call WriteToDisk().

::: startupcache/StartupCache.cpp
@@ +143,5 @@
> +  // If we shutdown quickly timer wont have fired. Instead of writing
> +  // it on the main thread and block the shutdown we simply wont update
> +  // the startup cache. Always do this if the file doesn't exist since
> +  // we use it part of the page step.
> +  if (!mTable.IsInitialized() && mTable.Count() != 0) {

WriteToDisk() checks this for us so there's no need to duplicate this check.

@@ +145,5 @@
> +  // the startup cache. Always do this if the file doesn't exist since
> +  // we use it part of the page step.
> +  if (!mTable.IsInitialized() && mTable.Count() != 0) {
> +    bool exists;
> +    nsresult rv = mFile->Exists(&exists);

An alternative might be to check whether mArchive is set - if it isn't, it should be reasonable to assume that the file doesn't exist since we were unable to load it previously. This would let us avoid checking the file system.

Comment 12

[Benoit Girard (:BenWa)]

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

Comment 13

[Benoit Girard (:BenWa)]

You're right. Changing this to use mArchive resolve the need for the redundant conditition to prevent IO and checking if the file exists.

Comment 14

[Benoit Girard (:BenWa)]

Attachment: patch 3

Carry forward r=mwu

Comment 15

[Benoit Girard (:BenWa)]

Waiting on tree to open to land

Comment 16

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19bf13eb533b

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/19bf13eb533b