847337 - [SMS] [Optimization] Moving the l10n library out of the startup critical path

Status: RESOLVED FIXED

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

Comment 1

[Julien Wajsberg [:julienw]]

borja, we opened Bug 847975, do you think this is a dup from this one ?

Comment 2

[Fernando Campo (:fcampo)]

I agree with Julien, but asking borja for confirmation (and shake the bug for an update a little :p)

Comment 3

[Julien Wajsberg [:julienw]]

We're still waiting for the document's "load" event before doing anything (see startup.js).

And we're waiting for l10n's "ready" event before loading the other files. I'd like to investigate if both stuff are still needed, now that Bug 847975 is done. I think we need to wait for "ready" to do some actions (like set the header, display the messages dates, etc), but we probably can start some requests earlier (eg: call getThreads) so that we have the data when l10n is ready.

So I think there is some interesting work left here :)

Comment 4

[Gabriele Svelto [:gsvelto]]

Nominating for 1.3+ as this blocks a blocker.

Comment 5

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Do not wait for l10n to be ready before running initialization code

I did some extensive profiling on this bug both as it is now, not waiting for l10n to be ready before starting up and not waiting for the load event either. In both cases there is no improvement in startup time. The reason for this is that in both cases we're not really waiting for those events, the core of the issue that is causing unnecessary delays is the lazy-loading of files we shouldn't be lazy-loading.

To describe in more detail what's happening the current code works like this (with the patch from bug 934429 applied):

1) we load all HTML/CSS code plus a minimum JS code including L10n and the lazy loader

2) we stop, waiting for the |load| event

3) l10n.js starts to run

4) since our code is waiting for the document to be loaded the layout engine gets a chance to kick in and reflows/paints the screen -> 150ms get wasted here

5) the load event handler is triggered and our code in startup.js starts to run, the first thing it does is wait for l10n which obviously triggers immediately

6) at this stage we start lazy-loading every JS file (which is what completely defeats the purpose of lazy-loading), this sometimes causes another reflow to trigger but not always (it's timing-dependent)

7) all initialization functions are called by the lazy-loader as it loads the various files

8) getThreads() is finally called, unfortunately too late so it has to wait up to 200ms for IDB to return some data to it

If we don't wait for l10n (as I'm doing in this patch) we just skip a small amount of overhead in (5) which is why I think we should do this. This is not *significant* however, we're talking about 1-2ms at most. If we don't wait for the load event and instead start running the code straight away the only thing that happens is that we start running the startup.js code earlier but still get stuck waiting (as the engine loads the JS files we requested) so effectively 6) happens within 3) but the net result is the same as all the actual code runs *after* we've done a useless reflow.

To sum it up, I'd like to apply the changes in this patch just because they simplify the code and remove a useless callback (IMHO) and I'll do the remaining heavy lifting of fixing the lazy-loader induced delay in bug 947234.

Comment 6

[Rick Waldron [:rwaldron]]

(In reply to Gabriele Svelto [:gsvelto] from comment #5)
> Created attachment 8355269 [details] [diff] [review]
> [PATCH] Do not wait for l10n to be ready before running initialization code
> 
> I did some extensive profiling on this bug both as it is now, not waiting
> for l10n to be ready before starting up and not waiting for the load event
> either. In both cases there is no improvement in startup time. The reason
> for this is that in both cases we're not really waiting for those events,
> the core of the issue that is causing unnecessary delays is the lazy-loading
> of files we shouldn't be lazy-loading.
> 
> To describe in more detail what's happening the current code works like this
> (with the patch from bug 934429 applied):
> 
> 1) we load all HTML/CSS code plus a minimum JS code including L10n and the
> lazy loader
> 
> 2) we stop, waiting for the |load| event
> 
> 3) l10n.js starts to run
> 
> 4) since our code is waiting for the document to be loaded the layout engine
> gets a chance to kick in and reflows/paints the screen -> 150ms get wasted
> here
> 
> 5) the load event handler is triggered and our code in startup.js starts to
> run, the first thing it does is wait for l10n which obviously triggers
> immediately
> 
> 6) at this stage we start lazy-loading every JS file (which is what
> completely defeats the purpose of lazy-loading), this sometimes causes
> another reflow to trigger but not always (it's timing-dependent)
> 
> 7) all initialization functions are called by the lazy-loader as it loads
> the various files
> 
> 8) getThreads() is finally called, unfortunately too late so it has to wait
> up to 200ms for IDB to return some data to it
> 
> If we don't wait for l10n (as I'm doing in this patch) we just skip a small
> amount of overhead in (5) which is why I think we should do this. This is
> not *significant* however, we're talking about 1-2ms at most. If we don't
> wait for the load event and instead start running the code straight away the
> only thing that happens is that we start running the startup.js code earlier
> but still get stuck waiting (as the engine loads the JS files we requested)
> so effectively 6) happens within 3) but the net result is the same as all
> the actual code runs *after* we've done a useless reflow.
> 
> To sum it up, I'd like to apply the changes in this patch just because they
> simplify the code and remove a useless callback (IMHO) and I'll do the
> remaining heavy lifting of fixing the lazy-loader induced delay in bug
> 947234.

