837111 - Minify js source

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

It looks like we currently do not minify our js source code. Minify it should help with memory usage and improve startup time because of less time spent parsing and compressing the source.

Comment 1

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

Bug 813396 landed opt-in support for concatenation on gaia/master:
https://github.com/mozilla-b2g/gaia/commit/53c83ac323095856522bc00aa41d20fda1594224

Only calendar was opted in.

For e-mail, we ran into the problem that minifiers based on performing full parse passes would break on us because they do not understand the entire 'dialect' of JS that SpiderMonkey understands.  Some of the ActiveSync code-base uses a combination of ES6-expected-legal stuff and possibly a few Mozilla-specific JS1.7+-isms.  https://bugzilla.mozilla.org/show_bug.cgi?id=812396#c0 implies that 'mince' should work, so I'll give it a spin, but otherwise I've had bad luck and would appreciate being pointed at known usable things.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

(In reply to Andrew Sutherland (:asuth) from comment #1)
> Some of the ActiveSync
> code-base uses a combination of ES6-expected-legal stuff and possibly a few
> Mozilla-specific JS1.7+-isms. 

How easy is it remove the use of these features?

Comment 3

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

(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> How easy is it remove the use of these features?

Easy compared to the alternatives, but I just got the node-jsmin2 port variant from https://github.com/twolfson/node-jsmin2 of Crockford's (C implemented) jsmin working on e-mail.  jsmin is fairly naive and just eats whitespace and comments, which seems generally desirable anyways as compared to variable renaming.  I did verify that it does not suffer from the "+ ++" => "+++" problem in the primary node-jsmin port.

The results aren't horrible and seem to work for me when I run with it:
gaia-email-opt.js:    1,277,298
gaia-email-opt.min.js:  582,173

Changes are on:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/119

We could change the ActiveSync code to allow a more extreme minifier to run, but more legwork would be required to be able to run our back-end unit tests to verify there is no breakage.

Other options in terms of better integrating with gaia and branching would be to run with https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/86 which copies over the files individually, and then concatenate/minify within gaia proper.  We might need some enhancements to the concatenation logic to support before/after stuff if it doesn't have it now, since we'd still want to net out to the same thing.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - add a simple minifier in the build system

This seems to have a positive impact on all apps. So let's add that to the build system directly.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 710697 [details] [diff] [review]
Patch - add a simple minifier in the build system

Review of attachment 710697 [details] [diff] [review]:
-----------------------------------------------------------------

::: Makefile
@@ -338,4 @@
>  	const GAIA_INLINE_LOCALES = "$(GAIA_INLINE_LOCALES)";                       \
>  	const GAIA_ENGINE = "xpcshell";                                             \
>  	';                                                                          \
> -	$(XULRUNNERSDK) $(XPCSHELLSDK) -e "$$JS_CONSTS" -f build/utils.js "build/$(strip $1).js"

Could you use subScriptLoader in JS instead of hacking the makefile?

With something similar to what is done for l10n module.
Something like this:
let scope = {};
Services.scriptloader.loadSubScript('file:///' + GAIA_DIR +
    '/build/jsmin.js?reload=' + new Date().getTime(), scope);
const { JSMin } = scope;

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I would like to land that on v1-train. This makes app launch faster globally. The risks is that some JS is written in a way that the minifier does not like and an error can happens. The minifier used is normally not very risky (it does not rename anything for example, just remove whitespaces and comments) but this risk always exists... So if something wrong happens once this patch lands it will always be possible to add an application to a blacklist that will resolves the issue immediatly.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v2 - add a simple minifier in the build system

Here the patch that address the review comments.

Comment 8

[David Scravaglieri [:scravag]]

Any information about performances ?
We are concern that if this patch lands we will have all errors pointed on line 1 and it will be difficult to see were the bug comes from.

Comment 9

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

The minifier maintains newlines.

Comment 10

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

Also, it's capable of generating source-map info for tools that consume it.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to David Scravaglieri [:scravag] from comment #8)
> Any information about performances ?
> We are concern that if this patch lands we will have all errors pointed on
> line 1 and it will be difficult to see were the bug comes from.

I'm concerned about the losing some informations as well. Someone needs to spend time playing with the map generated by the minifier but it could be done in a followup.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

fba586f468f6ac035f5c3e61e25e43d8fea7896f

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

As soon the bug 837139 lands I will be able to give more informations about the win here...

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - Use closure in order to apply more simple optimizations

Comment 15

[Rick Waldron [:rwaldron]]

Interesting, I think I posted

Comment 16

[Alex Keybl [:akeybl]]

If we do end up taking this bug for v1.0.1, we'll need to take bug 839574 as well.

Comment 17

[Alex Keybl [:akeybl]]

Given where we are in the cycle, we'll track instead of block a specific release.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #13)
> As soon the bug 837139 lands I will be able to give more informations about
> the win here...

Looks like it landed.  What's the news?

Comment 19

[Julien Wajsberg [:julienw]]

This does not seem to be in v1-train yet, while it's tracking+.

Comment 20

[Julien Wajsberg [:julienw]]

note: we need the uplift of Bug 812396 at first.

Comment 21

[Julien Wajsberg [:julienw]]

Uplifted Bug 812396 myself, so please go ahead (hoping there is no conflict).

Comment 22

[Julien Wajsberg [:julienw]]

So, for John, the first patch landed in fba586f468f6ac035f5c3e61e25e43d8fea7896f

I'll resolve this bug and file a new one for the second patch, to make things clearer.

Comment 23

[Michael Vines [:m1] [:evilmachines]]

(re: tef?, do we know how much perf win this gives us?)

Comment 24

[Michael Vines [:m1] [:evilmachines]]

(tef- for now as looks like we've already encountered one regression, bug 839574.  so unless the perf wins are amazing I don't think we want the risk)

Comment 25

[Julien Wajsberg [:julienw]]

The results are quite different depending on the app.

This patch does indeed 2 things: 
- add a minifier
- concatenates all js files for all apps (previously, this was done only for calendar)

For most apps we gain between 50 and 150ms, but for email this is 300ms, going from ~2200 to ~1900ms.

Even calendar that profits only from minification seems to gain ~70ms.

However merely cherry-picking this makes the FTU fail, I'll track this down before asking for tef? again.

Comment 26

[Julien Wajsberg [:julienw]]

FTU fail was caused by Bug 842249.

I feel that taking this bug with Bug 842249 and Bug 839574 is safe (several of our developers are running with this configuration for more than 1 month already) and makes enough improvement that we want it.

You might want Bug 840455 and Bug 841852 as well, as they add a GAIA_OPTIMIZE flag that you can turn off easily in the Makefile if necessary.

ask for tef? again then.

Comment 27

[Michael Vines [:m1] [:evilmachines]]

Okay thanks, yeah that's quite significant!   So I'm thinking that we don't want bug 840455 or bug 841852 on v1.0.1 because I'd prefer that the minifier be enabled in all builds for v1.0.1 by default to give us a better chance of catching any regressions introduced by this patch quicker (at the cost of reduced debugability, feedback on this trade off more than welcome!)

Comment 28

[Julien Wajsberg [:julienw]]

More information on this:
with bug 840455 and bug 841852, the optimizer is still turned on by default with |make production| or |make dogfood|.

I admit I don't know much how your builds are done :)

Comment 29

[Michael Vines [:m1] [:evilmachines]]

We use the stock android build system with no build.sh thinger.  PRODUCTION=1 is enabled by gaia/Android.mk for -user and -userdebug builds, which is almost enough.  I'd like this enabled for -eng builds as well on v1.0.1 cause some devs like to use -eng still, so they can, ummm, enjoy any bustage as well.

Comment 30

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 41fe27646971241cae65a51712fb0f1611a9fc38
v1.0.1: 0bf2642f7ee4d053f7662dcd0c817105900ed726

Comment 31

[amiller]

Does not make sense to create a regression issue.

Comment 32

[Darren]

Cannot verify, need steps to blackbox test this issue.

Comment 33

[Julien Wajsberg [:julienw]]

nothing to test here, just that the apps start faster than before.