Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Description • 10 years ago

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

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 1 • 10 years ago

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.

	Staś Małolepszy :stas
	Comment 2 • 10 years ago

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

	Staś Małolepszy :stas
	Comment 3 • 10 years ago

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

	Staś Małolepszy :stas
	Comment 4 • 10 years ago

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.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 5 • 10 years ago

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?

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 6 • 10 years ago

I tested again today GAIA_INLINE_LOCALES=0 on Tarako, Buri, Keon and Flame. No FOUCs.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 7 • 10 years ago

travis and tbpl green

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 8 • 10 years ago

(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 ?

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 9 • 10 years ago

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.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 10 • 10 years ago

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

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 11 • 10 years ago

(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

	Staś Małolepszy :stas
	Comment 12 • 10 years ago

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

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 13 • 10 years ago

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.

	Staś Małolepszy :stas
	Comment 14 • 10 years ago

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.

	Staś Małolepszy :stas
	Comment 15 • 10 years ago

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.

	Staś Małolepszy :stas
	Comment 16 • 10 years ago

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

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 17 • 10 years ago

Filed bug 1052861 to work on the Settings