1089145 - [Messages] Move the sticky header init out of the critical path

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Julien Wajsberg [:julienw]]

As foud in bug 1087329, looks like initializing the Sticky Header is in the critical path and takes too much time. Let's see if we can move it out of the path without visible issue.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: github PR

unit tests don't pass, and it's not tested on device yet. I don't know if it brings improvements either.

Comment 2

[Julien Wajsberg [:julienw]]

Ok, it brings a very good improvement, I'm finishing the unit tests and asking review then.

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 8511515 [details] [review]
github PR

hey Steve, I see a very noticeable improvement with this patch, with nearly no behavior change. What do you think?

Hey Kevin, can you look at the small change in the sticky header library? I checked it was not regressing the call log. The goal is that I don't want to trigger a sync reflow when the lib is loaded, but rather only when we need it the first time.

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8511515 [details] [review]
github PR

The change seems fine to me. Thanks!

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8511515 [details] [review]
github PR

Looks good to me, just wondering if it's even better that moving the StickyHeader in lazyloader and init header in lazy load completed? Anyway it's definitely better than original one, so thanks for discovering this!

Comment 6

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #5)
> Comment on attachment 8511515 [details] [review]
> github PR
> 
> Looks good to me, just wondering if it's even better that moving the
> StickyHeader in lazyloader and init header in lazy load completed? Anyway
> it's definitely better than original one, so thanks for discovering this!


I'm not _really_ convinced of the lazy loading advantage yet ;) I think it gives an advantage mainly because some of the js files are costly to execute, but they shouldn't.

Also I wanted the sticky header to still show up as soon as possible and I thought that lazy loading it would delay it some more.

However, I think that given where our current perf marker are located, I think the improvements won't show up in datazilla; how about moving the marker _before_ calling firstViewDone in thread_list_ui?

Comment 7

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> 
> However, I think that given where our current perf marker are located, I
> think the improvements won't show up in datazilla; how about moving the
> marker _before_ calling firstViewDone in thread_list_ui?

Although this change won't improve performance, but it's still good to have a better result on datazilla :)

Comment 8

[Julien Wajsberg [:julienw]]

master: 0fe329342ee4559c58cd271b0c19b49be88943b8

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8511515 [details] [review]
github PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: worse perf when launching the app
[Testing completed]: yes, manually and automatic
[Risk to taking this patch] (and alternatives if risky): low-medium
[String changes made]: -

Note: this will need a branch-specific patch to resolve conflicts, I'll do it shortly

Comment 10

[Julien Wajsberg [:julienw]]

Attachment: v2.1 specific PR

Comment 11

[Julien Wajsberg [:julienw]]

I think we win ~30ms here (rough guess)

Comment 12

[bhavana bajaj [:bajaj]]

Comment on attachment 8511515 [details] [review]
github PR

Comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1086529#c11 applies here.

Although I am NI geo to make sure we are seeing improvements in datazilla once these land.

Comment 13

[Julien Wajsberg [:julienw]]

Hey Bhavana,

we met the perf goals for v2.1 but we relaxed the goals for SMS. That's why I think we should still uplift this now because I fear that our partners will request this later. It will be more painful later than now. We're still _quite_ early in the v2.1 process.

Comment 14

[bhavana bajaj [:bajaj]]

Julien, sry about the late response on this one. I think its fine to let this go in 2.2 at this point, please flag me if you think this is still worth pushing in 2.1

Comment 15

[Julien Wajsberg [:julienw]]

Well, if we get requests from partners, we know where to point them.

Comment 16

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Checking back on Datazilla, it's not a drastic change but I do see a downward trend on master rooted around the mid/late-november time period. It's hard for me to quantify because there's a ton of noise but it looks like maybe as much as a 50-100ms improvement as far as instrumentation goes.

https://datazilla.mozilla.org/b2g/?branch=master&device=flame-319MB&range=60&test=startup_%3E_moz-app-visually-complete&app_list=sms&app=sms&gaia_rev=ca6e91e09ef3ab41&gecko_rev=c67f6e9baf16&plot=median

The trend is pretty obvious on there. I think it correlates with this patch.