1543090 - Clean up unused bits of XPIState

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Florian Quèze [:florian]]

Since I started working on bug 1540135 to catch startup main thread I/O regressions, some new startup main thread I/O got introduced:

TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_mainthreadio.js | unexpected stat on Z:\task_1554125950\build\application\firefox\omni.ja before first paint -
Stack trace:
nsLocalFile::ResolveAndStat
getModTime (resource://gre/modules/addons/XPIProvider.jsm:552:0)
syncWithDB (resource://gre/modules/addons/XPIProvider.jsm:593:0)
addAddon (resource://gre/modules/addons/XPIProvider.jsm:769:0)
addAddon (resource://gre/modules/addons/XPIProvider.jsm:1545:0)
install (resource://gre/modules/addons/XPIInstall.jsm:3750:0)
InterpretGeneratorResume (self-hosted:1281:0)
AsyncFunctionNext (self-hosted:837:0)
promise callback
XREMain::XRE_main
(PoisonIOInterposer) stat - Z:\task_1554125950\build\application\firefox\omni.ja

TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_mainthreadio.js | unexpected stat on Z:\task_1554125950\build\application\firefox\browser\omni.ja before first paint -
Stack trace:
nsLocalFile::ResolveAndStat
getModTime (resource://gre/modules/addons/XPIProvider.jsm:552:0)
syncWithDB (resource://gre/modules/addons/XPIProvider.jsm:593:0)
addAddon (resource://gre/modules/addons/XPIProvider.jsm:769:0)
addAddon (resource://gre/modules/addons/XPIProvider.jsm:1545:0)
install (resource://gre/modules/addons/XPIInstall.jsm:3750:0)
InterpretGeneratorResume (self-hosted:1281:0)
AsyncFunctionNext (self-hosted:837:0)
promise callback
XREMain::XRE_main
(PoisonIOInterposer) stat - Z:\task_1554125950\build\application\firefox\browser\omni.ja

TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_mainthreadio.js | unexpected stat on Z:\task_1554125950\build\application\firefox\browser\omni.ja before first paint -
Stack trace:
nsLocalFile::ResolveAndStat
getModTime (resource://gre/modules/addons/XPIProvider.jsm:552:0)
syncWithDB (resource://gre/modules/addons/XPIProvider.jsm:593:0)
addAddon (resource://gre/modules/addons/XPIProvider.jsm:769:0)
addAddon (resource://gre/modules/addons/XPIProvider.jsm:1545:0)
install (resource://gre/modules/addons/XPIInstall.jsm:3750:0)
InterpretGeneratorResume (self-hosted:1281:0)
AsyncFunctionNext (self-hosted:837:0)
promise callback
XREMain::XRE_main
(PoisonIOInterposer) stat - Z:\task_1554125950\build\application\firefox\browser\omni.ja

I vaguely suspect bug 1512436 of being the cause, but I haven't verified this.

Comment 1

[Kris Maglione [:kmag]]

This only happens on the first run with a new profile.

Comment 2

[Andrew Swan [:aswan]]

Even though we're not measuring it in automation, first run is still important, we don't want to be doing unnecessary main thread I/O. This is easy enough to fix, there's no point reading mod times for things built-in to omni.ja

Comment 3

[Kris Maglione [:kmag]]

There is if at some point we decide we want to re-read things when omni.ja changes. In any case, this is about the cheapest part of the installation process for these extensions, and every part of the process is much cheaper than the corresponding process for non-omni-ja extensions.

Comment 4

[Florian Quèze [:florian]]

(In reply to Kris Maglione [:kmag] from comment #3)

Bug 1357205 is indeed more expensive, but that's not a reason to take a new perf regression. omni.ja won't change without an update of Firefox.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Florian Quèze [:florian] from comment #4)

omni.ja won't change without an update of Firefox.

That's true, but this also only happens after a Firefox update.

If we want to invest effort in optimizing first run performance, we can do that, but there's no reason for this bug to exist on its own. Statting files is part of the normal add-on installation process. It's one of the cheapest parts. And to the extent that it applies to the overall add-on installation process, it may eventually be worth looking into. But as it applies specifically to the built-in themes, and until we do some serious profiling and address the major performance bottlenecks, it doesn't stand as an issue on its own.

Comment 6

[Andrew Swan [:aswan]]

Attachment: Bug 1543090 Clean up unused bits of XPIState r=zombie

PIState.getModTime() was setting a .changed property that nothing ever
looks at. It also sets lastModifiedTime which is used from about:addons
but built-in addons aren't visible there so there's no point setting it
for them.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94b9c150456b
Clean up unused bits of XPIState r=zombie

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/94b9c150456b

Comment 9

[Miruna Curtean]

Hello,
Does this fix require manual validation? If yes, please provide some steps to reproduce in order to correctly test it and also, please set the "qe-verify+" flag. Otherwise, could the "qe-verify-" flag be added? Thanks!