Closed Bug 817052 Opened 11 years ago Closed 11 years ago

Lazy load l10n, the contacts and the call log tabs to speedup the Dialer app's launch

Categories

(Firefox OS Graveyard :: Gaia::Dialer, defect, P3)

All
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+)

RESOLVED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +

People

(Reporter: pla, Assigned: etienne)

References

Details

(Keywords: perf, Whiteboard: interaction [target:12/21])

Attachments

(1 file, 1 obsolete file)

What makes it feel slow/broken?
It takes too long to startup the Dialer app after a reset.  Currently ~5 seconds, or more.  David Clarke to provide more accurate, recent numbers.

Did it prevent you from doing what you wanted? Why?

How does this make you feel?

[ ]  :)  I feel happy about it
[ ]  :|  Meh
[X]  :(  I'm upset
[ ] >:O  I'm angry

Device: Original numbers from Unagi, Nov. 22 nightly.  David Clarke to get Otoro numbers.

Details: (technical factors, FPS, app startup time, ms elapsed, etc)

Bonus: can you attach a video of the problem?
See metabug 814981
QA Contact: dclarke
blocking-basecamp: --- → ?
Additional info:

Comparable -
Android ICS 4.0.4 on Otoro - 2.0 seconds cold launch for Dialer.

David Clarke to provide our current numbers for B2G on Otoro.
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
BB+, P3 - performance on frequently used Dialer app
blocking-basecamp: ? → +
Priority: -- → P3
This bug seems like a meta bug and we don't block on meta bug. Someone need to investigate the reasons why it is slow and open blocking bugs that blocks this one.

Also the exact timing method should be described so the people that are going to work on it will be able to compare.

Renominating and triagers please explain why we block on a meta bug.
blocking-basecamp: + → ?
Hey Gabriele, could you investigate to see what is slow please ?
Flags: needinfo?(gsvelto)
Here is a full profile trace of the dialer starting up, it was taken on an Otoro with only a handful of contacts (~10) using today's code for gecko & gaia:

http://people.mozilla.com/~bgirard/cleopatra/#report=451ce24150ed5812d33b4056cd26ff6290744196

Total measured startup time is almost 5.1s and includes a few major problems.

First of all the code contains multiple expensive calls to layout::Flush(), they account for over 1000ms and happen four different times so it seems the application is spending a lot of time just laying out the page (which includes the dialer, call log and contact view). Of this time over 440ms are spent processing the CSS code, this often happens when expensive selectors are being used and should be investigated.

Over 1070ms are spent in l10n.js, divided among lots of calls. This is a known problem common to many apps and AFAIK both :philikon and :margaret have been working on so I'm CC'ing both in case they know more about it.

Various calls to contentSecurityPolicy.js are accounting for over 400ms with most of the time spent in csp_shouldLoad() (330ms) so that's another area that might use some improvement.

Finally I've found that fb_data.js is consuming around 200ms starting up. It's not much but it's surprising because I've haven't configured the Facebook contacts so I'm surprised it's showing up in the profile. Most of the time seem spent when calling contacts.js::contacts_init() from it which in turn is spending all the time in onLocalized() which might point to yet another l10n issue.

As a side note bug 817054 should probably be considered a duplicate of this one as both the dialer and the contacts are part of the communication app and are launched together.
Flags: needinfo?(gsvelto)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
My bad duplicate
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
blocking-basecamp: ? → +
Assignee: nobody → gfs07
Assignee: gfs07 → gtorodelvalle
I would say :etienne is our man, at least to give us some preliminary feedback about this performance issue :-) If you are too busy with other stuff, please assign the bug back to me.

Just wanted to say that the Communications app is kind of suffering of a "snow ball" effect and would greatly benefit of some refactoring :-)

