Closed Bug 1089145 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julienw, Assigned: julienw)

References

Details

Attachments

(2 files)

46 bytes, text/x-github-pull-request
steveck
: review+
kgrandon
: review+
Details | Review
46 bytes, text/x-github-pull-request
Details | Review
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.
Attached file github PR
unit tests don't pass, and it's not tested on device yet. I don't know if it brings improvements either.
Ok, it brings a very good improvement, I'm finishing the unit tests and asking review then.
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.
Attachment #8511515 - Attachment description: WIP github PR → github PR
Attachment #8511515 - Flags: review?(schung)
Attachment #8511515 - Flags: review?(kgrandon)
Assignee: nobody → felash
Comment on attachment 8511515 [details] [review]
github PR

The change seems fine to me. Thanks!
Attachment #8511515 - Flags: review?(kgrandon) → review+
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!
Attachment #8511515 - Flags: review?(schung) → review+
(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?
Flags: needinfo?(schung)
(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 :)
Flags: needinfo?(schung)
master: 0fe329342ee4559c58cd271b0c19b49be88943b8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8511515 - Flags: approval-gaia-v2.1?(bbajaj)
Attached file v2.1 specific PR
I think we win ~30ms here (rough guess)
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.
Attachment #8511515 - Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1-
Flags: needinfo?(gmealer)
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.
Flags: needinfo?(bbajaj)
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
Flags: needinfo?(bbajaj)
Well, if we get requests from partners, we know where to point them.
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.
Flags: needinfo?(gmealer)
You need to log in before you can comment on or make changes to this bug.