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

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Blocks 1 bug)

Trunk
mozilla20
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy][sps])

Attachments

(1 attachment, 2 obsolete attachments)

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 on attachment 686742 [details] [diff] [review]
patch

hg log says Taras is a good reviewer for this file.
Attachment #686742 - Flags: review?(taras.mozilla)
Attachment #686742 - Flags: review?(taras.mozilla) → review+
Whiteboard: [Snappy][sps]
Assignee: nobody → bgirard
Summary: Startup cache slows shutdown with < 10 sec uptime → [Shutdown] Startup cache slows shutdown with < 10 sec uptime
Posted patch patch 2 (obsolete) — Splinter Review
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?
Comment on attachment 688885 [details] [diff] [review]
patch 2

To mwu as Ehsan suggests
Attachment #688885 - Flags: review?(vdjeric) → review?(mwu)
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
No longer blocks: start-faster
review ping
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 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+
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)
Posted patch patch 3Splinter Review
Carry forward r=mwu
Attachment #688885 - Attachment is obsolete: true
Attachment #693938 - Flags: review+
Waiting on tree to open to land
Flags: needinfo?(bgirard)
https://hg.mozilla.org/mozilla-central/rev/19bf13eb533b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.