Regarding the FB issue, maybe José Manuel could shed some light on it.
Flags: needinfo?(jmcf)
Assignee: gtorodelvalle → etienne
(In reply to Gabriele Svelto [:gsvelto] from comment #5)
> Here is a full profile trace of the dialer starting up, it was taken on an
> Otoro with only a handful of contacts (~10) using today's code for gecko &
> gaia:
> 
> http://people.mozilla.com/~bgirard/cleopatra/
> #report=451ce24150ed5812d33b4056cd26ff6290744196
> 
> Total measured startup time is almost 5.1s and includes a few major problems.
> 
> First of all the code contains multiple expensive calls to layout::Flush(),
> they account for over 1000ms and happen four different times so it seems the
> application is spending a lot of time just laying out the page (which
> includes the dialer, call log and contact view). Of this time over 440ms are
> spent processing the CSS code, this often happens when expensive selectors
> are being used and should be investigated.
> 
> Over 1070ms are spent in l10n.js, divided among lots of calls. This is a
> known problem common to many apps and AFAIK both :philikon and :margaret
> have been working on so I'm CC'ing both in case they know more about it.
> 
> Various calls to contentSecurityPolicy.js are accounting for over 400ms with
> most of the time spent in csp_shouldLoad() (330ms) so that's another area
> that might use some improvement.
> 
> Finally I've found that fb_data.js is consuming around 200ms starting up.
> It's not much but it's surprising because I've haven't configured the
> Facebook contacts so I'm surprised it's showing up in the profile. 

Fb initialization is always done for simplicity reasons. Only 200 ms is not that much 

Most of
> the time seem spent when calling contacts.js::contacts_init() from it which
> in turn is spending all the time in onLocalized() which might point to yet
> another l10n issue.

The issue with L10n is that it waits for DOMContentLoaded to raise the onlocalized event and that's not always necessary

> 
> As a side note bug 817054 should probably be considered a duplicate of this
> one as both the dialer and the contacts are part of the communication app
> and are launched together.
Flags: needinfo?(jmcf)
Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.
Target Milestone: --- → B2G C3 (12dec-1jan)
This performance bug represents a non-trivial amount of work. Marking as a P1 issue to frontload risk.
Priority: P3 → P1
Whiteboard: interaction → interaction [target:12/21]
We have a plan: we actually don't need any l10n in the critical path of the dialer launch, so we definitely have some room for improvement!
Can we block on specific bugs and not on [This APP is slow] fuzzy bug? This make it hard to really understand what work is ungoing and to split the work between people and between teams when it comes to front-end versus backend.

blocking-basecamp? but I believe this is a meta bug and should be bb-.
blocking-basecamp: + → ?
Un meta-yfing the bug. I think it should get its bb+ back now :)
Priority: P1 → P3
Summary: [Gaia::Dialer] Long initial startup time for Dialer app → Lazy load l10n, the contacts and the call log tabs to speedup the Dialer app's launch
Attached patch Patch proposal (obsolete) — Splinter Review
Asking Vivien for review because of the performance focus, Kaze for the l10n part, and eagerly waiting for Francisco's feedback :)
Attachment #691897 - Flags: review?(kaze)
Attachment #691897 - Flags: review?(21)
Attachment #691897 - Flags: feedback?(francisco.jordano)
Please could you provide a pointer to the GH PR? It will be helpful for testing the proposed patch

thanks!
Comment on attachment 691897 [details] [diff] [review]
Patch proposal

Review of attachment 691897 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the l10n part, see nitpick below.

