1446012 - The first startup after an update does main thread I/O to update.status several times

Status: NEW

(Toolkit :: Application Update, enhancement, P3)

Description

[Florian Quèze [:florian]]

First there's a .exists() call at https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/toolkit/mozapps/update/nsUpdateServiceStub.js#30

Then https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/toolkit/mozapps/update/nsUpdateService.js#1610 reads the same file just for the sake of setting a pref.

And then, the same code falls through to https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/toolkit/mozapps/update/nsUpdateService.js#1635 with a _postUpdateProcessing call, and that reads the file again at https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/toolkit/mozapps/update/nsUpdateService.js#1700

There are several more calls to readStatusFile that would be nice to audit, I wouldn't be surprised if a significant portion of them could be converted to async I/O.

Comment 1

[:Gijs (out for now; he/him)]

(In reply to Florian Quèze [:florian] from comment #0)
> Then
> https://searchfox.org/mozilla-central/rev/
> 8976abf9cab8eb4661665cc86bd355cd08238011/toolkit/mozapps/update/
> nsUpdateService.js#1610 reads the same file just for the sake of setting a
> pref.

At least initially, it seems the problem is that quite a few things hang off of that pref. pdfjs initialization uses it, as well as our startup decision to load an update page. Especially that last thing happens very early on (in handling of default commandline arguments). It's not clear to me off-hand how we could fix that without forgoing the IO.

Of course, we could consolidate the IO into 1 call that reads the file, but I'm skeptical that would make much of a performance difference because of disk caching, and if we would need to refactor to eliminate the sync IO completely then that refactor would likely eliminate the use of the optimization.

Doug, certainly within the perf team I suspect you're the most familiar with our updater code... do you have any ideas for what scope we have to fix this by somehow communicating the update status and/or override page in a way that doesn't require early-startup sync IO?

Comment 2

[Alex Thayer [:alexical] (she/her)]

Well, the file is always pretty small. It seems like we could pass it as an argument from the updater back to firefox.exe no[1]? Other than that - do we have an intuition for how expensive this is, given that the updater just wrote to the file? I would expect it to be not so bad since it should be in the system file cache?

[1]: https://searchfox.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#2080

Comment 3

[Florian Quèze [:florian]]

Here is another more recent startup profile where the updater code does main thread I/O: https://perfht.ml/2LS415v

The stack is:
FileUtils_closeSafeFileOutputStream
writeStringToFile
writeStatusFile
Downloader_downloadUpdate
AUS_downloadUpdate
AUS__postUpdateProcessing
AUS_observe

Comment 4

[Abdelrahman Ahmed [:abdelrahman]]

I'd like to work on this bug.

Comment 5

[Florian Quèze [:florian]]

(In reply to Doug Thayer [:dthayer] from comment #2)

It seems like we could pass it as an
argument from the updater back to firefox.exe no[1]?

An environment variable might be even easier than a command line parameter.

Do you think this bug is ready for Abdelrahman to work on it?

Comment 6

[Robert Strong (they/them - no direct email)]

Sure.

Along with the env variable the pref to indicate that an update was just successfully performed should be removed and the following code will need to check for the env variable exists and it will need to remove it when it does.
https://searchfox.org/mozilla-central/source/browser/components/BrowserContentHandler.jsm#140-179

Comment 7

[Robert Strong (they/them - no direct email)]

Sorry, the pref check is actually here and the code in getPostUpdateOverridePage shouldn't need to be changed.
https://searchfox.org/mozilla-central/source/browser/components/BrowserContentHandler.jsm#564

Comment 8

[Robert Strong (they/them - no direct email)]

I've been thinking about this a bit and I think the code can be changed as follows:

Replace the use of the app.update.postupdate pref in BrowserContentHandler.jsm with a check for if there is an activeUpdate for OVERRIDE_NEW_MSTONE and OVERRIDE_NEW_BUILD_ID. For the OVERRIDE_NEW_MSTONE case when there is an activeUpdate it can be passed to getPostUpdateOverridePage and when there isn't an activeUpdate it can just use the value of the startup.homepage_override_url pref.
https://searchfox.org/mozilla-central/source/browser/components/BrowserContentHandler.jsm#555-577

Remove the following from UpdateService.jsm
https://searchfox.org/mozilla-central/source/toolkit/mozapps/update/UpdateService.jsm#1889-1894

Replace the following so it just QI's the activeUpdate to nsIWritablePropertyBag.
https://searchfox.org/mozilla-central/source/browser/components/BrowserContentHandler.jsm#143-158

Comment 9

[Robert Strong (they/them - no direct email)]

The above would of course load the updates.xml when the user has manually installed a different build ID / version. I think that is acceptable given the code simplification.

Comment 10

[Robert Strong (they/them - no direct email)]

Florian, do you think the approach outlined in comment #8 and comment #9 would be acceptable?

Comment 11

[Florian Quèze [:florian]]

I'm afraid I don't understand the suggestion in comment 8 (and sorry for the delay, I thought I had already commented here last week).

In case this helps, here is a profile of startup after an update, showing the main thread I/O we do: https://perfht.ml/2X4N4ve

Comment 12

[Robert Strong (they/them - no direct email)]

This will make it so app update doesn't work with any files in post-update-processing which was only done so the pref would be set in time for the What New page code in BrowserContentHandler.jsm.
https://searchfox.org/mozilla-central/source/toolkit/mozapps/update/UpdateService.jsm#1885

It is a first step in lessening this without the need for the pref, env var, or command line arg. I may split that part of this out to a separate bug since there are several other improvements that can be made here.

Comment 13

[Florian Quèze [:florian]]

(In reply to Robert Strong (Robert they/them) [:rstrong] (use needinfo to contact me) from comment #12)

Thanks! Seems like a good first step indeed.

Comment 14

[Robert Strong (they/them - no direct email)]

Bug 1555088 won't work because there is another consumer of the pref which was added in bug 1389443. So, either that needs to be changed and bug 1555088 can move forward or another method will need to be devised.

Comment 15

[:Gijs (out for now; he/him)]

(In reply to Robert Strong (Robert they/them) [:rstrong] (use needinfo to contact me) from comment #14)

Bug 1555088 won't work because there is another consumer of the pref which was added in bug 1389443. So, either that needs to be changed and bug 1555088 can move forward or another method will need to be devised.

I'm fairly sure that "we just updated" can be determined using different prefs, e.g. by setting some kind of flag in the code that reads and updates the buildid/version flags in https://searchfox.org/mozilla-central/source/browser/components/BrowserContentHandler.jsm#532-539 , and make that accessible using some kind of getter.

Comment 16

[Florian Quèze [:florian]]

It would be good to have a central and easy to access way to check for "first startup on a fresh profile" and "first startup after an update" (and maybe even "first startup after a crash", but I don't have consummers in mind for that last one).

Comment 17

[Robert Strong (they/them - no direct email)]

(In reply to :Gijs (he/him) from comment #15)

(In reply to Robert Strong (Robert they/them) [:rstrong] (use needinfo to contact me) from comment #14)

Bug 1555088 won't work because there is another consumer of the pref which was added in bug 1389443. So, either that needs to be changed and bug 1555088 can move forward or another method will need to be devised.

I'm fairly sure that "we just updated" can be determined using different prefs, e.g. by setting some kind of flag in the code that reads and updates the buildid/version flags in https://searchfox.org/mozilla-central/source/browser/components/BrowserContentHandler.jsm#532-539 , and make that accessible using some kind of getter.

The PdfJs.jsm code is using that pref before that code runs. I'm not saying there aren't ways, just that the way I thought would work that I filed that bug for won't because of that.

Comment 18

[Robert Strong (they/them - no direct email)]

The Update Agent project will make it so updates are applied without running Firefox afterwards so it won't be possible to use an env var or a command line argument.

It might be acceptable to have bug 1389443 just check if the mstone version has changed instead of relying on the post update pref and then go with what I previously proposed. I suspect that it probably should already be doing that since that code won't run when a client that has manually updated by installing a newer version. If the code shouldn't run on downgrades it can just check if the version is greater.

edit the code for bug 1389443 also doesn't run for profiles that didn't perform the update and I think it isn't doing what I think the author thinks it is.

Comment 19

[Robert Strong (they/them - no direct email)]

I talked with dthayer and he agrees with the assessment in comment #18 so I filed bug 1559195 to remove this use of the app.update.postupdate pref.

Comment 20

[Robert Strong (they/them - no direct email)]

I'll go ahead and reopen / implement bug 1555088 after bug 1559195 is fixed.

Comment 21

[:Gijs (out for now; he/him)]

:rstrong, do you know what's left to do here? It seems the startup tests still show us accessing this file, but the bugs in comment #20 are all marked fixed.

Comment 22

[Robert Strong (they/them - no direct email)]

When there is an update in progress (should be rare on release) and after an update is applied (happens approximately 2 to 3 times on release depending on how many point releases there are) there is sync file IO for the update.status file during profile-after-change.

Comment 23

[:Gijs (out for now; he/him)]

(In reply to Robert Strong [:rstrong] (Robert they/them - use needinfo to contact me) from comment #22)

When there is an update in progress (should be rare on release) and after an update is applied (happens approximately 2 to 3 times on release depending on how many point releases there are) there is sync file IO for the update.status file during profile-after-change.

Does this also happen for clean profiles, then? Maybe they get treated as the second case here? Otherwise I don't see why this is being tripped in the test... ( https://searchfox.org/mozilla-central/rev/7ed8e2d3d1d7a1464ba42763a33fd2e60efcaedc/browser/base/content/test/performance/browser_startup_mainthreadio.js#348-352 )

Comment 24

[Robert Strong (they/them - no direct email)]

New profiles check if the old update directory needs to be migrated and creates the new update directory if it doesn't exist.

For all profiles, there is also a check if a file exists which informs the client whether there is an update in progress.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Florian captured another profile today where the file IO from the UpdateService was blocking the main thread in the parent process for ~1s.

https://perfht.ml/2QHxy8c

Comment 26

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) (:⚙️) (Wayyyy behind on needinfos) from comment #25)

Florian captured another profile today where the file IO from the UpdateService was blocking the main thread in the parent process for ~1s.

https://perfht.ml/2QHxy8c

The syscall that blocked the main thread there was an fsync that I don't think is necessary. And what's also interesting in this profile is that the main thread I/O from the updater blocks the first network request of a page load, ie. this main thread I/O actively prevents the user from browsing.

Comment 27

[Jim Mathies [:jimm]]

We'll get to this as soon as we can.

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:Amir, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 29

[Robin Steuber (she/her) [:bytesized]]

It seems pretty safe to say that no one is currently working on this. The priority appears to already reflect this.