Last Comment Bug 994370 - Disable GAIA_INLINE_LOCALES
: Disable GAIA_INLINE_LOCALES
Status: RESOLVED FIXED
:
Product: Firefox OS
Classification: Client Software
Component: Gaia::L10n (show other bugs)
: unspecified
: All All
P2 normal (vote)
: ---
Assigned To: Zibi Braniecki [:gandalf][:zibi]
:
:
Mentors:
https://github.com/zbraniecki/gaia/tr...
Depends on: 999707
Blocks: 999778 1006359
  Show dependency treegraph
 
Reported: 2014-04-09 16:00 PDT by Zibi Braniecki [:gandalf][:zibi]
Modified: 2014-08-12 15:24 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch that turns off inline locales (46 bytes, text/x-github-pull-request)
2014-06-23 20:45 PDT, Zibi Braniecki [:gandalf][:zibi]
stas: review+
21: superreview+
Details | Review | Splinter Review

User Story
Gaia uses two optimizations flags - GAIA_CONCAT_LOCALES and GAIA_INLINE_LOCALES.
 - concat scans HTML files, combines all l10n resources into a single JSON file and links it instead of multiple l10n resources files.
 - inline takes a subset of strings, builds JSON resource out of them and includes the resource for all locales inside the HTML file

The goal of the GAIA_INLINE_LOCALES is to provide a way to quickly translate the visible DOM without having to load any external resource to avoid flashes of unstyled content.

The approach does not currently work at all. Turning off GAIA_INLINE_LOCALES does not seem to introduce any FOUCs at all. 
Testing on Tarako, Flame, Keon, Buri and Peak it seems that current l10n.js is capable of downloading resources and localizing DOM using them before firstPaint without regressing performance. In fact, turning GAIA_INLINE_LOCALES off provides minor performance wins for non default locale scenarios.

At the same time, GAIA_INLINE_LOCALES costs us:
 - it blows the HTML file size by adding a JSON object per locale
 - it makes l10n startup code much more complex
 - it costs us memory because we have to store the partial localizations

We'd like to turn off this optimization for 2.1 cycle and if it does not regress proceed to remove the functionality in the next cycle.      
Description User image Zibi Braniecki [:gandalf][:zibi] 2014-04-09 16:00:40 PDT
The concept of inline localization has been introduced in bug 815852.

We're unable to see any impact of turning it off and it does not seem to prevent FOUC in some scenarios. (starting Clock app on Keon on master on non-default locale produces FOUC)

It's also non-deterministic and prone to race conditions.

We'd like to consider:
 - removing inline localization completely
 - taking other measurements to ensure firstPaint happens after content has been localized
Comment 1 User image Zibi Braniecki [:gandalf][:zibi] 2014-04-17 17:30:58 PDT
I have first prototype of how it might work.

 - removed inline locales
 - moved .json resources to top of head (so that l10n.js can read it without waiting for interactive)
 - removed "defer" from l10n.js
 - removed build-time translation

Result on Settings is no flashing and ~50ms win compared to master on default locale.

Didn't measure against non-default locale but I'd expect much bigger perf win.
Comment 2 User image Staś Małolepszy :stas 2014-04-22 16:54:20 PDT
(In reply to Zbigniew Braniecki [:gandalf] from comment #1)
> I have first prototype of how it might work.
> 
>  - removed inline locales
>  - moved .json resources to top of head (so that l10n.js can read it without
> waiting for interactive)
>  - removed "defer" from l10n.js

I like these changes (conceptually; I understand the branch is just a prototype) a lot and I confirm they work well on my Keon.  It would be good to test them on a Tarako once we get hold of them.

>  - removed build-time translation

Can we move this part to a separate bug?  Also, bug 812993 is related.
Comment 3 User image Staś Małolepszy :stas 2014-04-22 17:51:37 PDT
I can't confirm the 50ms win you mention in comment 1 for the default locale, but I can see it for the non-default locale. I also applied https://pastebin.mozilla.org/4909672 on top of your branch to simulate the outcome of bug 812993 and here are the results:

                                       default locale        non-default locale
-------------------------------------------------------------------------------
master                                 1250/1264/62          1322/1344/66
your branch                            1270/1290/70          1270/1287/51
your branch + no en-US in HTML         1234/1274/111         1265/1279/33
Comment 4 User image Staś Małolepszy :stas 2014-04-23 04:48:29 PDT
Let's limit the scope of this bug to inline localization only.  Pretranslation is bug 998094 and removing of en-US text from HTML is bug 812993.
Comment 5 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-09 17:38:13 PDT
Vivien: I'd like to turn off inline locales soon after landing bug 992473.

I'm testing the non-default locale with MutationObservers and inline turned off on Tarako, Keon, Peak, Flame and Buri and I don't see any FOUC's. 

I see memory improvement per app and I see performance improvements.

I'd like to turn it off initially, and if we will not see any regressions clean up the code. It will help us with the whole bootstrap logic.

How does it sound to you?
Comment 6 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-23 20:45:41 PDT
Created attachment 8444929 [details] [review]
patch that turns off inline locales

I tested again today GAIA_INLINE_LOCALES=0 on Tarako, Buri, Keon and Flame. No FOUCs.
Comment 7 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-24 09:34:50 PDT
travis and tbpl green
Comment 8 User image Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2014-07-11 06:04:29 PDT
(In reply to Staś Małolepszy :stas from comment #3)
> I can't confirm the 50ms win you mention in comment 1 for the default
> locale, but I can see it for the non-default locale. I also applied
> https://pastebin.mozilla.org/4909672 on top of your branch to simulate the
> outcome of bug 812993 and here are the results:
> 
>                                        default locale        non-default
> locale
> -----------------------------------------------------------------------------
> --
> master                                 1250/1264/62          1322/1344/66
> your branch                            1270/1290/70          1270/1287/51
> your branch + no en-US in HTML         1234/1274/111         1265/1279/33


Gandalf,

why do you want to remove the pre-translating optimization now ? If Stas numbers are accurated the only benefit we will see from this patch is a startup time regression for the moment, until the other patches have landed.

Also, what you mentioned in comment 2, does not indicate that the startup wins are related to removing the build time optimization, but may be related to the shuffling around the json and removing defer, correct ?
Comment 9 User image Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2014-07-11 06:05:36 PDT
Comment on attachment 8444929 [details] [review]
patch that turns off inline locales

sr- for now until I understand the rationale and the perfs numbers.

Sorry for the delay - I finally got this annoying red number in bugzilla close to a reasonable number.
Comment 10 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-11 11:02:36 PDT
(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #8)
> Gandalf,
> 
> why do you want to remove the pre-translating optimization now ?

We are not. We are turning off INLINE_LOCALES here. We keep pre-translation as it benefits the performance :)

> If Stas numbers are accurated the only benefit we will see from this patch is a
> startup time regression for the moment, until the other patches have landed.

Stas numbers do not show any startup time regression with this patch and they are coming from an older approach where I combined several changes. I apologies for the confusion.

This patch exclusively turns off GAIA_INLINE_LOCALES.

> Also, what you mentioned in comment 2, does not indicate that the startup
> wins are related to removing the build time optimization, but may be related
> to the shuffling around the json and removing defer, correct ?

Correct. That's why I narrowed down the scope of this bug. This patch only turns off INLINE_LOCALES and does nothing else. It's literally a one liner that changes 1 to 0 :)

Why do we want to turn off inline locales:

 - it adds a lot of complexity in l10n bootstrap code
 - it adds a lot of complexity in build time code

On top of that, turning off inline locales does give us perf win that scales with the number of locales because each locale grows the .html file.

Vivien, if you prefer me to file a separate bug for this change to make it less confusing, let me know :)

