Closed Bug 1355492 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 56
Iteration:
56.3 - Jul 24
Tracking Status
firefox56 --- fixed

People

(Reporter: enndeakin, Assigned: florian)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-performance])

Attachments

(3 files, 1 obsolete file)

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.
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-performance]
Blocks: photon-startup
No longer blocks: 1336227
See Also: → 1358921
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
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1
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)
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)
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.
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+
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: