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

Status: RESOLVED WONTFIX

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

Description

[Julien Wajsberg [:julienw]]

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.

Comment 1

[Julien Wajsberg [:julienw]]

NI Wesley to put the feature-b2g flag

Comment 2

[Oleg Zasypkin [:azasypkin]]

Will look into it.

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

This is for app launch time improvement, so let's put feature-b2g:2.2+.

Comment 4

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub Branch (WIP with localstorage cache)

Comment 5

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub Branch (WIP without cache)

Comment 6

[Oleg Zasypkin [:azasypkin]]

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!

Comment 7

[Oleg Zasypkin [:azasypkin]]

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]

Comment 8

[Julien Wajsberg [:julienw]]

Merging both runs:

Master: 1512.5
With localStorage: 1485
Without localStorage: 1496.5

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[changing flag for the performance improvement task]

Comment 10

[Oleg Zasypkin [:azasypkin]]

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.

Comment 11

[Julien Wajsberg [:julienw]]

Oleg, do you think it makes sense to pursue this lead?

Comment 12

[Oleg Zasypkin [:azasypkin]]

(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 :)

Comment 13

[Julien Wajsberg [:julienw]]

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 ...

Comment 14

[Oleg Zasypkin [:azasypkin]]

(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 :)

Comment 15

[Mike Lee [:mlee]]

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 :)

Comment 16

[Julien Wajsberg [:julienw]]

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.

Comment 17

[Oleg Zasypkin [:azasypkin]]

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.

Comment 18

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 19

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(