Investigate if nsBrowserGlue.js :: _finalUIStartup code could be done later

RESOLVED FIXED in Firefox 56

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: Neil Deakin (mostly unavailable until September), Assigned: florian)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(3 attachments, 1 obsolete attachment)

On my machine it takes about 1110ms to show the window in an optimized Firefox.

About 80ms of that is in _finalUIStartup within nsBrowserGlue.js

The more time consuming bits of this function are:

this._sanitizer.onStartup();  - 9ms
PageThumbs.init(); - 13ms
DirectoryLinksProvider.init(); - 8ms
NewTabMessages.init(); - 13ms
SessionStore.init(); - 10ms
ContentClick.init(); - 6ms
ContentPrefServiceParent.init(); - 11ms
Cu.import("resource:///modules/AutoMigrate.jsm"); - 7ms

Investigate whether these or parts of them can be moved to happen after the window opens.

Updated

4 months ago
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-performance]
Blocks: 1355956
No longer blocks: 1336227

Updated

4 months ago
See Also: → bug 1358921

Updated

3 months ago
Flags: qe-verify? → qe-verify-
(In reply to Neil Deakin (not available until Aug 9) from comment #0)

> this._sanitizer.onStartup();  - 9ms
> PageThumbs.init(); - 13ms
> DirectoryLinksProvider.init(); - 8ms

These 3 things were moved to after first paint in bug 1371710.

> NewTabMessages.init(); - 13ms

Doesn't exist anymore.

> SessionStore.init(); - 10ms

Still there.

> ContentClick.init(); - 6ms
> ContentPrefServiceParent.init(); - 11ms

Made lazy in bug 1358921.

> Cu.import("resource:///modules/AutoMigrate.jsm"); - 7ms

Isn't referenced from nsBrowserGlue.js anymore, and is a lazy getter wherever it is used.
Assignee: nobody → florian

Updated

a month ago
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1
Created attachment 8885862 [details] [diff] [review]
rename nsBrowserGlue.js' _finalIUStartup method to _beforeUIStartup

I don't think we can change the notification name without breaking add-on compat, but we can at least make the method name less confusing, so that it's less tempting to add more stuff here.
Attachment #8885862 - Flags: review?(mconley)
Created attachment 8885864 [details] [diff] [review]
delay initialization of BrowserUsageTelemetry and BrowserUITelemetry

In my try push I had blacklisted UITelemetry.jsm too, but it is intermittently loaded before first paint (3 times out of 16 runs on the Linux 32 debug jobs, both with and without e10s). Something else must be initializing that module. I haven't investigated as it wasn't my direct target here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=614eef832187cb12647eba34b51397fea1381945 is the try results I got.
Attachment #8885864 - Flags: review?(mconley)
Created attachment 8885893 [details] [diff] [review]
initialize ContentCrashHandlers.jsm after first paint
Attachment #8885893 - Flags: review?(mconley)
Looks greenish on try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=678ded874d49c359428e50e1090912e52fe933e5) once the failures that caused the backout at bug 1378727 comment 5 are ignored.
Comment on attachment 8885862 [details] [diff] [review]
rename nsBrowserGlue.js' _finalIUStartup method to _beforeUIStartup

Review of attachment 8885862 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8885862 - Flags: review?(mconley) → review+
Comment on attachment 8885864 [details] [diff] [review]
delay initialization of BrowserUsageTelemetry and BrowserUITelemetry

Review of attachment 8885864 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ Great!
Attachment #8885864 - Flags: review?(mconley) → review+
Comment on attachment 8885893 [details] [diff] [review]
initialize ContentCrashHandlers.jsm after first paint

Review of attachment 8885893 [details] [diff] [review]:
-----------------------------------------------------------------

Code looks good, but I have an alternative solution to the waiting you're doing in browser_UnsubmittedCrashHandler. Let me know what you think.

::: browser/modules/test/browser/browser_UnsubmittedCrashHandler.js
@@ +168,5 @@
>    }
>    return Promise.all(promises);
>  }
>  
> +function waitForUnsubmittedCrashHandlerInit() {

This seems kinda hacky, tbh. I wonder if it'd be better to have nsBrowserGlue.js call checkForUnsubmittedCrashReports on an idle callback instead of having UnsubmittedCrashHandler.init() do it.

Alternatively, I wonder if it'd be generally useful to fire an observer notification during the first idle callback after first paint, and have UnsubmittedCrashHandler wait for that instead?

That way, uninit'ing can remove the observer for that notification, kind of like how we currently do it.

If you feel strongly about leaving things as they are, let me know, and we can discuss. :)
Attachment #8885893 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) - Digging out of needinfo / review backlog. from comment #8)

> ::: browser/modules/test/browser/browser_UnsubmittedCrashHandler.js

> This seems kinda hacky, tbh.

Yeah, it's a gross hack I wasn't proud of... I actually hesitated on submitting the patch or sleeping on it to see if I would find something more satisfying... but decided it was just a test and wasn't worth spending more time there.
It initially started with a simpler hack, but for a reason some of the test (almost at the end of it) fails in a way I couldn't understand last night if I don't replace UnsubmittedCrashHandler.checkForUnsubmittedCrashReports.

> I wonder if it'd be better to have
> nsBrowserGlue.js call checkForUnsubmittedCrashReports on an idle callback
> instead of having UnsubmittedCrashHandler.init() do it.

Great idea, and that lets us delay this stuff even further at the end of startup :-). Thanks!

> Alternatively, I wonder if it'd be generally useful to fire an observer
> notification during the first idle callback after first paint, and have
> UnsubmittedCrashHandler wait for that instead?

I'm afraid that notification may soon be abused by other stuff trying to detect the end of startup, so I would rather not do that.
Created attachment 8886317 [details] [diff] [review]
initialize ContentCrashHandlers.jsm after first paint, v2

Addressed comment 8. I like this much better :-).
Attachment #8886317 - Flags: review?(mconley)
Attachment #8885893 - Attachment is obsolete: true
Comment on attachment 8886317 [details] [diff] [review]
initialize ContentCrashHandlers.jsm after first paint, v2

Review of attachment 8886317 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Thanks!
Attachment #8886317 - Flags: review?(mconley) → review+

Comment 12

a month ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50109c60da4d
rename nsBrowserGlue.js' _finalIUStartup method to _beforeUIStartup, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/42dd3838b575
delay initialization of BrowserUsageTelemetry and BrowserUITelemetry until after we are done restoring the previous session, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/67205d8212f1
initialize ContentCrashHandlers.jsm after first paint, r=mconley.
https://hg.mozilla.org/mozilla-central/rev/50109c60da4d
https://hg.mozilla.org/mozilla-central/rev/42dd3838b575
https://hg.mozilla.org/mozilla-central/rev/67205d8212f1
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.