Closed Bug 1102154 Opened 10 years ago Closed 7 years ago

Remember calculated margins and font size of gaia-header in local storage (>200ms)

Categories

(Firefox OS Graveyard :: Gaia::Components, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(tracking-b2g:backlog)

RESOLVED WONTFIX
tracking-b2g backlog

People

(Reporter: ting, Unassigned)

References

Details

(Keywords: perf)

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1089920 +++ Thinker'd like to have this bug created since based on bug 1089920 comment 24, it reduces >200ms from gaia-header's runFontFit(). His idea is remembering the calculated results can avoid layout engine for the gaia-header. Some notes: 1. Julien is working on another patch in bug 1089920 which adds "start" and "end" attributes to gaia-header to escape from layout engine as well, and make gaia-header.js async in bug 1101532. 2. Julien has mentioned that the problem of local storage is storing takes time.
Summary: Remember calculated margins and font size of gaia-header in local storage → Remember calculated margins and font size of gaia-header in local storage (~200ms)
Summary: Remember calculated margins and font size of gaia-header in local storage (~200ms) → Remember calculated margins and font size of gaia-header in local storage (>200ms)
I believe this bug would affect contacts, calender and other applications too, right?
Simply grep '<gaia-header ' under gaia, there're wappush, system, verticalhome, calendar, fl, gallery, bluetooth, music, ftu, costcontrol, collection, dialer, contacts, clock, settings, wallpaper, video, sms, ringtones.
Component: Gaia::SMS → Gaia
Blocks: AppStartup
No longer blocks: 1074783
How do you think about this solution? It reduces more than 200ms from experiment profile, see bug 1089920 comment 24 for more details. Thanks.
Flags: needinfo?(wilsonpage)
Blocks: 1074783
Steve: Could you take this bug?
Flags: needinfo?(schung)
Is this change apply only on messages? or, also affects other Apps?
(In reply to Ting-Yu Chou [:ting] from comment #3) > How do you think about this solution? It reduces more than 200ms from > experiment profile, see bug 1089920 comment 24 for more details. Thanks. I'm concerned this approach will expose us to a lot of edge cases: 1. What if the user changes language? 2. What if the app can be used in landscape and portrait? 3. What if the number of buttons in the header vary depending on app state? All of these variables will impact the margin value, meaning we can't always depend on cached values that were once appropriate.
Flags: needinfo?(wilsonpage)
Exactly what I said in bug 1089920 comment 9, with the additional caveat that storing the value is synchronous.
Julien, if you are talking only *storing*, I think it is acceptable for it is almost one time cost. Ting has a experiment report in bug 1089920 comment 24, the test with *getting* and *storing* is really fast.
(In reply to Wilson Page [:wilsonpage] from comment #6) > (In reply to Ting-Yu Chou [:ting] from comment #3) > > How do you think about this solution? It reduces more than 200ms from > > experiment profile, see bug 1089920 comment 24 for more details. Thanks. > > I'm concerned this approach will expose us to a lot of edge cases: > > 1. What if the user changes language? > 2. What if the app can be used in landscape and portrait? > 3. What if the number of buttons in the header vary depending on app state? > > All of these variables will impact the margin value, meaning we can't always > depend on cached values that were once appropriate. I thought the same actually (bug 1089920 comment 13), but I found myself can hardly ignore the time it reduces. From my experiment patch, I use the header's text content as the key of cached values. So (1) actually won't be a problem since the text content will be different when the language is different. With similar idea, is it possible that we include orientation and number of buttons (or anything else could affect the values) to make the key?
(In reply to Ting-Yu Chou [:ting] from comment #9) > (In reply to Wilson Page [:wilsonpage] from comment #6) > > (In reply to Ting-Yu Chou [:ting] from comment #3) > > > How do you think about this solution? It reduces more than 200ms from > > > experiment profile, see bug 1089920 comment 24 for more details. Thanks. > > > > I'm concerned this approach will expose us to a lot of edge cases: > > > > 1. What if the user changes language? > > 2. What if the app can be used in landscape and portrait? > > 3. What if the number of buttons in the header vary depending on app state? > > > > All of these variables will impact the margin value, meaning we can't always > > depend on cached values that were once appropriate. > > I thought the same actually (bug 1089920 comment 13), but I found myself can > hardly ignore the time it reduces. > > From my experiment patch, I use the header's text content as the key of > cached values. So (1) actually won't be a problem since the text content > will be different when the language is different. With similar idea, is it > possible that we include orientation and number of buttons (or anything else > could affect the values) to make the key? Maybe we can, but what if the gaia-header styling changed? Do we need version control for this localStorage? For some reason I think we can narrow down our scope to Julien's WIP in Bug 1089920 and profiling the result first. We could revisit the styling caching idea if the patch doesn't improve as we expect.
Flags: needinfo?(schung)
I was told that the user agent string of gecko would be changed for b2g for every revision. Since this is a pre-installed app, it maybe enough to use user agent string to trigger re-computing. Fonts, styles, ..., should be changed only for new production firmwares, right?!
(In reply to Steve Chung [:steveck] from comment #10) > down our scope to Julien's WIP in Bug 1089920 and profiling the result Is there anyone who could give a number of how much improvement the bug 1089920 would be? The solution of this bug is very promising on the number, so please give me a number if you think bug 1089920 is also promising too. To handle font, style, .. change is quite easy, you just need to compute a checksum of the string of user_agent_str + lang + gaia_version. And store the checksum in the localstore. All we need to do is to test it and fix bugs.
By the way, I don't mean we just pick one patch to land. We could do both solution, since there is no conflict, and putting them together maybe get a even better result.
We should consider making it a feature blocker if this improves launch time.
feature-b2g: --- → 2.2?
Waiting for profiling result of bug 1089920 for the decision of if we should work on this bug.
Check bug 1089920 comment 34 for the profile results, Julien's WIP (attachment 8538765 [details]) improves most overall (the time takes by gaiga-header.js), even more than the WIP here.
(In reply to Ting-Yu Chou [:ting] from comment #16) > gaiga-header.js), even more than the WIP here. I don't say so, since it is only better for first launch. (Although they are very close). For reality, first launch is not a common case.
I'm still very afraid that the caching approach can bring so many subtle bugs. And I'm sure that the people responsible for fixing those bugs would be Wilson and Guillaume...
Attached file wip v1 (deleted) —
The WIP eliminates the overhead from runFontFit(), but setting the shadow DOM's innerHTML [1] is costly as well. [1] http://lxr.mozilla.org/gaia/source/shared/elements/gaia-header/dist/gaia-header.js#280
Attachment #8541627 - Attachment mime type: application/mbox → text/plain
feature-b2g: 2.2? → 2.2+
Ting, It seems that you are working on this bug, so I assign this bug to you. If this is the wrong assignment, please reassign it.
Assignee: nobody → tchou
Status: NEW → ASSIGNED
Please remove 2.2+ from this bug since bug 1089920 seems very promise.
feature-b2g: 2.2+ → ---
Hardware: x86_64 → ARM
tracking-b2g: --- → +
[Tracking Requested - why for this release]:
Component: Gaia → Gaia::Components
Priority: -- → P2
Unassigned myself as I am not working on this.
Assignee: janus926 → nobody
Status: ASSIGNED → NEW
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: