Closed Bug 821691 Opened 12 years ago Closed 12 years ago

Precompile application to the target locale

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
x86_64
Linux
Type:
defect

Tracking

(blocking-basecamp:+)

Status:
RESOLVED FIXED
Milestone:
B2G C3 (12dec-1jan)
Project Flags:
blocking-basecamp +

People

(Reporter: vingtetun, Assigned: kaze)

References

(Blocks 1 open bug)

Details

(Whiteboard: [target:12/21], QARegressExclude)

Attachments

(1 file, 1 obsolete file)

patch proposal
190 bytes, text/html
ochameau
: review+
Details
Application can be compile to the target locale. As a result l10n.js will *not* be executed at all during the boot up phase. An initial prototype to compile apps is https://github.com/vingtetun/gaia/tree/l10n-compile
blocking-basecamp: --- → ?
Whiteboard: [target:12/21]
blocking-basecamp: ? → +
Priority: -- → P1
Assignee: nobody → 21
Attached file patch proposal (obsolete) —
Work in progress. I’m still a bit unsure on how |make webapp-l10n| should pre-translate HMTL files: • either it should replace existing HTML files with pre-translated ones • or it should generate additional HTML files (e.g. index.html.en-US, index.html.pt-BR…) and one of these files would be added to the webapp zip archive as the main index.html file.
Assignee: 21 → kaze
Target Milestone: --- → B2G C3 (12dec-1jan)
Kaze, did you see gandalf's proposal in bug 815852? It seems like that might be an easier and less risky first approach for making performance good enough for v1.
Yes I saw it, I already replied to it and I even started to work on it. AFAIK it’s neither easier nor less risky.
Blocks: 815852
> AFAIK it’s neither easier nor less risky. Can you elaborate? According to numbers I got we actually spend: - around 400ms in CSS/JS resource loading - around 300ms in l10n resource loading - around 120ms in parsing l10n + node translation plus we sometimes repaint after firstPaint because translation happens after firstPaint. The approach proposed in bug 817048 for .prop file inlining removes l10n resource loading and firstPaint redrawing. The proposal used here does the same plus removes l10n parsing/node translation cost which is relatively low. If we want to shave time and we're ready to give up on webapp modularity, it sounds like a bigger win to also incorpoarate CSS/JS minification and inlining into the file. This could shave another ~400ms. On top of that, since we mess with it, we can also minify CSS/JS (bug 812396) which can save another 200-300ms depending on the size of JS/CSS resources. Out of all of that, pre-compiling translated nodes is the smallest win with significant impact on debugging, localization and development processes. If we want to get even that, than I recommend us to also compile JS/CSS resources into this result HTML file.
This is perf only, moving back into the triage bin, as per latest mail to b2g drivers.
blocking-basecamp: + → ?
(In reply to Zbigniew Braniecki [:gandalf] from comment #4) > > AFAIK it’s neither easier nor less risky. > > Can you elaborate? Inlining l10n resources would require: • the same patch on the build system (⇒ that’s why bug 815852 depends on this one); • some additional code to insert l10n resources in the <head> (⇒ more complexity); • another patch on l10n.js to prevent it from translating the HTML document when it’s loaded (⇒ more risk). As I started to work on inlining l10n resources, it became clear that this patch is a necessary first step. > Out of all of that, pre-compiling translated nodes is the smallest win with > significant impact on debugging, localization and development processes. Pre-compiling translated nodes means we don’t execute l10n.js *at all* during startup — you agreed in bug 815852 that there’s no faster approach! — and it doesn’t modify the HTML structure: even line numbers are preserved. Inlining resources will make it more difficult to debug (e.g. line numbers won’t match), and we still need a way to automate this operation: 1. retrieve all HTML files in a webapp; 2. for each file, load HTML document and parse l10n resources; 3. insert l10n resources in the document <head>. This patch does 1. and 2. — l10n processing (= pre-translation) is done automatically when the HTML document is loaded because that’s how l10n.js works. If I wanted *not* to translate the HTML document I’d have to modify l10n.js. This approach allows to pre-translate *and* opens the door to l10n resource inlining without changing the l10n.js library. I really don’t understand why it’s such a problem for you. > significant impact on debugging, localization and development processes. That’s just a build-time patch: we’re not modifying any existing file in the source directory (neither *.html nor *.properties). I can’t see any impact on localization nor development.
How much space in system.img does that cost per locale?
Thanks for the details. I test the numbers for this patch today. Can you write similar estimation of impact of precompilation proposal? Because my main point here is that on the same scale the amount of changes you will introduce with precompilation is much higher. (In reply to Fabien Cazenave [:kaze] from comment #6) > As I started to work on inlining l10n resources, it became clear that this > patch is a necessary first step. Oh, can you take a look at bug 819228. I don't seem to be able to get it resolved, and it seems that you'll need it anyway. > > Out of all of that, pre-compiling translated nodes is the smallest win with > > significant impact on debugging, localization and development processes. > > Pre-compiling translated nodes means we don’t execute l10n.js *at all* > during startup — you agreed in bug 815852 that there’s no faster approach! Yeah, once again. It's just that this win is marginally small compared to the impact it has. Please, understand, it's not binary "faster"/"slower" game. It's risk/win assessment. The "win" from getting JSON vs. properties or getting pre-translated nodes is marginally small. As I indicated, it should be no more than 30-60ms per file. What is the cost? Here I believe you see no cost where I see cost. > That’s just a build-time patch: we’re not modifying any existing file in the > source directory (neither *.html nor *.properties). I can’t see any impact > on localization nor development. In bug 812396 there's some discussion on wherever we should minify/combine CSS/JS resources at build time. It's not cost free in the same way as this is not cost free. It alters the way resources (in this example whole DOM tree) is stored on the production. To summarize, removing the l10n resource loading phase removes major slowdown in app startup. Removing CSS/JS resource loading is another major win. Removing node-translation phase gives minimal win. Now, I see two approaches: - we either want to introduce an intermediate step in building phase where we build a combined HTML file that is "static" - then your precompilation makes sense, but so does CSS/JS minification. - we want to workaround the current slow resource loading problem with a minimal hack that we will soon remove and get back to modular, webby, run time. I believe in the latter approach. What I understand from your reasoning is that you're baking solution that is in between those two. I'm mostly worried about two aspects: - it's impact on how gaia apps modularity and runtime localizability will be preserved once we stop depend on it internally with this approach removing runtime l10n phase overall. (possible impact on code that will operate on DOM nodes before they'd be runtime localized if not for precompilation) - it's impact on localization workflow. How do localizers test their localizations, how they discover bugs, where do they learn about missing entities etc. That's why I CC'ed Stas and Pike in this bug and I'm really worried that you didn't see reasons to get people from bug 815852 even CC'ed on this one.
> As I indicated, it should be no more than 30-60ms per file. It should be: per app.
(In reply to Zbigniew Braniecki [:gandalf] from comment #8) > Oh, can you take a look at bug 819228. I don't seem to be able to get it > resolved, and it seems that you'll need it anyway. Ugh, I didn’t see that bug. I had to write with my own `writeContent_UTF8' for this patch… I’ll see that with Alexandre tomorrow. > To summarize, removing the l10n resource loading phase removes major > slowdown in app startup. Removing CSS/JS resource loading is another major > win. Removing node-translation phase gives minimal win. As I wrote earlier: I’m not refusing to inline l10n resources, I just say that the really tricky part is the build system — and this bug is about changing the build system to handle l10n resources. Believe me or not: the fact that nodes are translated is just a side-effect. This is a required first step, and it’s neither slower nor riskier than inlining l10n resources. That’s why I start with this bug. > Now, I see two approaches: > - we either want to introduce an intermediate step in building phase where > we build a combined HTML file that is "static" - then your precompilation > makes sense, but so does CSS/JS minification. FTR, Vivien has tested CSS/JS minification and he reported that the benefit is really low. > - we want to workaround the current slow resource loading problem with a > minimal hack that we will soon remove and get back to modular, webby, run > time. It’s not a “minimal hack” — unless you consider that tweaking HTML documents with regexps and modifying l10n.js is safe. In both cases (node translation / inline resources), pre-processing HTML files with l10n resources should be done with DOM methods and in the build system. And again, that’s what this patch is about. > I'm mostly worried about two aspects: > - it's impact on how gaia apps modularity and runtime localizability will > be preserved once we stop depend on it internally with this approach > removing runtime l10n phase overall. (possible impact on code that will > operate on DOM nodes before they'd be runtime localized if not for > precompilation) l10n.js is still loaded and is still able to translate a whole document on-the-fly when the language is changed: it will just not block the startup any more. Being able to change the language dynamically is one of the goals we had from the very beginning of this project. I certainly won’t drop this feature now. > - it's impact on localization workflow. How do localizers test their > localizations, how they discover bugs, where do they learn about missing > entities etc. Nothing changes either, but we’ll get all “undefined entities” warnings at build time, too. > That's why I CC'ed Stas and Pike in this bug and I'm really worried that you > didn't see reasons to get people from bug 815852 even CC'ed on this one. Again, I didn’t close bug 815852: I’m just addressing this one now because it is a necessary first step.
blocking-basecamp: ? → +
Attachment #692934 - Flags: review?(poirot.alex)
This patch adds a 'GAIA_DEFAULT_LOCALE' variable to the Makefile (= 'en-US' by default) and pre-translates all HTML files for this default locale. At run-time, l10n.js won’t have to translate the HTML document if the selected locale matches the default one.
Comment on attachment 692934 [details] patch proposal Needs this small fix: https://github.com/fabi1cazenave/gaia/pull/7 Otherwise it looks good, at the end it isn't that hard to precompile html. I haven't been able to see regression while testing it, I'm just having random idea on how to make it even simplier. We can continue simplifying and lower the maintenance of this code, but given that there is doubt about the regression potential of this work, better land that sooner than later. So r+, but I'll try to provide some followups.
Attachment #692934 - Flags: review?(poirot.alex) → review+
I’ve just merged your fix, thanks! FTR: l10n.js is almost unchanged, the main change is that the mozL10n API is now defined /before/ the startup code, in order to play nice with the build system. https://github.com/mozilla-b2g/gaia/commit/028000c501e1d8b331f5995f5afb753fb2c82d59
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Jumping in late here, back from vacation.. I see this has been backed out yesterday. What was the reason? https://github.com/mozilla-b2g/gaia/commit/004f33794c15cdd202e4c3a1bc819fc4e25679d5 Also, how is this approach supposed to work with the commented out parts of the Settings app? And, what are the follow-up steps on the releng side to complete this?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file patch proposal —
Alexandre: this seems to work properly on my side, please test carefully.
Attachment #692934 - Attachment is obsolete: true
Attachment #694373 - Flags: review?(poirot.alex)
(In reply to Staś Małolepszy :stas from comment #14) > I see this has been backed out yesterday. What was the reason? The build process was broken and there was a bad mistake with relative paths. > Also, how is this approach supposed to work with the commented out parts of > the Settings app? No impact on that, it should work as before with the same perfs. > And, what are the follow-up steps on the releng side to complete this? I haven’t tested any releng build yet, but one of the obvious follow-ups would be to stop loading the [en-US] locale before any other locale. Side note: this patch now gives nice reports for all missing l10n entities that are found at build time.
Depends on: 823540
Blocks: 811540
Comment on attachment 694373 [details] patch proposal See coment in PR. Tested clean complete flash in english and french, then open various app and see bug 815852 being fixed.
Attachment #694373 - Flags: review?(poirot.alex) → review+
Thanks Alexandre! https://github.com/mozilla-b2g/gaia/commit/5fe546fda8a44a33e8feea782530b1827dd4ca1c
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [target:12/21] → [target:12/21], QARegressExclude
Whiteboard: [target:12/21], QARegressExclude → [target:12/21], QARegressExclude, [perf_tsk]
Whiteboard: [target:12/21], QARegressExclude, [perf_tsk] → [target:12/21], QARegressExclude, [FFOS_perf]
Whiteboard: [target:12/21], QARegressExclude, [FFOS_perf] → [target:12/21], QARegressExclude
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: