Last Comment Bug 816656 - [Shutdown] Startup cache slows shutdown with < 10 sec uptime
: [Shutdown] Startup cache slows shutdown with < 10 sec uptime
Status: RESOLVED FIXED
[Snappy][sps]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla20
Assigned To: Benoit Girard (:BenWa)
:
:
Mentors:
Depends on:
Blocks: shutdown-faster
  Show dependency treegraph
 
Reported: 2012-11-29 11:57 PST by Benoit Girard (:BenWa)
Modified: 2012-12-22 07:07 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (859 bytes, patch)
2012-11-29 12:26 PST, Benoit Girard (:BenWa)
taras.mozilla: review+
Details | Diff | Splinter Review
patch 2 (1.33 KB, patch)
2012-12-05 11:15 PST, Benoit Girard (:BenWa)
mwu.code: review+
Details | Diff | Splinter Review
patch 3 (1.22 KB, patch)
2012-12-19 10:17 PST, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-11-29 11:57:02 PST
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 2 Benoit Girard (:BenWa) 2012-11-29 12:31:46 PST
Comment on attachment 686742 [details] [diff] [review]
patch

hg log says Taras is a good reviewer for this file.
Comment 5 Benoit Girard (:BenWa) 2012-12-05 11:15:43 PST
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.
Comment 6 :Ehsan Akhgari 2012-12-05 11:27:18 PST
Perhaps mwu is a better reviewer here?
Comment 7 Benoit Girard (:BenWa) 2012-12-05 11:42:12 PST
Comment on attachment 688885 [details] [diff] [review]
patch 2

To mwu as Ehsan suggests
Comment 8 Benoit Girard (:BenWa) 2012-12-05 11:42:52 PST
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) 2012-12-11 11:05:59 PST
review ping
Comment 10 Benoit Girard (:BenWa) 2012-12-18 11:53:17 PST
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] 2012-12-18 12:23:47 PST
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) 2012-12-18 15:51:45 PST
https://tbpl.mozilla.org/?tree=Try&rev=4ef7fed26a6b
Comment 13 Benoit Girard (:BenWa) 2012-12-18 20:49:59 PST
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) 2012-12-19 10:17:06 PST
Created attachment 693938 [details] [diff] [review]
patch 3

Carry forward r=mwu
Comment 15 Benoit Girard (:BenWa) 2012-12-19 10:19:01 PST
Waiting on tree to open to land
Comment 17 Ed Morley [:emorley] 2012-12-20 13:48:06 PST
https://hg.mozilla.org/mozilla-central/rev/19bf13eb533b

Note You need to log in before you can comment on or make changes to this bug.