Thanks for looking into this! I agree with your rationale for simplification.

Comment 7

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8355269 [details] [diff] [review]
[PATCH] Do not wait for l10n to be ready before running initialization code

(In reply to Rick Waldron [:rwaldron] from comment #6)
> Thanks for looking into this! I agree with your rationale for simplification.

Thanks for your feedback Rick! Let's go forward with this.

Comment 8

[Julien Wajsberg [:julienw]]

Have you tried with another locale than the default one?

With a non-default locale, the l10n library has to actually load the locale file, whereas with the default locale the strings are already cached in the HTML file.

I really don't know whether it would be better or worse, but we _need_ to profile and test using non-default locale as well.

Comment 9

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] (PTO until January 6th) from comment #8)
> I really don't know whether it would be better or worse, but we _need_ to
> profile and test using non-default locale as well.

Good point, let me try that.

Comment 10

[Rick Waldron [:rwaldron]]

(In reply to Julien Wajsberg [:julienw] (PTO until January 6th) from comment #8)
> Have you tried with another locale than the default one?
> 
> With a non-default locale, the l10n library has to actually load the locale
> file, whereas with the default locale the strings are already cached in the
> HTML file.
> 
> I really don't know whether it would be better or worse, but we _need_ to
> profile and test using non-default locale as well.

Sorry, I assumed this was part of the testing—thanks for calling it out :)

Comment 11

[Joe Cheng [:jcheng] (please needinfo)]

can you quickly describe what's the user benefit of this? better performance?

Comment 12

[Gabriele Svelto [:gsvelto]]

(In reply to Joe Cheng [:jcheng] from comment #11)
> can you quickly describe what's the user benefit of this? better performance?

Yes, we're getting things out of the startup path to reduce the time the SMS app takes to start. This change will likely provide only a small benefit but is part of the other changes I'm working on in dependent bugs.

Comment 13

[Joe Cheng [:jcheng] (please needinfo)]

triage: blocks a bug that blocks CS release

Comment 14

[Gabriele Svelto [:gsvelto]]

OK, finally back on this bug. I've done some extensive profiling both with the default locale and a non-default one. In both cases the profiles look very similar, there's some differences but they're so small that they're well below the noise level (50ms over a total startup time of 1,6-1,7s for an empty database and 4+ seconds for a database with 200 messages).

With my patch applied the profiles do not change much either. By focusing on the specific function I can see that 2-3 samples are missing which would account for 10-15ms at most. Since the change is so small it's well within the noise threshold and so the reason for my change is motivated more by code hygiene than performance considerations.

Functionality-wise I cannot spot any differences between the two versions. I've tested them using the English, French and Chinese locales and they look pretty much the same both with and without the patch.

Comment 15

[Julien Wajsberg [:julienw]]

Ok, I think Vivien was focussing on "apploadtime" time (that is, the "onload" event), and in that time 50ms is something we can see. But I agree that what matter to us is the whole startup time and that "apploadtime" is mostly marketing ;)

Comment 16

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #15)
> But I agree that what matter to us is the whole startup time and that "apploadtime" is
> mostly marketing ;)

Let's put it this way, every little bit counts :)

Comment 17

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Do not wait for l10n to be ready before running initialization code

I forgot to add a pull-request in addition to the patch so here we go. Feel free to leave the review comments there instead of on the patch.

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8355269 [details] [diff] [review]
[PATCH] Do not wait for l10n to be ready before running initialization code

r=me

Comment 19

[Gabriele Svelto [:gsvelto]]

Thanks for the review! Pushed to gaia/master e55980a062e5764a3ba23f440b678ca3530f1348 and gaia/v1.3 922ec63a4255b9e9bc3edebf763a0fd1717e10c1