Closed Bug 853933 Opened 12 years ago Closed 11 years ago

Don't inline all locales as JSON in the HTML

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed

People

(Reporter: stas, Assigned: kaze)

References

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

Here's the scoop, taking the Settings app as an example. index.html is around 94 KB before build. On build, we do two things: (1) we pre-localize the HTML according to GAIA_DEFAULT_LOCALE setting, (2) we inline all translations in the build (according to LOCALES_FILE setting) as a JSON blob at the end of the file. For the 8 1.0.1 locales (locales/laguages_dev.json on v1.0.1), the resulting index.html weighs in at 460 KB! (Yes, that's 460 vs. 94; five times as much.) The reason for both steps was performance. I believe (2) was implemented first by John Ford and Margaret, then Kaze added (1). (2) saves us time by skipping file I/O, while (1) saves time by avoiding DOM modifications on the client-side on runtime (this is partly true for Settings app, actually, but is the general idea behind this optimization.) Unless there are other reasons, I suggest that we just do (1). Changing the locale is a rare user story and I don't think we should sacrifice the memory consumption by doing (2) just for that. Setting GAIA_INLINE_LOCALES=0 still inlines the default locale, which makes sense for the Settings app, which has much of its HTML commented out and consequently, these parts don't get localized in the (1) step. If other apps don't do that, it might be a good idea to not inline even the default locale for them. Inlining all 8 locales (coldboot, milliseconds) browser 750-800 settings 1100-1300 Inlining only the default locale (coldboot, milliseconds) browser 650-750 settings 900 Browser is a small app in terms of localization, so not much of a difference there, but it would still start consistently around 50ms faster. For the Settings app, the gains are bigger, finally getting us below 1 second. (These numbers are the best I could get; sometimes the apps would start in 1.5 or 2 seconds…)
(In reply to Staś Małolepszy :stas from comment #0) > Setting GAIA_INLINE_LOCALES=0 still inlines the default locale, which makes > sense for the Settings app, which has much of its HTML commented out and > consequently, these parts don't get localized in the (1) step. If other > apps don't do that, it might be a good idea to not inline even the default > locale for them. Actually, looks like the Browser app also does this, but calendar or email for example don't.
(In reply to Staś Małolepszy :stas from comment #0) > (These numbers are the best I could get; sometimes the apps would start in > 1.5 or 2 seconds…) Make sure you wait 5 sec between reopening app if you want to get less fluctuation in results.
This bug has significant performance impact. Nominating for shira.
blocking-b2g: --- → shira?
shira? -> tef?
blocking-b2g: shira? → tef?
Seems like a good win, but I'll defer performance issues to mvines
Flags: needinfo?(mvines)
Sounds great but what's the risk?
Flags: needinfo?(mvines) → needinfo?
Adding kaze to the needinfo request. Kaze, what's the risk of taking this approach?
Flags: needinfo? → needinfo?(kaze)
TL;DR: the startup time would be quicker for the default locale and much longer for all other locales, and I don’t think it’s acceptable. (In reply to Staś Małolepszy :stas from comment #0) > (1) we pre-localize the HTML according to GAIA_DEFAULT_LOCALE setting, > (2) we inline all translations in the build (according to LOCALES_FILE > setting) as a JSON blob at the end of the file. > […] > The reason for both steps was performance. I believe (2) was implemented > first by John Ford and Margaret, then Kaze added (1). I implemented both. FTR, Gandalf insisted a lot that I should drop (1) and do only (2) — see bug 815852. > Unless there are other reasons, I suggest that we just do (1). Changing the > locale is a rare user story and I don't think we should sacrifice the memory > consumption by doing (2) just for that. Dropping (2) means that: • for the default locale (probably English), we’ll save some startup time; • for all other locales, the startup time will be much higher (one more XHR I/O) and we’ll have either a white screen or English strings until the app is loading — which is what bug 815852 was about. This would *not* be i18n-friendly. At all. I’d love to save 200ms on the Settings app startup time but I’d prefer to consider any other approach that does not degrade the user experience for all languages except the default one. Note that the JSON dictionary is at the end of the HTML document: the document is rendered before the dictionary is loaded. Your numbers /might/ over-represent the actual startup time cost as perceived by the user — I’d need to have two devices side-by-side to compare. On the same line, the overall weight of the Settings app is much more than just the HTML file. I agree the current approach is not perfect and doesn’t scale well as the number of supported locales grows, but favoring one language against all others is not a solution imho. I’m tempted to think that a simple templating system would be nice to use on the Settings app because it would allow us to split the big HTML document & JSON dictionary in small chunks, though I don’t know what would be the trade-off for the sub-panel startup time. At the beginning of my involvement on Gaia I dreamed of a way to control Gecko’s cache in a way that would allow us to keep external l10n resource files, and cache the localized DOM document. If that’s feasible, the very first launch of an app would take some time but all other launches would be quicker (the localization cost would be null after the first launch). We also considered relying on the fact that application manifests can specify one entry point per locale: each locale would have its own HTML file (including its own JSON dictionary). That isn’t supported yet by web activities, though.
Flags: needinfo?(kaze)
(In reply to Fabien Cazenave [:kaze] from comment #8) > TL;DR: the startup time would be quicker for the default locale and much > longer for all other locales, and I don’t think it’s acceptable. To be clear: the “default locale” here is defined on build time, not by the user. Users who don’t speak the build-time default language would have significantly longer startup times and flickering effects on startup all the time, for all apps (see bug 815852) — thus, defeating the point in deploying multi-locale builds.
Thanks Kaze. This sounds like tef- to me at this point.
(In reply to Fabien Cazenave [:kaze] from comment #8) > I implemented both. FTR, Gandalf insisted a lot that I should drop (1) and > do only (2) — see bug 815852. FTR - I insisted on doing (2) for default locale only. Not for all locales available. I'm still not sure how much of a win the (1) really is in terms of numbers, but that's the same for (2). And I'm worried about how much of the startup timing conversation happens and how many decisions are made without actual numbers. > Dropping (2) means that: > • for the default locale (probably English), we’ll save some startup time; > • for all other locales, the startup time will be much higher (one more XHR > I/O) and we’ll have either a white screen or English strings until the app > is loading — which is what bug 815852 was about. I believe it's a bit more complex, because the current process is entangled. In order to verify how much is "much higher" we'd have to remove the strings from HTML and block firstPaint until this one XHR I/O and node translation happens. The result should be then compared to current numbers. This would give us an actual price of runtime localization without inlining all available locales in HTML. My guestimation is that this could cost us ~150-250ms. And then, if Stas is right about 200ms of JSON load cost, then maybe there would be little or no price at all. I don't know.
(In reply to Zbigniew Braniecki [:gandalf] from comment #11) > FTR - I insisted on doing (2) for default locale only. Not for all locales > available. I can think of three easy ways to handle such client-side localization in a locale-neutral way, i.e. without degrading the performances for all locales except the default one: A. remove all textContent from the HTML document + one inline multi-locale JSON dictionary B. remove all textContent from the HTML document + one external JSON dictionary per locale C. one pre-translated HTML document per locale + inline JSON dictionary We’re currently close to (A). I did not remove the textContent from the HTML document because I had no data that proved it was necessary, and because we’ve considered to do it in the source HTML documents instead of during the build (as suggested by Margaret). Your suggestion is close to (B). I’m worried that blocking firstPaint until the external dictionary is loaded will cause some delay (I expect some flickering at startup) but I haven’t tried it. I’m pretty sure (C) would be the fastest but we’d need to either solve bug 811540 or tweak the application manifests in order to use one different entry point for each locale. The main drawback is that it would take more disk space. > I'm still not sure how much of a win the (1) really is in terms of numbers, > but that's the same for (2). And I'm worried about how much of the startup > timing conversation happens and how many decisions are made without actual > numbers. We had your data when working on bug 815852, that’s why we are doing (1) and (2): we trusted you on this. And for what I saw when I worked on this bug, the main win comes from (2). The Settings app is a bit extreme because most of the HTML is commented out and the JSON dictionary is huge — hence, (1) is negligible compared to (2) here. > My guestimation is that this could cost us ~150-250ms. And then, if Stas is > right about 200ms of JSON load cost, then maybe there would be little or no > price at all. A good way to know would be to try (B), i.e. relying on external JSON dictionaries for all locales. I doubt my manager would let me work on this but if you have some time to create a branch for that, I’d be interested by your measurements.
tef- until we have a clear proposal that increases performance in all situations.
blocking-b2g: tef? → -
(In reply to Fabien Cazenave [:kaze] from comment #12) > We had your data when working on bug 815852, that’s why we are doing (1) and > (2): we trusted you on this. And for what I saw when I worked on this bug, > the main win comes from (2). The only numbers in that bug (despite pretty lenghty conversation) were in bug 815852 comment 19. The numbers (mid-Dec 2012) were as follows: - avg. 412ms for end-to-end l10n - 60ms per file I/O async - node translation avg. 50ms I expect those numbers to be different today, esp. for the whole end-to-end. > Your suggestion is close to (B). I’m worried that blocking firstPaint until the external dictionary is loaded will cause some delay (I expect some flickering at startup) Yeah. The only way to remove to flickering is to block displaying on l10n. I don't know the cost. > And for what I saw when I worked on this bug, the main win comes from (2). Yes. I would also expect more win from (2) than (1) and that's in line with the numbers from mid-Dec. Coming back to the question about this bug prospect. I believe we need numbers. "Main win" from including JSON blob is meaningless and not comparable to "bigger gains" from removing JSON blob. We have to replace it with the actual numbers if we want to evaluate the solution proposal. My long term concern about the current approach is scalability. How many locales we will want to support out of the box in the future? Do we want to carry JSON blobs for 40 locales? For 50? But that's mid to long term, not tef.
If (B) turned out to be “cheap” performance-wise, it’d be a fine mid-term solution. (In reply to Zbigniew Braniecki [:gandalf] from comment #14) > My long term concern about the current approach is scalability. How many > locales we will want to support out of the box in the future? Do we want to > carry JSON blobs for 40 locales? For 50? I worry about that, too. The big multi-locale JSON dictionary doesn’t claim to be a long-term solution. But thinking of long-term: • do we want to support language packs? The platform implications would probably require to store the l10n data very differently… • how will FxOS be deployed, and by whom? Will we have language-specific updates or should we aim for multi-locales updates? If we want to ship 80 locales with Gaia, we will have a scalability issue anyway — just consider the size on the disk. Solving this scalability issue will require more than just splitting a JSON dictionary, and I’d prefer to have a plan on how we intend to handle 80+ locales before breaking something that works OK for now.
Attached file link to pull request (obsolete) (deleted) —
https://github.com/mozilla-b2g/gaia/pull/10554 If we don’t want to inline all locales in the HTML document, here’s a patch that allows to: • concatenate all l10n resources into a single, external JSON file for each HTML file; • use <link type="application/l10n" href="locales/app.{{locale}}.properties /> references. IMHO this patch is not really useful for real-world production builds, but it can be interesting for developer builds with lots of locales, where loading l10n resources asynchronously makes sense (⇒ the startup time will seem faster but the document gets translated a bit afterwards). This patch could also be used to get rid of *.ini files, but I’m not fond of the {{locale}} “magic” in the URL — I guess it should be discussed with the WebAPI team. At the moment I rather see this {{locale}} quasi-literal as a part of the build-time optimization. Note also that the concatenation can lead to heavy `application.zip' files for some webapps, e.g. Settings where the biggest l10n file ever (settings.*.properties) is used by two pages (index.html and onpair.html). It would be much more useful if we splitted the l10n files on a per-panel basis. For all these reasons, this optimization is not enabled by default. If you want to test it, please set the `GAIA_CONCAT_LOCALES' environment variable to 1: GAIA_CONCAT_LOCALES=1 make
Assignee: nobody → kaze
Attachment #766892 - Flags: review?(21)
Attached file link to pull request — split the JSON dictionary (obsolete) (deleted) —
https://github.com/mozilla-b2g/gaia/pull/10544 Here’s a complimentary approach: instead of embedding a single JSON dictionary for all supported locales in the HTML document, we can embed one JSON dictionary per supported locale. The tests I did seem to show that it divides by two the “startup time cost” of each locale: (tested on Unagi, v1-train, Settings startup time — average on 5 tries): 4 locales, without patch: 991 ms (1043+983+968+1011+949) 4 locales, with patch: 977 ms (1020+989+932+934+1009) 61 locales, without patch: 1928 ms (1971+1927+1942+1894+1907) 61 locales, with patch: 1521 ms (1514+1542+1486+1555+1506) The Settings app is quite an extreme use case because it includes a very big *.properties file, hence a startup cost of ~9 ms/locale on my Unagi (16 ms/locale without the patch). However, the real solution would rather be to split the l10n resources on a per-panel basis, which would lower the startup cost to ~1 ms/locale, maybe less. The main benefit of this patch is rather to avoid a RAM consumption peak when dealing with a big number of locales: instead of parsing a JSON object for 60 locales, we only parse a JSON object for one locale.
Attachment #766899 - Flags: review?(21)
(In reply to Fabien Cazenave [:kaze] from comment #15) > If (B) turned out to be “cheap” performance-wise, it’d be a fine mid-term > solution. > > (In reply to Zbigniew Braniecki [:gandalf] from comment #14) > > My long term concern about the current approach is scalability. How many > > locales we will want to support out of the box in the future? Do we want to > > carry JSON blobs for 40 locales? For 50? > > I worry about that, too. The big multi-locale JSON dictionary doesn’t claim > to be a long-term solution. > > But thinking of long-term: > • do we want to support language packs? The platform implications would > probably require to store the l10n data very differently… > • how will FxOS be deployed, and by whom? Will we have language-specific > updates or should we aim for multi-locales updates? > > If we want to ship 80 locales with Gaia, we will have a scalability issue > anyway — just consider the size on the disk. Solving this scalability issue > will require more than just splitting a JSON dictionary, and I’d prefer to > have a plan on how we intend to handle 80+ locales before breaking something > that works OK for now. the future is hitting us right now. we need both a developer solution and a shipping solution. we are trying to add at least ca, ja, th, and 14 indian languages and others into the build process so they can get some testing and dogfooding, but can't because of performance concerns. these are localizations that are basically complete, but are getting no testing. then when they do get testing how is it that we are going to ship them? ca expects to ship in a release very soon, but how can we do that without lots of testing to determine performance impact. we need to get out of that incremental performance testing loop each time we add a locale. It is killing us! on devices that have lots of storage the simplest configuration management and distribtuion mechanism will be to throw all languages on the devices. all other solutions are going to be complex and a mess to manage. >>> re: comment 26 IMHO this patch is not really useful for real-world production builds, but it can be interesting for developer builds with lots of locales, where loading l10n resources asynchronously makes sense (⇒ the startup time will seem faster but the document gets translated a bit afterwards). >>>> lets get working on proposing solutions that are going to solve what we have called our long term problems. those problems are here now and hitting us hard in v1.1
(In reply to chris hofmann from comment #18) > on devices that have lots of storage the simplest configuration management > and distribtuion mechanism will be to throw all languages on the devices. > all other solutions are going to be complex and a mess to manage. Of course it’d be simpler for us — and it would be fine for our community — but I’m not sure it’s going to be the preferred solution for real-world users, hence my question in comment 15. Even Firefox is shipped on a per-locale basis, and it’s not updated over 3G. Shipping FxOS with 80 locales will have a significant bandwidth cost, and I don’t know if it would be acceptable or not. Well, unless we can come up with a way to download separate language packs for FxOS… (In reply to chris hofmann from comment #18) > the future is hitting us right now. we need both a developer solution and a > shipping solution. > […] > lets get working on proposing solutions that are going to solve what we have > called our long term problems. those problems are here now and hitting us > hard in v1.1 That’s what these two patches are about: • splitting the JSON dictionary on a per-locale basis should get us out of trouble for the targeted leo locales (speed + RAM usage) and could be applied right now; • concatenating all l10n data into an external, locale-specific JSON file is scalable since we don’t load /all/ locales along with the HTML file. In both cases, the performance issues are mostly caused by the fact that we’re using monolithic l10n files: one l10n file per app, instead of one l10n file per panel. The “GAIA_CONCAT_LOCALES” patch won’t bring any performance improvements until we split the l10n resources on a per-panel basis, that’s why it’s pref’ed off at the moment. Most apps are now split into several panels, with various hacks. If we used plain HTML files for each panel (there’s been a recent proposal about that on m.d.gaia), it would be trivial to keep these monolithic l10n files and modify the build system to split them in a way that matches every panel. We could take the best of the two solutions by: • inlining the l10n data that is used declaratively (data-l10n-id) to ensure the first panel is displayed as quickly as possible, without any flickering when using a non-default locale; • keeping the l10n data that is used dynamically (with mozL10n.get / mozL10n.localize) in external files. Of course, we could also split these l10n files manually right now to use this, and we’d have a fast *and* scalable solution.
(In reply to chris hofmann from comment #18) > (In reply to Fabien Cazenave [:kaze] from comment #15) > > If (B) turned out to be “cheap” performance-wise, it’d be a fine mid-term > > solution. > > > > (In reply to Zbigniew Braniecki [:gandalf] from comment #14) > > > My long term concern about the current approach is scalability. How many > > > locales we will want to support out of the box in the future? Do we want to > > > carry JSON blobs for 40 locales? For 50? > > > > I worry about that, too. The big multi-locale JSON dictionary doesn’t claim > > to be a long-term solution. > > > > But thinking of long-term: > > • do we want to support language packs? The platform implications would > > probably require to store the l10n data very differently… > > • how will FxOS be deployed, and by whom? Will we have language-specific > > updates or should we aim for multi-locales updates? > > > > If we want to ship 80 locales with Gaia, we will have a scalability issue > > anyway — just consider the size on the disk. Solving this scalability issue > > will require more than just splitting a JSON dictionary, and I’d prefer to > > have a plan on how we intend to handle 80+ locales before breaking something > > that works OK for now. > > the future is hitting us right now. we need both a developer solution and a > shipping solution. > > we are trying to add at least ca, ja, th, and 14 indian languages and > others into the build process so they can get some testing and dogfooding, > but can't because of performance concerns. these are localizations that are > basically complete, but are getting no testing. > > then when they do get testing how is it that we are going to ship them? ca > expects to ship in a release very soon, but how can we do that without lots > of testing to determine performance impact. we need to get out of that > incremental performance testing loop each time we add a locale. It is > killing us! Then just load ca and not 'ja, th, and 14 indian languages and others' ?
Also if those problems are hitting us hard as said in https://bugzilla.mozilla.org/show_bug.cgi?id=853933#c18 why leo? has not been asked on the bug? Let's raise that and see how much it should be prioritized.
blocking-b2g: - → leo?
It's very late in 1.1 to block the whole release on this change. However, as Choffman says, we are experiencing problems without it. Please continue the review and landing on master. If the patch is stable, and shows the gains that you expect, renominate at that time.
blocking-b2g: leo? → -
Attached file link to pull request (deleted) —
https://github.com/mozilla-b2g/gaia/pull/10709 This patch takes the best of the two former ones, and allows to support 60+ locales more easily by having: • one minimal JSON dictionary per locale in each HTML file; • one extensive JSON dictionary per locale in each webapp. More details on the pull request. Here are the test results on the Settings app: Gecko b2g18, Gaia master, unagi, non-default locale -- zip size and startup time. without patch, 4 locales: 2.8MB, 1173ms (1186+1170+1190+1159+1164) with patch, 4 locales: 2.7MB, 1153ms (1156+1132+1178+1153+1148) without patch, 61 locales: 6.1MB, 2181ms (2189+2192+2154+2165+2207) with patch, 61 locales: 6.1MB, 1151ms (1139+1193+1144+1150+1129) The “cost per locale” shows that this approach is both fast and scalable: without patch: 58 KB, 17 ms with patch: 60 KB, 0 ms The storage cost is almost unchanged, the startup time no longer depends on the number of locales, and we keep the advantages of the inline JSON dictionary: the translation of the initial UI is done before first paint, no need to wait for the `localized' event to start an app. This patch only affects l10n.js and the build system: apps are unchanged, thus limiting the risk of uplifting it.
Attachment #766892 - Attachment is obsolete: true
Attachment #766899 - Attachment is obsolete: true
Attachment #766892 - Flags: review?(21)
Attachment #766899 - Flags: review?(21)
Attachment #769508 - Flags: review?(21)
Attachment #769508 - Flags: review?(21) → review?(anthony)
Attachment #769508 - Flags: review?(poirot.alex)
Attachment #769508 - Flags: review?(anthony)
Attachment #769508 - Flags: feedback?(anthony)
Comment on attachment 769508 [details] link to pull request will have a look too.
Attachment #769508 - Flags: feedback?(felash)
Thanks Julien!
I did a preliminar review, I have mainly suggested some simplification to ease code comprehension and also, I missed various comments here and there. I need another round to give it a try, but the patch looks in good shape!
Comments addressed, waiting for the second round. Thanks for the review!
Comment on attachment 769508 [details] link to pull request Works as described. We can see similar load times no matter how many locales we ship. Ship it!
Attachment #769508 - Flags: review?(poirot.alex) → review+
Merged on master: https://github.com/mozilla-b2g/gaia/commit/708543fe64a99f0f0a9d98b2e820283b902debd1 As per comment 22, we’ll renominate it for leo? next week.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to Dietrich Ayala (:dietrich) from comment #22) > It's very late in 1.1 to block the whole release on this change. > > However, as Choffman says, we are experiencing problems without it. > > Please continue the review and landing on master. If the patch is stable, > and shows the gains that you expect, renominate at that time. datazilla shows some gains for Settings, Music, Gallery, Email, Contacts, Phone (other apps are too unstable to validate abything). I looked at the Inari device where the results seems more stable.
Attachment #769508 - Flags: feedback?(felash)
Attachment #769508 - Flags: feedback?(anthony)
blocking-b2g: - → leo?
Sorry, we talked in triage and we're just way too far into 1.1 to take things without direct partner requirement, or fixing regression/stability/security issue. We're release more often now, so it'll be in 1.2 if that's a consolation.
blocking-b2g: leo? → -
Keywords: perf
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #30) > (In reply to Dietrich Ayala (:dietrich) from comment #22) > > It's very late in 1.1 to block the whole release on this change. > > > > However, as Choffman says, we are experiencing problems without it. > > > > Please continue the review and landing on master. If the patch is stable, > > and shows the gains that you expect, renominate at that time. > > datazilla shows some gains for Settings, Music, Gallery, Email, Contacts, > Phone (other apps are too unstable to validate abything). I looked at the > Inari device where the results seems more stable. CA is landing on 1.1 so it would be good to get all the precise data about how much this saved us, and/or the cost of adding one more locale without this patch on 1.1. Caze and all others, can you share your performance test scripts that you used in your assessments for this bug with QA?
AFAIK the script used for datazilla's launch time measurement is http://hg.mozilla.org/users/tmielczarek_mozilla.com/b2gperf also, you can try and use the script that comes with gaia : make test-perf APPS="apps to be tested" with a marionette-enabled device.
looks like datazilla isn't working right now. Anyone have thoughts on bug 898300 while dave hunt is on vacation?
Blocks: 898281
chofmann: see comment 23 please :)
Requesting leo? again because it blocks bug 898281, which is leo+.
blocking-b2g: - → leo?
Assuming this is low risk and solves a blocker I am leo+'ing this. PLease flag-us if you think this is too risky given we are close to shipping 1.1
blocking-b2g: leo? → leo+
v1.1.0hd: 0f6e9d96a7b374aaaf8e42efe987fbdc15e571ea v1.1.0hd: 25ff928244aeea1661cabca58beae966d2d35e74
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: