1087811 - [email] update minification step to work with newer ES6-ish features

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[James Burke [:jrburke]]

Currently the uglifyjs used inside r.js will likely not handle things like template strings and fat arrows, and at least template strings seem to be used in some of the gaia- custom elements (like gaia-header).

So we should see if we can get a minification strategy that will work with them.

On what is required:

Right now the biggest minification gain is removing comments. We do some variable mangling, but we keep line returns to help in debugging.

Comment 1

[James Burke [:jrburke]]

I have an initial pass at this in a branch, compare view:

https://github.com/jrburke/gaia/compare/bug1087811-email-es6-minify

Basically, can only get comment removal right now. See notes in the files. But it works, and it would allow at least arrow functions and template strings. I would not expect all es6 features to work, would need to review/try them through this minification to make sure. 

For the email app, the zip file sizes, in bytes:

master: 590500
branch: 652784

So a bit bigger, about 10% bigger. On the plus side, the build is faster. 

The increase in size needs to be weighed with the value of the new features. If we are tied to using custom elements that use template strings, then it may just be part of that cost. Switching all the bind(this) usage to fat arrows may save some space (but not 62KB worth!).

Hopefully over time it could get better once the language ASTs are settled and work more across tools. Right now I could not use esmangle to shorten variable names because of disparities in the expected AST (also why esprima-harmony is used here instead of Reflect.parse, see the notes).

Will want to talk more with the team before proceeding with official review (which would include asking a build/ reviewer for review), so just documenting what is bolted together so far.

Comment 2

[James Burke [:jrburke]]

Calendar is another app that could benefit from this approach. It is already using arrow functions, and it is not getting the full benefits of a comprehensive JS minification, since the existing jsmin gaia build step only finds and bundles scripts that are script src tags in the index.html, and not things dynamically loaded by the AMD loader, or by the Worker call.

So I changed calendar/build/build.js to use esomin (see branch link in previous comment), and here are some results for the size of the calendar application.zip files, in bytes:

master: 723425
branch: 604264

For the main layer that is created by the jsmin flow, gaia_build_defer_index.js (size when expanded out of the zip file):

master: 208026
branch: 207590

Not much change is expected there since the jsmin step does some identifier mangling/shortening for gaia_build_defer_index.js.

Overall, I think it is a win, and sets us up for later, when AST trees are more compatible across tools for ES6 features. So I will check with the calendar folks to see if they are OK with the change, then proceed with formalizing the pull request and getting official r? from build module, calendar. 

I will likely not change the email app yet to this process until we make the jump to arrow functions or template strings, but this change would help any of the apps that use AMD modules but do not use uglify already for the GAIA_OPTIMIZE=1 route.

Comment 3

[James Burke [:jrburke]]

I was mistaken about JSMin: it does not do variable mangling, just comment and whitespace removal. Got confused because of the lack of line returns in the file I saw.

jsmin is robust against some language changes because it does not create an AST based on language constructs, but rather a character state machine, looking mostly for comments and white space.

I expect it to be more robust vs. something that wants a valid AST. Some of the new features that use punctuation, like back ticks for template strings, could mess up the state machine, but may just be fine, would need more analysis and best to try some tests too.

Since the same trimming is all we get now with esomin, is it worth using esomin?

One nice thing about esomin: escodegen does allow for keeping line returns, which can help in debugging, where jsmin does not have any options, it collapses the lines.

The downside is needing to do more careful evaluation of es6 features with esomin as the AST support for the feature may not be there yet. While I prefer to have the line breaks for some chance at reasonable debugging, often devs will not consider the GAIA_OPTIMIZE=1 effects while doing code changes. 

On the plus side, if the AST approach does have issues, it just means the file does not get optimized, but it still continues to work. It is just a larger file. Just need to live with "skipping file x because of an error" in the build output.

With esomin, then we at least have a path for the future to get name mangling if we want it, even unused code removal, since AST tools can be used. The issue is that the AST tools are not all compatible yet with each other or with what may be possible in a specific JS engine as far as es6 support.

What path is chosen is likely up to the app owners. For email, my initial thought is that we still want to keep line returns, and we are OK with more careful es6 feature use consideration, so we likely want AST-powered tooling. Given that, I will still pursue providing a route for that in this ticket.

Comment 4

[James Burke [:jrburke]]

Attachment: GitHub pull request

The resulting pull request. Asking review from a few people:

:asuth, for the email parts.

:millermederios, for the calendar parts. Miller, if you prefer to not do this for calendar, I can just remove it, it is not critical to the core of the pull request, just seemed like an easy gain.

:ochameau, for the gaia/build modifications. I just picked you since you are on the Build Module Owners page, but feel free to point me elsewhere. 

This change should not affect other apps, just ones that opt-in to this, and it primarily benefits gaia apps that use an AMD module loader, where code can be dynamically loaded, so not caught by the existing jsmin/script src parts of the build.

If this lands, then I will broadcast the option on dev-gaia, and also comment on bug 913617 about Reflect.parse AST structure, to indicate it would be helpful long term for the use of AST-based minifiers in Gaia.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8511397 [details] [review]
GitHub pull request

Note that we never got the stack size boosted for xpcshell on linux.  This should be fine given that esprima at the time was out-of-date and major stack silliness (wrapper trampolines wrapping every call, expression parser was broken out so it resulted in like 8 levels of stack for most constructs) that is no longer an issue, but we do want to be aware that the change away from Reflect.parse does make blowing the stack more possible.

Comment 6

[James Burke [:jrburke]]

Good point about watching the stack. On that front:

* If we get this sorted and landed, then I will mention on bug 913617 we would benefit from the changes, and maybe Reflect gets updated in the near-ish future. Last I heard, supposedly ES6 will be "final" in June-ish, so hopefully there is an end point for the disparities, likely with some convergence even before then.

* Worse case, we switch to jsmin if there is an issue. We lose line returns, but the perf/file size gains would outweigh in that case. Many apps need to deal with this already, if they put most of their scripts in index.html and fall under the default gaia build scheme.

* For email, for this portion of build process (optimization, and not dependency parsing, which still uses Reflect), we were dependent on what uglifyjs was doing, and it was not using Reflect. I think the optimization part is also very contained: one file at a time, where the dependency parsing was doing more cross-file work.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8511397 [details] [review]
GitHub pull request

I really like that:
* the build system being 30s faster on PRODUCTION=1!! (1m40 down to 1m06)
* the optimize step is out of requirejs.js blackbox and just apply on the app stage directory.

What would be even better:
* use the same optimizer for all apps. I don't see why one app would need something different from others. jsmin sounds like the KISS solution.
You gave some benchmarks here, but it looks like you are comparing master vs branch only. So uglify vs esomin. I would be interested to see the actual difference between jsmin and esomin
* execute the (same) optimize step for all apps during webapps-optimize based on their stage directory, just before pushing to zip files. ("nice to have")

Comment 8

[James Burke [:jrburke]]

Comment on attachment 8511397 [details] [review]
GitHub pull request

Forgot to reset this: talked with Miller in person and said I would reformulate the patch to not include changing Calendar, just to clear this to land at some point. Might not get to it right away, but want to avoid this review request showing up in his list.

Comment 9

[James Burke [:jrburke]]

Comment on attachment 8511397 [details] [review]
GitHub pull request

Rebased, but carrying over previous reviews as no material changes, just dependency updates. Asking Ricky for additional feedback since this may impact build system refactoring and just want to get an OK from that perspective.

I have rebased the changes to latest master, with these changes:

* calendar no longer calls esomin, just email does now. This was to limit review burden on calendar.

* r.js is updated to 2.1.17. Apps should not notice a change in behavior, the previous version in the tree was r.js 2.1.8+ (contained a fix that ended up in 2.1.9). This newer version relies on esprima 2.1.0 so it supports tracing modules that use ES6 features supported by esprima, so it should allow apps that use r.js to start using those kinds of features, like arrow functions and template strings.

* esprima is now at 2.1.0 and officially supports some ES6 features. Also updated escodegen to 1.6.1, its latest release.

As to :ochameau's follow up questions:

1) Why not use the same minifier for all apps? For email, we want to have line returns in the minified files, so prefer to not use jsmin. Other apps are free to continue to use jsmin though, this new ast-based minifier is opt-in. There are good reasons to use jsmin, since it is flexible with language syntax changes, and I did not want to qualify all apps on the new minifier. I figured we would let it sit with email for a bit to see if it works out.

2) Executing same optimize step for all apps just before pushing to app zip: I would prefer to wait for the larger build system refactoring that is being planned before doing changes in that area.

So this change should be seen as an intermediate step. It is important for email though since we are working on some refactors that will use some ES6 features, and want the app to continue to build/minify correctly on master.

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8511397 [details] [review]
GitHub pull request

That's great!

We're going to introduce Gulp for build system new architecture. To make such thing happen, we could wrap esomin as a plugin for supporting Gulp streaming. Therefore, after configuration step is completed, we could pass a minification options in app's config to customize their options. 

{
  mangling: true
}

I'd like to see this happen in the future. I'm fine with this intermediate step.

Comment 11

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/62042ffcc8c6cca0f51ad23f5c2b979fc153b5a7