::: shared/js/l10n.js
@@ +924,5 @@
> +    startLoading();
> +  } else {
> +    window.addEventListener('DOMContentLoaded', function l10nStartup() {
> +      startLoading();
> +    });

why don’t you use the following?

  window.addEventListener('DOMContentLoaded', startLoading)

(and I liked the `l10nStartup' function name ;-))
Triage: BB+, for dialer performance
blocking-basecamp: ? → +
Comment on attachment 691897 [details] [diff] [review]
Patch proposal

It's looking great!

Nice improvement, my only concern is about the lazy initialisation of the contacts iframe.

Take into account that we will access from the call log to that iframe as well, check functions:

createNewContact and addToExistingContact

They are modifying the contacts iframe src, but they expect to have the src setup.

Apart from that, looking awesome, impressive job.
Attachment #691897 - Flags: feedback?(francisco.jordano)
Sorry, forgot to mentiong:

createNewContact and addToExistingContact

in the recents.js file

Cheers!
Attached patch Patch v2Splinter Review
New patch with Kaze's comments addressed.
Attachment #691897 - Attachment is obsolete: true
Attachment #691897 - Flags: review?(kaze)
Attachment #691897 - Flags: review?(21)
Attachment #692244 - Flags: review?(21)
Attachment #692244 - Flags: feedback?(francisco.jordano)
(In reply to Jose M. Cantera from comment #17)
> Please could you provide a pointer to the GH PR? It will be helpful for
> testing the proposed patch
> 
> thanks!

You can  |git apply the_patch| directly or use the branch on my fork here:
https://github.com/etiennesegonzac/gaia/tree/dialer-1337
(In reply to Francisco Jordano [:arcturus] from comment #21)
> Sorry, forgot to mentiong:
> 
> createNewContact and addToExistingContact
> 
> in the recents.js file
> 
> Cheers!

Did some successful testing for those path, thanks for pointing them out!
Comment on attachment 692244 [details] [diff] [review]
Patch v2

Review of attachment 692244 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits addressed.

Also a lot of whitespace changes make the patch bigger than what it is supposed to be, next time I guess it would be better to not add them in the PR.

I wonder also if lazy l10n should be included in l10n.js in the long term but this is Kaze's call here.

::: apps/communications/dialer/index.html
@@ +7,5 @@
> +    <!-- Localization -->
> +    <script defer type="application/javascript" src="/dialer/js/lazy_l10n.js"></script>
> +    <link rel="resource" type="application/l10n" href="/dialer/locales/locales.ini">
> +    <link rel="resource" type="application/l10n" href="/shared/locales/date.ini">
> +    <!-- Here to make the build script works his magick

/me does not understand the last part of the comment.

::: apps/communications/dialer/js/dialer.js
@@ +392,5 @@
>          Recents.updateLatestVisit();
>          break;
>        case '#contacts-view':
> +        var frame = document.getElementById('iframe-contacts');
> +        if (!frame.src.length) {

if (!frame.src) is not enough?

::: apps/communications/dialer/js/lazy_l10n.js
@@ +14,5 @@
> +    }
> +
> +
> +    // Uncomment the first comment node in head where we have
> +    // the l10n resources

uncomment? Sounds like you're adding things to the head instead.

@@ +15,5 @@
> +
> +
> +    // Uncomment the first comment node in head where we have
> +    // the l10n resources
> +    var derahs = 'shared'; // You've got to love webapps-zip.js

Argh! Is it because of some webapps-zip bug? I believe so. I should fix it....

r? on a patch in bug 822108!

@@ +39,5 @@
> +      self._loaded = true;
> +      callback(navigator.mozL10n.get);
> +
> +      document.documentElement.lang = navigator.mozL10n.language.code;
> +      document.documentElement.dir = navigator.mozL10n.language.direction;

You may want to move those before you call the callback, you never know if this one will use such information. Or worse, will throw an error and so your assignment will never been made!
Attachment #692244 - Flags: review?(21) → review+
https://github.com/mozilla-b2g/gaia/commit/27cf9e648f55288e9aa92ff86005ba29e82663e1
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Attachment #692244 - Flags: feedback?(francisco.jordano)
Whiteboard: interaction [target:12/21] → interaction [target:12/21], [perf_tsk]
Whiteboard: interaction [target:12/21], [perf_tsk] → interaction [target:12/21], [FFOS_perf]
Whiteboard: interaction [target:12/21], [FFOS_perf] → interaction [target:12/21]
You need to log in before you can comment on or make changes to this bug.