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

Status: RESOLVED FIXED

(Firefox Graveyard :: Reading List, defect, P3)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 1

[Drew Willcoxon :adw]

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.

Comment 2

[Avi Halachmi (:avih)]

(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

[Drew Willcoxon :adw]

I don't.  And maybe I'm wrong in comment 1.

Comment 4

[Justin Dolske [:Dolske]]

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.

Comment 5

[Avi Halachmi (:avih)]

(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.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/cc15af8f883d

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cc15af8f883d

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8591093 [details] [diff] [review]
Patch

Congrat for the perf improvement.

Should be in 38 beta 5.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1f41c34ba0f9

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9e1bf10888cd