Closed Bug 1543090 Opened 5 months ago Closed 5 months ago

Clean up unused bits of XPIState

Categories

(WebExtensions :: General, defect)

defect
Not set

Tracking

(firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: florian, Assigned: aswan)

Details

(Keywords: perf, regression)

Attachments

(1 file)

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.

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

Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → INVALID

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

Status: RESOLVED → REOPENED
Resolution: INVALID → ---

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.

Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Resolution: --- → INVALID

(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.

Status: RESOLVED → REOPENED
Resolution: INVALID → ---

(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.

Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Resolution: --- → WONTFIX

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.

Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/94b9c150456b
Clean up unused bits of XPIState r=zombie
Assignee: nobody → aswan
Resolution: WONTFIX → FIXED
Summary: lastModifiedTime is called multiple times during startup for omni.ja → Clean up unused bits of XPIState
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.