p.s. Yesterday Stas told me that it seems that around 10 days ago some patch accidentally regressed Settings app by effectively making inlinelocales not work for that app, and no regression has been noted.
Comment 11 User image Zibi Braniecki [:gandalf][:zibi] 2014-07-11 11:15:07 PDT
(In reply to Zibi Braniecki [:gandalf] from comment #10)
> p.s. Yesterday Stas told me that it seems that around 10 days ago some patch
> accidentally regressed Settings app by effectively making inlinelocales not
> work for that app, and no regression has been noted.

That's changeset 37d20cd
Comment 12 User image Staś Małolepszy :stas 2014-07-14 05:38:02 PDT
(In reply to Zibi Braniecki [:gandalf] from comment #10)
> 
> Correct. That's why I narrowed down the scope of this bug. This patch only
> turns off INLINE_LOCALES and does nothing else. It's literally a one liner
> that changes 1 to 0 :)
> 
> Why do we want to turn off inline locales:
> 
>  - it adds a lot of complexity in l10n bootstrap code
>  - it adds a lot of complexity in build time code

It may be more precise to phrase the above as follows:

We want to turn off inline locales because it doesn't give us noticeable perf wins and it slows startup down due to the increased size of HTML files.  This is what this patch is doing, flipping the default GAIA_INLINE_LOCALES flag to 0.  We'll need some time to verify that no unknown perf regressions result from this change.  Once we're confident there are none, we will proceed to:

 1. make sure releng build configs don't use GAIA_INLINE_LOCALES=1,
 2. remove relevant logic from shared/js/l10n.js and build/l10n.js to reduce code complexity.  After that, GAIA_INLINE_LOCALES will no longer be a recognized make flag.
Comment 13 User image Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2014-08-04 07:44:37 PDT
Comment on attachment 8444929 [details] [review]
patch that turns off inline locales

Let's turn of the json stuff for now and let's see if it regress anything.
Comment 14 User image Staś Małolepszy :stas 2014-08-04 11:20:39 PDT
Thanks, Vivien!

Master: https://github.com/mozilla-b2g/gaia/commit/3702588587395166e9ffbba8db71bac532512b83

I'll talk to releng and build automation folks and see if we need to change any other configs.
Comment 15 User image Staś Małolepszy :stas 2014-08-04 11:31:06 PDT
Edited the bug title to match what we did here.  Bug 1048411 will be tracking the removal of GAIA_INLINE_LOCALES if perf is good.
Comment 16 User image Staś Małolepszy :stas 2014-08-07 06:43:06 PDT
Sadly, I can see a regression in the Settings app caused by this combined with bug 1024893; flod confirms it too.  See:

  https://groups.google.com/forum/#!topic/mozilla.dev.gaia/gsAkgtq_PSw

and

  https://github.com/mozilla-b2g/gaia/commit/1cccdf3b99148ebf1a31697d1cd85d6cc844edac

Before bug 1024893 was fixed, the root panel in the Settings app was created dynamically, and the inline dict were actually empty.  Now it is inlined into index.html on buildtime and the inline dicts are used again.

Gandalf, as I'm going to be on PTO for the next two weeks, please take a look at this and, if need be, enable GAIA_INLINE_LOCALES again (maybe just for the Settings app?).  I'm also hoping that you'll be able to find a better solution :)
Comment 17 User image Zibi Braniecki [:gandalf][:zibi] 2014-08-12 15:24:54 PDT
Filed bug 1052861 to work on the Settings

Note You need to log in before you can comment on or make changes to this bug.