Closed Bug 1118214 Opened 5 years ago Closed 3 years ago

[Messages] Investigate how we can make shared/js/date_time_helper.js more performant at executing time

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX
2.2 S4 (23jan)

People

(Reporter: julienw, Unassigned)

References

Details

(Whiteboard: [p=2])

Attachments

(2 files)

shared/js/date_time_helper.js takes too much time (~40ms on Buri), probably because of the mozSettings use.

In this bug we want to see if we can reduce this time, with a focus on the Messages application.

2 main ideas are:
* don't use mozSettings at execution time, but only when it's accessed. Not sure if this is easily possible
* try to not make date_time_helper necessary for ThreadListUI.
Whiteboard: [p=2]
Target Milestone: --- → 2.2 S4 (23jan)
NI Wesley to put the feature-b2g flag
Flags: needinfo?(whuang)
Will look into it.
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
This is for app launch time improvement, so let's put feature-b2g:2.2+.
feature-b2g: --- → 2.2+
Flags: needinfo?(whuang)
Comment on attachment 8545758 [details]
GitHub Branch (WIP with localstorage cache)

Hey Julien,

I've attached two WIPs, both start lazy loading of "date_time_helper" on the first demand.

* Idea for the first one (attachment 8545758 [details]) is the following:

** if "navigator.mozHour12" is available use it immediately to format time;

** if not, check local storage cache for the cached "mozHour12" and if we have one - use it immediately _and_ start lazy loading of data_time_helper to refresh element time once it's loaded and actual "mozHour12" has a different value;

** if neither "navigator.mozHour12" nor cached "mozHour12" is available use default value to format time and _and_ start lazy loading of data_time_helper to refresh element time once it's loaded and actual "mozHour12" has a different value;

I'm in doubt about the last step, but it looks even more ugly if we don't display anything in thread timestamp section.

* Idea for the second patch (attachment 8545759 [details]) is the same except that it doesn't have localstorage cache step.

Here are not impressing numbers I got with Flame, master engineering build, 2x30 runs of "make test-perf":

* Master [1565.8, 1567.9], avg. 1566.8
* Patch with localstorage cache [1552.3, 1546.8], avg. 1549.5
* Patch without localstorage cache [1548.4, 1560.26], avg. 1554.3

So as you see if we start lazy loading when first page is still rendering we won't get any benefit, maybe if we start lazy loading once first page is loaded the numbers can be better. For example I tried it with localstorage patch:

* Patch with localstorage cache and delayed lazy load [1499.4, 1494.4], avg. 1496.9

What do you think about it or maybe you have better ideas on how to improve that?

Thanks!
Attachment #8545758 - Flags: feedback?(felash)
Attachment #8545759 - Flags: feedback?(felash)
Some median values instead of averages for 2x30 runs (see http://jsbin.com/zebepafedu/1/edit?js,output):

* Master [1522, 1509]
* Patch with localstorage cache [1483, 1487.5]
* Patch without localstorage cache [1494.5, 1500]
Merging both runs:

Master: 1512.5
With localStorage: 1485
Without localStorage: 1496.5
See Also: → 1122649
[changing flag for the performance improvement task]
blocking-b2g: --- → 2.2+
feature-b2g: 2.2+ → ---
Since bug 1124102 is resolved, there are some updated median values for 2x50 runs, light workload (see http://jsbin.com/wexivi/1/edit?js,output):

* Master [1490.5, 1484.5] (median from joined data, 100 data points - 1486.5)
* Patch with localstorage cache [1501.5, 1505.5] (median from joined data, 100 data points - 1504)
* Patch without localstorage cache [1510, 1505.5] (median from joined data, 100 data points - 1506)

It's kind of funny, but looks like my patches just make things worse on the current master.
Oleg, do you think it makes sense to pursue this lead?
(In reply to Julien Wajsberg [:julienw] from comment #11)
> Oleg, do you think it makes sense to pursue this lead?

Yeah, taking into account the latest measurements from comment 10, complexity of "solution" and possible late UI updates I'd say it doesn't make sense. Even if we had 10-15ms win, I'd day it isn't worth it.

So I propose to close this issue as WONTFIX unless you have better solution in mind that I can try :)
The other solution is to load date_time_helper later and change the UI if the value we get is different than the value we assumed. But I'm not sure of the trade-off ...
(In reply to Julien Wajsberg [:julienw] from comment #13)
> The other solution is to load date_time_helper later and change the UI if
> the value we get is different than the value we assumed. But I'm not sure of
> the trade-off ...

Do you mean load helper only after first page is loaded? With current patches we load it "later" as well, but this "later" happens when first rendered thread tries to format time :)
Julien,

Please reply to Oleg's comment #14. We're tracking this issue against 2.2 FL and haven't seen an update for the past 3 weeks.

Thanks,
Mike

(In reply to Oleg Zasypkin [:azasypkin] from comment #14)
> ...
> 
> Do you mean load helper only after first page is loaded? With current
> patches we load it "later" as well, but this "later" happens when first
> rendered thread tries to format time :)
Flags: needinfo?(felash)
This issue is less important now that bug 1124102 has been fixed.

That said I just saw that bug 1124102 was not 2.2+, I just asked a blocker request to uplift it.

mozSettings still has some performance penalty, so we could still improve from this, but the impact would be lower (~10-15ms; compared to ~30ms when I filed the bug).

Removing the 2.2 blocker status then.
blocking-b2g: 2.2+ → ---
Flags: needinfo?(felash)
Comment on attachment 8545758 [details]
GitHub Branch (WIP with localstorage cache)

Removing feedback requests since the change doesn't look reasonable for the value and complexity it brings.

Still we may revisit it once again if such necessity arise in the future.
Attachment #8545758 - Flags: feedback?(felash)
Attachment #8545759 - Flags: feedback?(felash)
Assignee: azasypkin → nobody
Status: ASSIGNED → NEW
Mass closing of Gaia::SMS bugs. End of an era :(
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Mass closing of Gaia::SMS bugs. End of an era :(
You need to log in before you can comment on or make changes to this bug.