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)
Tracking
(tracking-b2g:backlog)
RESOLVED
WONTFIX
tracking-b2g | backlog |
People
(Reporter: ting, Unassigned)
References
Details
(Keywords: perf)
Attachments
(1 file)
(deleted),
text/plain
|
Details |
+++ 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.
Reporter | ||
Updated•10 years ago
|
Updated•10 years ago
|
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)
Updated•10 years ago
|
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)
Comment 1•10 years ago
|
||
I believe this bug would affect contacts, calender and other applications too, right?
Reporter | ||
Comment 2•10 years ago
|
||
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.
Updated•10 years ago
|
Component: Gaia::SMS → Gaia
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 3•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
Is this change apply only on messages? or, also affects other Apps?
Comment 6•10 years ago
|
||
(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)
Comment 7•10 years ago
|
||
Exactly what I said in bug 1089920 comment 9, with the additional caveat that storing the value is synchronous.
Comment 8•10 years ago
|
||
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.
Reporter | ||
Comment 9•10 years ago
|
||
(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?
Comment 10•10 years ago
|
||
(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)
Comment 11•10 years ago
|
||
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?!
Comment 12•10 years ago
|
||
(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.
Comment 13•10 years ago
|
||
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.
Comment 14•10 years ago
|
||
We should consider making it a feature blocker if this improves launch time.
feature-b2g: --- → 2.2?
Comment 15•10 years ago
|
||
Waiting for profiling result of bug 1089920 for the decision of if we should work on this bug.
Reporter | ||
Comment 16•10 years ago
|
||
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.
Comment 17•10 years ago
|
||
(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.
Comment 18•10 years ago
|
||
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...
Reporter | ||
Comment 19•10 years ago
|
||
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
Reporter | ||
Updated•10 years ago
|
Attachment #8541627 -
Attachment mime type: application/mbox → text/plain
Updated•10 years ago
|
feature-b2g: 2.2? → 2.2+
Comment 20•10 years ago
|
||
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
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 21•10 years ago
|
||
Please remove 2.2+ from this bug since bug 1089920 seems very promise.
Updated•10 years ago
|
feature-b2g: 2.2+ → ---
Updated•10 years ago
|
Hardware: x86_64 → ARM
Updated•10 years ago
|
tracking-b2g:
--- → +
Comment 22•9 years ago
|
||
[Tracking Requested - why for this release]:
Reporter | ||
Comment 23•9 years ago
|
||
Unassigned myself as I am not working on this.
Assignee: janus926 → nobody
Status: ASSIGNED → NEW
Comment 24•7 years ago
|
||
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.
Description
•