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)
Firefox
General
Tracking
()
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: enndeakin, Assigned: florian)
References
(Blocks 1 open bug)
Details
(Whiteboard: [photon-performance])
Attachments
(3 files, 1 obsolete file)
2.26 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
6.86 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
6.12 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-performance]
Updated•7 years ago
|
Updated•7 years ago
|
Flags: qe-verify? → qe-verify-
Assignee | ||
Comment 1•7 years ago
|
||
(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 | ||
Updated•7 years ago
|
Assignee: nobody → florian
Updated•7 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8885893 -
Flags: review?(mconley)
Assignee | ||
Comment 5•7 years ago
|
||
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 6•7 years ago
|
||
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 7•7 years ago
|
||
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 8•7 years ago
|
||
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-
Assignee | ||
Comment 9•7 years ago
|
||
(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.
Assignee | ||
Comment 10•7 years ago
|
||
Addressed comment 8. I like this much better :-).
Attachment #8886317 -
Flags: review?(mconley)
Assignee | ||
Updated•7 years ago
|
Attachment #8885893 -
Attachment is obsolete: true
Comment 11•7 years ago
|
||
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•7 years 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.
Comment 13•7 years ago
|
||
bugherder |
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
Closed: 7 years 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.
Description
•