Closed
Bug 1152327
Opened 9 years ago
Closed 9 years ago
ReadingListUI.init() should be called from delayedStartup, not onLoad
Categories
(Firefox Graveyard :: Reading List, defect, P3)
Firefox Graveyard
Reading List
Tracking
(firefox38 fixed, firefox39 fixed, firefox40 fixed)
RESOLVED
FIXED
Firefox 40
People
(Reporter: jaws, Assigned: jaws)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
1.58 KB,
patch
|
Gavin
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Currently in browser.js, ReadingListUI.init() is called from onLoad, but I don't think that is necessary that we initialize the ReadingList UI before the chrome is painted. We should be able to move this to delayedStartup and get some better startup performance. Drew, it looks like you looked in to doing this in bug 1147554. Any reason why this wasn't done then or can't be done now?
Flags: needinfo?(adw)
Comment 1•9 years ago
|
||
It turned out not to be necessary to fix the regression, so I left it as-is. I agree we should move it though, but I wouldn't expect it to make any non-negligible performance improvement right now because ReadingListUI.init doesn't do much at this point.
Flags: needinfo?(adw)
Comment 2•9 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #1) > It turned out not to be necessary to fix the regression, so I left it as-is. > I agree we should move it though, but I wouldn't expect it to make any > non-negligible performance improvement right now because ReadingListUI.init > doesn't do much at this point. Well, _something_ at the reading list affects startup times meaningfully on most platforms, and something (possibly different) within reading list also affects every page load meaningfully. See bug 1150656 comment 10 onwards. If it's not this initialization, do you have any idea what other thing it might be?
Comment 4•9 years ago
|
||
Seems like this would be wise to do, usually things in onLoad vs delayedStartup have ended up causing (slight) perf regressions. So unless there's a PITA complication from moving this, we should just do it. Hard to prioritize, calling it a P3 based on it maybe-fixing a possible perf issue, but if other work fixes out perf issues it's not super important.
Priority: -- → P3
Comment 5•9 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #4) > Hard to prioritize, calling it a P3 based on it maybe-fixing a possible perf > issue, but if other work fixes out perf issues it's not super important. See bug 1150656 comment 17. Unfortunately, it doesn't seem like anyone is trying to address these perf issues elsewhere.
Assignee | ||
Comment 6•9 years ago
|
||
Updated•9 years ago
|
Updated•9 years ago
|
Attachment #8591093 -
Flags: review?(adw) → review+
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cc15af8f883d
Points: --- → 1
Flags: qe-verify? → qe-verify-
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8591093 [details] [diff] [review] Patch Approval Request Comment [Feature/regressing bug #]: reading list [User impact if declined]: startup speed a bit slower [Describe test coverage new/current, TreeHerder]: covered by other reading list tests [Risks and why]: none expected, delaying initialization of the feature [String/UUID change made/needed]: none
Attachment #8591093 -
Flags: approval-mozilla-beta?
Attachment #8591093 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Assignee | ||
Comment 9•9 years ago
|
||
Talos email says that this helped towards the following improvement: Improvement: Fx-Team-Non-PGO - Session Restore no Auto Restore Test - Ubuntu HW 12.04 - 5.71% decrease
https://hg.mozilla.org/mozilla-central/rev/cc15af8f883d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment 11•9 years ago
|
||
Comment on attachment 8591093 [details] [diff] [review] Patch Congrat for the perf improvement. Should be in 38 beta 5.
Attachment #8591093 -
Flags: approval-mozilla-beta?
Attachment #8591093 -
Flags: approval-mozilla-beta+
Attachment #8591093 -
Flags: approval-mozilla-aurora?
Attachment #8591093 -
Flags: approval-mozilla-aurora+
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•