1087329 - [messages] More perf improvements

Status: RESOLVED WONTFIX

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

Description

[Julien Wajsberg [:julienw]]

In this meta bug, I want to investigate possible improvements we can make solely in Gaia::SMS.

Comment 1

[Julien Wajsberg [:julienw]]

I've made a simple script to automatically insert time/timeEnd calls at some places. You can find it here: https://github.com/julienw/gaia-juju-tools/blob/master/perf-instrument

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: One run on Buri

Lines starting with "filename:" show how much time it takes to merely execute the script.
Lines starting with "settings.js: init:" show how much time the "init" method takes.

Comment 3

[Julien Wajsberg [:julienw]]

So, what do we especially see?

These files take too much time to merely exccute:
* notify.js takes more than 50ms 
* possibly utils.js (more than 1ms)
* possibly app.js (more than 1ms)

These files have a long init that's executed at startup:
* settings.js: 26ms
* time_headers.js: 37.28ms
* thread_list_ui.js: nearly 35ms -- from this, instanciating StickyHeader might be a culprit
* possibly activity_handler.js: nearly 9ms


Note that the script did not instrument shared libraries, I think we should measure this too.

Comment 4

[Julien Wajsberg [:julienw]]

Attachment: another run on buri

Comment 5

[Julien Wajsberg [:julienw]]

Attachment: github PR with measurements inside

Here is the code I pushed to measure the times

Comment 6

[Julien Wajsberg [:julienw]]

I fixed Compose and ThreadUI because the primitive script couldn't understand properly where to put timeEnd.
I also instrumented shared.

What do we see then?

These additional files take time to execute:
* gaia-header takes 353ms
* l10n takes 10ms
* date_time_helper takes 40ms
* fb_reader_utils takes 7ms

This file takes time to init:
* thread_ui.js takes 80ms

Comment 7

[Julien Wajsberg [:julienw]]

Note: gaia-header in Settings takes only 6ms.

Comment 8

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

We should consider making it a feature blocker if this improves launch time.

Comment 9

[Julien Wajsberg [:julienw]]

p=1 to investigate more

Comment 10

[Oleg Zasypkin [:azasypkin]]

Hey Bevis,

Currently we're trying to squeeze more performance out of SMS app and we played a bit with "maxReadAheadEntries" thing introduced in patch for bug 1057915.

As you probably remember we dispatch "moz-app-visually-complete" event once we loaded and rendered 9 threads (per comment in code this number was taken from number of thread visible on Peak), Flame can have up to 8 threads (8 for reference workload where we have only one date group/header, and 6 when every thread on first page inside its own date group). And obviously we spent much more time to retrieve last 9th thread (~200 ms for my Flame & engineering debug build) probably because of the fact that we read ahead next chunk of threads.

So it would be nice to have more insight from you on that to understand how we can tune this :) 

* What if we increase "maxReadAheadEntries" default value to 9 or 10, will we have a noticeable increase in a memory pressure (eg. for Flame 319mb)?

* What is the right way to tune this setting/preference? Kind of per device/operator config? Or SMS app can calculate appropriate value on the first launch and save it to mozSettings (if possible at all of course)? 

Thanks!

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Oleg Zasypkin [:azasypkin] from comment #10)
> Hey Bevis,
> 
> Currently we're trying to squeeze more performance out of SMS app and we
> played a bit with "maxReadAheadEntries" thing introduced in patch for bug
> 1057915.
> 
> As you probably remember we dispatch "moz-app-visually-complete" event once
> we loaded and rendered 9 threads (per comment in code this number was taken
> from number of thread visible on Peak), Flame can have up to 8 threads (8
> for reference workload where we have only one date group/header, and 6 when
> every thread on first page inside its own date group). And obviously we
> spent much more time to retrieve last 9th thread (~200 ms for my Flame &
> engineering debug build) probably because of the fact that we read ahead
> next chunk of threads.
> 
> So it would be nice to have more insight from you on that to understand how
> we can tune this :) 
> 
> * What if we increase "maxReadAheadEntries" default value to 9 or 10, will
> we have a noticeable increase in a memory pressure (eg. for Flame 319mb)?
It should be fine because the properties of a message thread is simple, so it wouldn't increase too much IMHO.
In additional, I expect that the device memory is larger if the resolution of the device is larger.

> * What is the right way to tune this setting/preference? Kind of per
> device/operator config? Or SMS app can calculate appropriate value on the
> first launch and save it to mozSettings (if possible at all of course)? 

The only thing I can imagine is to ensure the scroll rate in gaia <= the retrieval rate from DB (items/per seconds).

> Thanks!

Comment 12

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

removing feature-b2g- flag for the meta bug

Comment 13

[Julien Wajsberg [:julienw]]

1 more point to investigate the read ahead setting and async scripts.

Comment 14

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Julien, it's a test patch that including some small refinement in renderThread path:
- Remove some unnecessary class innerHTML setup
- Refine the list/container insertion logic. In the original logic, the new node will check the nodes' timestamp from the latest to the oldest. It is good when inserting a new thread node, but really bad for inserting the threads in renderThread(Because the timestamp of node is always the oldest in the list, which mean it always travse all the list for finding the correct position). I change the checking from the oldest first, and it should reduce the checking for insertion. The overall time reduced ~50ms on my flame.

Comment 15

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck](OOO 2/18 - 2/23) from comment #14)
> Created attachment 8564061 [details] [review]
> Link to github
> 
> Hi Julien, it's a test patch that including some small refinement in
> renderThread path:
> - Remove some unnecessary class innerHTML setup
> - Refine the list/container insertion logic. In the original logic, the new
> node will check the nodes' timestamp from the latest to the oldest. It is
> good when inserting a new thread node, but really bad for inserting the
> threads in renderThread(Because the timestamp of node is always the oldest
> in the list, which mean it always travse all the list for finding the
> correct position). I change the checking from the oldest first, and it
> should reduce the checking for insertion. The overall time reduced ~50ms on
> my flame.

Is it 50ms for the first panel or 50ms for the whole thread list?

Because we're inserting 9 threads only, I doubt that the insertion logic change wins 50ms... Can you try the 2 changes separately?

Also maybe we should use a data model in JavaScript instead of relying on the DOM to find the correct location to insert a thread...

Comment 16

[Julien Wajsberg [:julienw]]

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

Comment 17

[Julien Wajsberg [:julienw]]

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