bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

ReadingListUI.init() should be called from delayedStartup, not onLoad

RESOLVED FIXED in Firefox 40

Status

Firefox Graveyard
Reading List
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 40
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Details

Attachments

(1 attachment)

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

3 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

3 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 3

3 years ago
I don't.  And maybe I'm wrong in comment 1.
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

3 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.
Created attachment 8591093 [details] [diff] [review]
Patch
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8591093 - Flags: review?(adw)

Updated

3 years ago
Blocks: 1132074
Iteration: --- → 40.1 - 13 Apr
Flags: qe-verify?
Flags: firefox-backlog+
Attachment #8591093 - Flags: review?(adw) → review+
https://hg.mozilla.org/integration/fx-team/rev/cc15af8f883d
Points: --- → 1
Flags: qe-verify? → qe-verify-
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?
status-firefox38: --- → affected
status-firefox39: --- → affected
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
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.