The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla20

Status

()

Core
XPCOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Blocks: 1 bug)

Trunk
mozilla20
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy][sps])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

4 years ago
Created attachment 686742 [details] [diff] [review]
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.
(Assignee)

Comment 2

4 years ago
Comment on attachment 686742 [details] [diff] [review]
patch

hg log says Taras is a good reviewer for this file.
Attachment #686742 - Flags: review?(taras.mozilla)

Updated

4 years ago
Attachment #686742 - Flags: review?(taras.mozilla) → review+
(Assignee)

Updated

4 years ago
Whiteboard: [Snappy][sps]
(Assignee)

Updated

4 years ago
Assignee: nobody → bgirard
(Assignee)

Comment 3

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc20093cfc9d
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
(Assignee)

Updated

4 years ago
Summary: Startup cache slows shutdown with < 10 sec uptime → [Shutdown] Startup cache slows shutdown with < 10 sec uptime
(Assignee)

Comment 5

4 years ago
Created attachment 688885 [details] [diff] [review]
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.
Attachment #686742 - Attachment is obsolete: true
Attachment #688885 - Flags: review?(vdjeric)
Perhaps mwu is a better reviewer here?
(Assignee)

Comment 7

4 years ago
Comment on attachment 688885 [details] [diff] [review]
patch 2

To mwu as Ehsan suggests
Attachment #688885 - Flags: review?(vdjeric) → review?(mwu)
(Assignee)

Comment 8

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

Updated

4 years ago
No longer blocks: 810156
(Assignee)

Updated

4 years ago
Blocks: 819063
(Assignee)

Comment 9

4 years ago
review ping
(Assignee)

Comment 10

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

4 years ago
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.
Attachment #688885 - Flags: review?(mwu) → review+
(Assignee)

Comment 12

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4ef7fed26a6b
Flags: needinfo?(bgirard)
(Assignee)

Comment 13

4 years ago
You're right. Changing this to use mArchive resolve the need for the redundant conditition to prevent IO and checking if the file exists.
Flags: needinfo?(bgirard)
(Assignee)

Comment 14

4 years ago
Created attachment 693938 [details] [diff] [review]
patch 3

Carry forward r=mwu
Attachment #688885 - Attachment is obsolete: true
Attachment #693938 - Flags: review+
(Assignee)

Comment 15

4 years ago
Waiting on tree to open to land
Flags: needinfo?(bgirard)
(Assignee)

Comment 16

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/19bf13eb533b
Flags: needinfo?(bgirard)
https://hg.mozilla.org/mozilla-central/rev/19bf13eb533b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.