903149 - Minify the chrome js code

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[[:fabrice] Fabrice Desré]

I did a quick experiment that consists in running a simple js minifier on js xpcom components and jsm modules that we ship with b2g. The minifier used is very safe: it only removes comments and formatting spaces.

The initial results are a reduction of 1MB in size for omni.ja, and a change in explicit allocation in the main process from 30.63 MB to 27.19 MB.

I also tested with fennec's omni.ja where we end up with a 400KB size reduction, but did not try yet on desktop.

All credits to jlebar that told me that since we were minifying gaia apps we should also do it for chrome.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: the minifier used.

Comment 2

[Justin Lebar (not reading bugmail)]

> All credits to jlebar that told me that since we were minifying gaia apps we should also do it for 
> chrome.

And yet, you actually did the work!

Comment 3

[Nicholas Nethercote [inactive]]

That's a surprisingly large improvement for just stripping comments and whitespace.  What's the file size reduction like?

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> That's a surprisingly large improvement for just stripping comments and
> whitespace.  What's the file size reduction like?

Normal size is 9636KB, minified 5808KB.

Comment 5

[Nicholas Nethercote [inactive]]

This is great -- what's the next step?

Comment 6

[[:fabrice] Fabrice Desré]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> This is great -- what's the next step?

Find someone familiar with the build system to let us do that behind a configure flag I guess...

Comment 7

[Mike Hommey [:glandium]]

I'd suggest importing the python jsmin implementation, which is derived from that c code (https://pypi.python.org/pypi/jsmin) and hooking it into the packager code:
http://hg.mozilla.org/mozilla-central/file/aebdc69b02e5/python/mozbuild/mozpack/files.py#l499

There's a flag to trigger that (MOZ_PACKAGER_MINIFY) and it's already set on both b2g and android.

Comment 8

[Gregory Szorc [:gps]]

I'm moving this to Core :: Build Config because I believe any patches will have cross product impact (even if we ultimately only enable minifying for B2G - at least initially).

There are some obvious perf wins here, so I'd like to see this landed. But I do have some questions:

1) What's the developer impact of shipping source that doesn't align with what's in the source tree? We have processes that look at line numbers in stack traces, etc. Minifying will obviously screw these up. I would like to know what the plan for mitigating this impact is. Presumably it involves source maps. But I'm not sure how source maps work or at what level we apply the source maps to unminify the output. Do we ship the source maps and have Firefox spit out the original line numbers, etc? Along that vein, are there downstream systems that index the in-package JS (as opposed to the original in source control)? Will this break OrangeFactor or other sheriffy tasks relying on certain output?

2) I reinforce glandium's suggestion that we use the Python implementation for minifying. That gives us more control over how minification is done and integrates with the build system better.

3) What are the plans for rolling this out to other products? Presumably desktop and Fennec will see similar benefits. IMO we should try to get this enabled everywhere when it lands unless there are significant drawbacks (e.g. impact on downstream systems caring about line numbers). i.e. balance the perf needs of B2G (and Fennec?) against the drawbacks.

4) Why stop at minifying JS - this still incurs parser overhead! Is there some kind of binary/bytecode representation we could ship instead? If not, is that something we should get the JS team thinking about?

Many of my questions could bikeshed into "perfect is the enemy of done." Let's not lose focus on the immediate goal of making B2G better, even if there are some drawbacks. We can initially land this on B2G for the immediate perf win and roll out to other products once drawbacks (if any) are addressed.

Comment 9

[Benjamin Smedberg]

We already have developer impact because of preprocessing. Source maps FTW, but later and we can make decisions about how to ship them also later.

Who wants to actually own this?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

My main concern from a sheriffing standpoint is what you already stated - it will make hunting down errors harder when the line number in the log doesn't match the raw source. I can't forsee any reason it would affect OrangeFactor, though.

Comment 11

[Gregory Szorc [:gps]]

Attachment: Part 1: Add jsmin Python package

Exported source code from changeset fdfa1b187681 from
https://bitbucket.org/dcs/jsmin without modifications.

Comment 12

[Gregory Szorc [:gps]]

Attachment: Part 2: Add jsmin to virtualenv

Comment 13

[Gregory Szorc [:gps]]

Attachment: Part 3: Minify packaged JavaScript

This hooks jsmin up to the packager code. I ran this locally and
verified the packager completes. I stopped short of verifying everything
actually loads. I figure someone else can do that.

This needs tests and verification. But other than that, this is a pretty
simple change. Yay for the rewritten packager code!

https://tbpl.mozilla.org/?tree=Try&rev=43d67414c3dc

Comment 14

[Gregory Szorc [:gps]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Who wants to actually own this?

I just submitted the initial patches to bootstrap the work. But I'd prefer someone else carry this over the finish line, if possible.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 797441 [details] [diff] [review]
Part 1: Add jsmin Python package

It would maybe be better to import a released version?

Comment 16

[Gregory Szorc [:gps]]

I manually reviewed the changesets between the last release and they are trivial changes to metadata. Using that logic, I could revert to a released changeset with no functional impact. However, since one of those changesets clarified the license of the code, leaving as-is may appease the lawyers.

Comment 17

[Ed Morley [:emorley]]

(In reply to Gregory Szorc [:gps] from comment #8)
> in source control)? Will this break OrangeFactor or other sheriffy tasks
> relying on certain output?

It won't break OrangeFactor, it will just mean manual cross referencing when using hg annotate requires bit more thought, but we already have to deal with that for preprocessed files - so I have no objections :-)

Comment 18

[Gregory Szorc [:gps]]

Attachment: Part 1: Add jsmin Python package

Exported source code from changeset fdfa1b187681 from
https://bitbucket.org/dcs/jsmin without modifications.

Just needs a rubber stamp.

Comment 19

[Gregory Szorc [:gps]]

Attachment: Part 2: Add jsmin to virtualenv

Comment 20

[Gregory Szorc [:gps]]

Attachment: Part 3: Minify packaged JavaScript

Now with a simple test. I packaged this locally on OS X and it appeared
to work.

https://tbpl.mozilla.org/?tree=Try&rev=191193e59e77

This try push has minification enabled for desktop builds, just for
kicks. Let's see if Talos tells us anything interesting.

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 799818 [details] [diff] [review]
Part 1: Add jsmin Python package

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

Might be good to put an in-tree note about where this was taken, at what revision.

Comment 22

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #20)
> This try push has minification enabled for desktop builds, just for
> kicks. Let's see if Talos tells us anything interesting.

minification could be something we put behind --enable-release.

Comment 23

[Gregory Szorc [:gps]]

Desktop builds on that try push are showing some syntax errors. Even if Android or B2G builds go green, it seems the minified JS isn't playing well in all cases. I think we should investigate the lossy conversation, patch minify, then land. Otherwise, we're taking a huge risk that code without test coverage may not parse properly.

Comment 24

[Gregory Szorc [:gps]]

One syntax error is reported in browser/base/content/pageinfo/pageInfo.js.

Original:

function checkProtocol(img)
{
  var url = img[COL_IMAGE_ADDRESS];
  return /^data:image\//i.test(url) ||
    /^(https?|ftp|file|about|chrome|resource):/.test(url);
}

Minified:

function checkProtocol(img)
{var url=img[COL_IMAGE_ADDRESS];return/^data:image\
/^(https?|ftp|file|about|chrome|resource):/.test(url);}

16:50:26     INFO -  System JS : ERROR chrome://browser/content/pageinfo/pageInfo.js:224
16:50:26     INFO -                       SyntaxError: unterminated regular expression literal
16:50:26     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug460146.js | Console message: [JavaScript Error: "unterminated regular expression literal" {file: "chrome://browser/content/pageinfo/pageInfo.js" line: 224 column: 38 source: "{var url=img[COL_IMAGE_ADDRESS];return/^data:image\
16:50:26     INFO -  "}]

Comment 25

[Gregory Szorc [:gps]]

Looks like this package has known issues with regex literals. I'll probably be contributing a patch to upstream. Although, we may land this first so this is landed.

https://bitbucket.org/dcs/jsmin/issue/4/escaped-backslash-in-regex-literal

Comment 26

[Gregory Szorc [:gps]]

Attachment: More support of regex literals in jsmin

This appears to fix the jsmin bug where regex literals aren't detected
properly. The underlying problem is the re literal detection has this
concept of "previous non-whitespace characters" that must precede a JS
literal. "return" was not allowed. So I allowed it.

No clue if there are other tokens that also need to be detected.
Probably! I figure a core SM developer can point out everything that's
wrong with this.

While I have some core SM developers here: I don't suppose you know of a
better way to minify JS? I know there's a bunch of tools out there.
Although I'm scared to death they don't support crazy new ES6 syntax yet
and attempting to run it on our chrome JS will produce all kinds of
badness.

It would be rad if SM had a tool to compare the "ASTs" of two pieces of
JS. i.e. if we could ask the JS engine to verify the before and after
source code were identical, that would give us some peace of mind. Do we
have anything like that?

https://tbpl.mozilla.org/?tree=Try&rev=90abfa3e0577

Comment 27

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #26)
> It would be rad if SM had a tool to compare the "ASTs" of two pieces of
> JS. i.e. if we could ask the JS engine to verify the before and after
> source code were identical, that would give us some peace of mind. Do we
> have anything like that?

It would be nice if we could ask the js engine to minify itself, although that would complicate things for cross-compiles.

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 799897 [details] [diff] [review]
More support of regex literals in jsmin

And the upstream project just committed a fix:

https://bitbucket.org/dcs/jsmin/commits/f73fdb89512114cbaa98dfe7a13301f3a1d8b322

I guess I can cancel this review.

Comment 29

[Gregory Szorc [:gps]]

https://tbpl.mozilla.org/?tree=Try&rev=2f500bb9f385 with latest upstream Python code.

I would still like a SM person to weigh in here. I worry relying on 3rd party code to perform minimization is risky in the face of a changing JavaScript language. SM will gain new language features before the minifier and these have the potential to break the minifier and introduce bugs in shipped code (often not caught by local development).

Comment 30

[Jason Orendorff [:jorendorff]]

Yes, it is possible to use the JS engine to check that two given scripts have the same AST. Here's a little script that does it:

    function ast(filename) {
        return JSON.stringify(Reflect.parse(snarf(filename), {loc: 0}));
    }

    if (scriptArgs.length !== 2)
        throw "usage: $OBJDIR/dist/bin/js checkit.js FILE1.js FILE2.js";
    assertEq(ast(scriptArgs[0]), ast(scriptArgs[1]));
    print("files match");

You can run this using the JS shell:

    $OBJDIR/dist/bin/js checkit.js FILE1.js FILE2.js

On success, the exit code will be 0. On failure, there will be some indecipherable error message about an assertion failure, and the exit code will be nonzero.

Regarding the other thing: from a risk perspective, it seems best to use existing code, with outside maintainers, that others are also using--preferably something whose maintainer is willing to take fixes from us when the language changes. That way others benefit from our fixes and we benefit from theirs. Now, whether that's jsmin or something else, I don't know, but either way, we should tell Dave St. Germain how much we love his stuff and send him a thank-you postcard.

Comment 31

[Gregory Szorc [:gps]]

Attachment: Part 3: Minify packaged JavaScript

Incorporated AST comparison (thanks Jason!). If the ASTs don't match up,
we use the unminified JS.

Unfortunately, there are a few dozen files where minification fails. I
figure perfect is the enemy of done and we can land this with
"partial" minification. We'll submit bug reports and repro cases
upstream and hopefully the minification code will eventually be perfect.

I can't submit to try right now because it is closed.

Open question: now that we invoke the js binary as part of minification,
perhaps we should just use a known and working JS minification library
instead of Python.

Open question: This patch regresses wall time of packaging due to
executing the js shell several times. It's very tempting to use the
xpcshell and to start a server in that process that processes commands.
This would reduce the process overhead. Although, it's considerably more
complicated. I'm not sure it is worth it. Perhaps we should hide JS
minification behind an additional flag so local developers aren't
inconvenienced by the performance overhead. (Shouldn't we do this anyway
since some development workflows rely on packaging and we don't want to
force minification on people?)

Anyway, there are plenty of open questions. I figure they'll get
addressed during review.

Comment 32

[[:fabrice] Fabrice Desré]

Great stuff Gregory! Could we implement our own minifier by re-serializing the AST?

Comment 33

[Gregory Szorc [:gps]]

Since the parser API exposes source location info for each node in the AST, I suppose it should be rather trivial to convert two iterables of AST nodes to a source map. Do we have anything in the tree that does this? I haven't made my way through all of https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/sourcemap/SourceMap.jsm yet...

Comment 34

[Jason Orendorff [:jorendorff]]

Please use an existing minifier.

Some minifiers do transformations like

    if (x) do_something();
    x&&do_something();

because hey look I saved 2 bytes. They can also rename local variables and on and on. So the test program I gave you will definitely balk in cases where the minifier is correct.

Comment 35

[Gregory Szorc [:gps]]

I eyeballed a few of the AST differences and it was pretty obviously a bug in the minifier's "parser." Things like line breaks added in the middle of comments.

Comment 36

[Gregory Szorc [:gps]]

Comment on attachment 802588 [details] [diff] [review]
Part 3: Minify packaged JavaScript

Gonna cancel this until more science is conducted.

Comment 37

[[:fabrice] Fabrice Desré]

(In reply to Jason Orendorff [:jorendorff] from comment #34)
> Please use an existing minifier.
> 
> Some minifiers do transformations like
> 
>     if (x) do_something();
>     x&&do_something();
> 
> because hey look I saved 2 bytes. They can also rename local variables and
> on and on. So the test program I gave you will definitely balk in cases
> where the minifier is correct.

That looks really risky to use this kind of "smart" minifier on our chrome code. I would be happy with one that does correctly the bare minimum: removing comments and blanks.

Comment 38

[Gregory Szorc [:gps]]

jorendorff pointed out bug 590755 on IRC.

So many choices.

Comment 39

[Mike Hommey [:glandium]]

Comment on attachment 802588 [details] [diff] [review]
Part 3: Minify packaged JavaScript

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

::: python/mozbuild/mozpack/files.py
@@ +503,5 @@
> +                subprocess.check_output([self._bin, self._ast_compare, fh1.name,
> +                    fh2.name], stderr=subprocess.STDOUT)
> +            except subprocess.CalledProcessError as e:
> +                errors.warn('JS minification failed for %s: %s' %
> +                    (self._file.path, e.output))

Note these errors would be triggered on cross-compiles.

Comment 40

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #39)
> Comment on attachment 802588 [details] [diff] [review]
> Part 3: Minify packaged JavaScript
> 
> Review of attachment 802588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozpack/files.py
> @@ +503,5 @@
> > +                subprocess.check_output([self._bin, self._ast_compare, fh1.name,
> > +                    fh2.name], stderr=subprocess.STDOUT)
> > +            except subprocess.CalledProcessError as e:
> > +                errors.warn('JS minification failed for %s: %s' %
> > +                    (self._file.path, e.output))
> 
> Note these errors would be triggered on cross-compiles.

Ugh. We don't build host native JS binaries do we :/ Ugh, scope bloat.

Comment 41

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

My understanding from reading the initial comments in the bug is that even the simplest possible minifier--one that merely de-indents code and removes comments--leads to a significant improvement. Is there any reason we shouldn't do this simple thing now then do something more later?

I think it is odd that we even keep the chrome JS source around in memory at all. I know that for web content we have to keep it around because people may do crazy things that require it. But, Chrome JS seems like an environment where we can avoid such crazy things. So, with that in mind, it seems reasonable to me (being relatively naive) to precompile the JS into a form that lets us throw away most of the source code bits and which is easier/faster to parse.

Comment 42

[Mike Hommey [:glandium]]

Note we already store chrome js in a kind of pre-parsed form on desktop (although the source is still kept around). We just don't on android and b2g because that requires running the built binaries, which we can't while cross-compiling.

Comment 43

[Simon Kornblith]

Since I don't see this mentioned here: esmangle (http://constellation.github.io/esmangle/) and escodegen (https://github.com/Constellation/escodegen) use the Mozilla Parser API. Together they can minify JavaScript and generate a SourceMap. There's an online demo at http://esprima.org/demo/minify.html that uses Esprima for parsing, although I think Reflect.parse() should work equally well. I don't know how closely they track new JS features or how well they perform, but they might be worth investigating.

Comment 44

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We have escodegen integrated in the devtools, but it doesn't support all the ES6 features we do, let alone our own mozilla js extensions. Constellation has been really receptive of ES6 patches, not sure about our extensions, or if this is the easiest path for this bug.

Comment 45

[Gervase Markham [:gerv]]

In case anyone was wondering :-), it's OK to remove license blocks when you do this.
http://www.mozilla.org/MPL/2.0/FAQ.html#minified-js

Gerv

Comment 46

[David Bruant]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> My main concern from a sheriffing standpoint is what you already stated - it
> will make hunting down errors harder when the line number in the log doesn't
> match the raw source.
Source map? http://www.html5rocks.com/en/tutorials/developertools/sourcemaps/
Already implemented and supported in devtools. No idea how much source maps weigh though.

Comment 47

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Source maps are used by devtools, but not exceptions, which is what we care about here.

Comment 48

[David Bruant]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #47)
> Source maps are used by devtools, but not exceptions, which is what we care
> about here.
Just so I understand, exceptions thrown contain line/column numbers of the shipped source. Currently it's the raw source, so debugging isn't an issue. If the shipped source is minified, the line/col of the exceptions does not corresponds to the initial source. That's where source maps come into play. They translate source coordinates from one file to another, so it's technically possible to map minified source error coordinates to actual source coordinates and get debugging back.

Comment 49

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

If we use source maps to allow for nice errors and debugging, are we going to embed the source maps in the build or host them on a server somewhere? If we were to include them in the build, it would likely make the build bigger than if we just didn't minify at all...

Comment 50

[Benjamin Smedberg]

For the immediate Firefox OS 1.2 timeframe, we are not going to generate source maps at all. Let's focus the discussion on that, and leave questions about desktop source maps for another time?

Comment 51

[Gregory Szorc [:gps]]

Here's where we are with this.

We have a short term goal to deploy minification to Firefox OS 1.2, whose train leaves Monday. For this goal, source maps or anything related to them are not a high priority. Minification for Fennec, Desktop, etc will be tackled at some future date.

The major blocker to this landing is that the minifier code is lossy. Furthermore, detecting lossy conversion on cross-compiles (of which many B2G builds are) is not possible in our current build system because detecting lossy conversion requires running the js binary, which is built for the target, not host machine. Without a host/native js binary, we won't be able to verify minification is sane as part of the build. This terrifies me because without verification that the output is sane, we're at the mercy of test coverage to find errors.

Talking things through with bsmedberg earlier today, I'm proposing that we focus on making the Python minifier non-lossy. Once we've achieved that we'll check this in for B2G. We will establish a temporary manual QA step to verify the minified JS is sane. Someone (likely me) will provide QA a script that can be used to compare the contents of two omni.jars files - one original, one minified. Eventually this verification procedure can be automated as part of release automation. We may even have automated coverage for B2G desktop builds, since a native js binary will be available at package time.

If people want to help making the Python minifier non-lossy, the source code lives at https://bitbucket.org/dcs/jsmin/. https://people.mozilla.org/~gszorc/minify_failures/ contains minification failures for desktop JS files. *I could use help filing bugs with repro cases and then submitting patches that fix them.*

Comment 52

[Gregory Szorc [:gps]]

Attachment: Part 3: Support for minifying packaged JavaScript

This patch implements a feature for minifying JavaScript as part of
packaging. It is *not* enabled anywhere because it hides behind a new
flag which isn't yet defined anywhere.

I refactored the verification mechanism a bit. We can now pass a program
name with optional arguments into packager.py. This program + arguments
will be invoked to verify the minification of JavaScript. Unlike the
previous approach, this is generically extensible.

If the build isn't cross compiling, the packager will invoke a
JavaScript script to compare the ASTs using the js binary.

Assuming it passes try, this patch should be safe to commit today as it
shouldn't change anything. I plan to commit with parts 1 and 2 ASAP.
We'll leave the bug open and will refine the Python minification code
and eventually enable JS minification for B2G builds in future patches.

https://tbpl.mozilla.org/?tree=Try&rev=9b7983833c9b

Comment 53

[Gregory Szorc [:gps]]

Here is a try push with minification enabled everywhere:

https://tbpl.mozilla.org/?tree=Try&rev=482b0c58d92b

Comment 54

[Mike Hommey [:glandium]]

Comment on attachment 803323 [details] [diff] [review]
Part 3: Support for minifying packaged JavaScript

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

::: python/mozbuild/mozpack/files.py
@@ +499,5 @@
> +                args = list(self._verify_command)
> +                args.extend([fh1.name, fh2.name])
> +                subprocess.check_output(args, stderr=subprocess.STDOUT)
> +            except subprocess.CalledProcessError as e:
> +                errors.warn('JS minification verification failed for %s:\n%s' %

I'm torn between failing the build deliberately in that case, and warning and returning the non-minified file like you do.

::: toolkit/mozapps/installer/packager.mk
@@ +627,5 @@
>  		$(addprefix --removals ,$(MOZ_PKG_REMOVALS)) \
>  		$(if $(filter-out 0,$(MOZ_PKG_FATAL_WARNINGS)),,--ignore-errors) \
>  		$(if $(MOZ_PACKAGER_MINIFY),--minify) \
> +		$(if $(MOZ_PACKAGER_MINIFY_JS),--minify-js \
> +		  $(if $(CROSS_COMPARE),,--minify-js-verify $(DIST)$(_BINPATH)/js --minify-js-verify $(MOZ_PKG_SRCDIR)/toolkit/mozapps/installer/js-compare-ast.js) \

I think you meant CROSS_COMPILE. MOZ_PKG_SRCDIR is most likely not what you're after. MOZILLA_DIR is.
I'm not a big fan of the "put several args for the command line" thing. It feels to me it would just be better to pass a --js-binary, and hardcode js-compare-ast.js in the packager, like it's done for precompile_cache.js. The --js-binary could even default to the "right" value in the packager itself, like it's done for xpcshell, and the CROSS_COMPILE check could be done there too.
So the --js-binary argument wouldn't really be necessary *except* if we want to allow some smarts from releng, which could download a jsshell tarball from a desktop build and use that.

::: toolkit/mozapps/installer/packager.py
@@ +239,5 @@
>      parser.add_argument('--minify', action='store_true', default=False,
>                          help='Make some files more compact while packaging')
> +    parser.add_argument('--minify-js', action='store_true',
> +                        help='Minify JavaScript files while packaging.')
> +    parser.add_argument('--minify-js-verify', action='append',

I'd remove action='append' ...

@@ +312,5 @@
>      with errors.accumulate():
> +        finder_args = dict(
> +            minify=args.minify,
> +            minify_js=args.minify_js,
> +            minify_js_verify_command=args.minify_js_verify,

... and split args.minify_js_verify.

Comment 55

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #54)
> I'm torn between failing the build deliberately in that case, and warning
> and returning the non-minified file like you do.

We could make this a flag on packager.py.

> ::: toolkit/mozapps/installer/packager.mk
> @@ +627,5 @@
> >  		$(addprefix --removals ,$(MOZ_PKG_REMOVALS)) \
> >  		$(if $(filter-out 0,$(MOZ_PKG_FATAL_WARNINGS)),,--ignore-errors) \
> >  		$(if $(MOZ_PACKAGER_MINIFY),--minify) \
> > +		$(if $(MOZ_PACKAGER_MINIFY_JS),--minify-js \
> > +		  $(if $(CROSS_COMPARE),,--minify-js-verify $(DIST)$(_BINPATH)/js --minify-js-verify $(MOZ_PKG_SRCDIR)/toolkit/mozapps/installer/js-compare-ast.js) \
> 
> I think you meant CROSS_COMPILE.

That explains the unexpected Try failures!

> I'm not a big fan of the "put several args for the command line" thing. It
> feels to me it would just be better to pass a --js-binary, and hardcode
> js-compare-ast.js in the packager, like it's done for precompile_cache.js.
> The --js-binary could even default to the "right" value in the packager
> itself, like it's done for xpcshell, and the CROSS_COMPILE check could be
> done there too.
> So the --js-binary argument wouldn't really be necessary *except* if we want
> to allow some smarts from releng, which could download a jsshell tarball
> from a desktop build and use that.

The releng use case is what I had in mind when I wrote this to be flexible. Of course, said feature would require packager.mk changes anyway, so perhaps YAGNI applies.

I should have patch with feedback addressed in a few minutes...

Comment 56

[Gregory Szorc [:gps]]

Attachment: Part 3: Support for minifying packaged JavaScript

Incorporated feedback. packager.py now takes --js-binary. It can be
supplied by an environment variable. If release automation can provide a
js binary during cross compile builds, all they need to do is define an
environment variable pointing to its path and verification will just
work.

https://tbpl.mozilla.org/?tree=Try&rev=fcfa655ce039

Comment 57

[Mike Hommey [:glandium]]

Comment on attachment 803400 [details] [diff] [review]
Part 3: Support for minifying packaged JavaScript

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

::: toolkit/mozapps/installer/packager.mk
@@ +621,5 @@
> +# (such as release automation) can provide their own js binary to enable
> +# verification when cross-compiling.
> +ifndef JS_BINARY
> +ifndef CROSS_COMPILE
> +JS_BINARY = $(DIST)$(_BINPATH)/js

$(wildcard), maybe (builds --with-libxul-sdk won't have $(DIST)$(_BINPATH)/js, for instance). We may want a followup to do the right thing with --with-libxul-sdk builds, btw.

@@ +637,5 @@
>  		$(addprefix --removals ,$(MOZ_PKG_REMOVALS)) \
>  		$(if $(filter-out 0,$(MOZ_PKG_FATAL_WARNINGS)),,--ignore-errors) \
>  		$(if $(MOZ_PACKAGER_MINIFY),--minify) \
> +		$(if $(MOZ_PACKAGER_MINIFY_JS),--minify-js \
> +		  $(if JS_BINARY,--js-binary $(JS_BINARY)) \

$(addprefix --js-binary ,$(JS_BINARY))

::: toolkit/mozapps/installer/packager.py
@@ +317,5 @@
> +
> +        finder_args = dict(
> +            minify=args.minify,
> +            minify_js=args.minify_js,
> +            minify_js_verify_command=verify_command,

might as well do
if args.js_binary:
    finder_args['minify_js_verify_command'] = [ args.js_binary, ... ]

(which would then use the actual default from the finder in the case where no js binary is given, instead of feeding None)

Comment 58

[Jason Orendorff [:jorendorff]]

I'm OK with patching JSMin, but I'm not confident in any simple program's ability to parse JS. We have to verify at build time.

Some TBPL build must break when JSMin goes wrong; but I'm not picky about which build. If we lack the capability to verify during a cross-compiling build, can we arrange for that exact code to be verified during other builds?

We shouldn't do this without verification. It's just too likely to go wrong.

Comment 59

[Gregory Szorc [:gps]]

We don't have a good, verifiable solution that works with cross compiles. Even if we patch jsmin and have non-cross-compile builds burning the tree if minification verification fails, there is no guarantee that the set of JS files on host native builds is identical to the set for cross-compile builds. So, additional verification beyond the build would be needed. This could be facilitated through additional automation configs or jobs. But, we still want to allow developers to do this locally or developers will get angry, I reckon.

I don't see how we can do this reliably without a host native js binary available at package time. How we get a host native js binary is up in the air. I'd hate to require dual SM builds when cross compiling, as this will make the build slower. Perhaps we'll publish pre-built binaries on a public server and have the build system attempt to fetch one as part of the build. If it's not available, we build a host native binary.

Anyway, from a #developers conversation, Fabrice says "tbh I'm a bit afraid of landing that now, unless we are 110% sure that that will never fail. It's ok to take our time and bake it slowly in 1.3."

I agree with this conclusion. Landing this by Monday would be a 4 alarm fire drill and would probably annoy a lot of people due to the shortcuts needed to get it landed. Since Fabrice is the feature requester and is OK with delaying this, let's delay it.

I guess the next steps are figuring out the least inconvenient way to provide a host native js binary to the packaging part of the build.

Comment 60

[Mike Hommey [:glandium]]

There's not much risk landing an opt-in feature, even now.

Comment 61

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #60)
> There's not much risk landing an opt-in feature, even now.

Yes. But from the sounds of it, jsmin will never be robust enough for our needs. So if we're never going to use it, why check it in?

We bought ourselves a few more weeks to figure out how to do this right. Let's not land something prematurely only to back it out later.

Comment 62

[Reuben Morais [:reuben]]

Is the plan still to only enable this on B2G and Android? I feel like the "view source" aspect of the Firefox UI is important enough to be weighed in when making the decision. I remember reading somewhere about being able to open a browser inside the browser using a chrome URL, then I figured out I could extract omni.jar and change things, and that got me interested enough to start doing Mozilla stuff. Maybe that's just me, or maybe we don't care, or maybe the package size/startup time/etc improvements are really good, but I just thought I'd leave my one cent.

Comment 63

[[:fabrice] Fabrice Desré]

(In reply to Gregory Szorc [:gps] from comment #61)
> 
> We bought ourselves a few more weeks to figure out how to do this right.
> Let's not land something prematurely only to back it out later.

Any hope we can make it happen for b2g 1.3 (ie before December 9)?

(In reply to Reuben Morais [:reuben] from comment #62)
> Is the plan still to only enable this on B2G and Android? I feel like the
> "view source" aspect of the Firefox UI is important enough to be weighed in
> when making the decision. I remember reading somewhere about being able to
> open a browser inside the browser using a chrome URL, then I figured out I
> could extract omni.jar and change things, and that got me interested enough
> to start doing Mozilla stuff. Maybe that's just me, or maybe we don't care,
> or maybe the package size/startup time/etc improvements are really good, but
> I just thought I'd leave my one cent.

Reuben, I think that the memory wins are worthwhile to turn that on at least on production builds. I don't expect people to use "view source" on devices. What would be great though is to get sourcemap support to get useful errors and js stack traces.

Comment 64

[[:fabrice] Fabrice Desré]

(In reply to Jason Orendorff [:jorendorff] from comment #58)
> I'm OK with patching JSMin, but I'm not confident in any simple program's
> ability to parse JS. We have to verify at build time.
> 
> Some TBPL build must break when JSMin goes wrong; but I'm not picky about
> which build. If we lack the capability to verify during a cross-compiling
> build, can we arrange for that exact code to be verified during other builds?
> 
> We shouldn't do this without verification. It's just too likely to go wrong.

Well, so what do you think we should do here?
Would fixing bug 590755 be a better alternative since it would be easier to make sure it supports all our JS features?

Comment 65

[Gregory Szorc [:gps]]

I believe the last working theory was "use esprima."

Comment 66

[[:fabrice] Fabrice Desré]

(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #65)
> I believe the last working theory was "use esprima."

Unfortunately if fails to parse :
const {classes:Cc, interfaces:Ci, utils:Cu, results:Cr} = Components;

Comment 67

[Arpad Borsos [:Swatinem]]

(In reply to Fabrice Desré [:fabrice] from comment #66)
> (In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from
> comment #65)
> > I believe the last working theory was "use esprima."
> 
> Unfortunately if fails to parse :
> const {classes:Cc, interfaces:Ci, utils:Cu, results:Cr} = Components;

Esprima has a `harmony` branch that should be able to parse destructuring assignments. And I added `const` a year ago so it should also handle that in master.

Not sure if esmangle/escodegen can work with those ASTs however.

I’m kind of surprised you mentioned esprima at all. What is the policy on introducing new dependencies (node) to the build system?

Comment 68

[Gregory Szorc [:gps]]

Node must not be a dependency for the build system.

Comment 69

[Arpad Borsos [:Swatinem]]

(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #68)
> Node must not be a dependency for the build system.

In that case esprima is out anyway.

Comment 70

[Thinker Li [:sinker]]

Since we are working on bug 944659, do we still need to minify the source code?  Minify don't actually change size much for distributing apps, but make the code more hard to be debugged.

Comment 71

[[:fabrice] Fabrice Desré]

(In reply to Thinker Li [:sinker] from comment #70)
> Since we are working on bug 944659, do we still need to minify the source
> code?  Minify don't actually change size much for distributing apps, but
> make the code more hard to be debugged.

I think we need - I was seeing bigger savings here, but I'll do another round of measurements. And yes, that will make debugging harder, but not harder than debugging gaia!

Comment 72

[Joe Cheng [:jcheng] (please needinfo)]

Fabrice, since you are working on this, do you mind taking the bug so we know it's owned from bug queries?

triage: will wait for Fabrice's further anaylsis to decide whether this needs uplift for tarako

Comment 73

[Joe Cheng [:jcheng] (please needinfo)]

Fabrice, per triage, it is believed that after bug 944659 lands, this bug may no longer be needed. Can you please confirm this? thanks

Comment 74

[[:fabrice] Fabrice Desré]

(In reply to Joe Cheng [:jcheng] from comment #73)
> Fabrice, per triage, it is believed that after bug 944659 lands, this bug
> may no longer be needed. Can you please confirm this? thanks

I think it's too early to confirm. We'll need to reassess after bug 944659 lands. We also need to decide what is the threshold for an improvement to be considered worthwhile for tarako.

Comment 75

[Tony Chung [:tchung]]

leaving this as 1.3? for now until fabrice can assess the risks on bug 944659 after landing.  ni? fabrice to follow up.

Comment 76

[Tony Chung [:tchung]]

spoke to Fabrice, and he's fine with this landing for the Tarako branch when that happens.  1.3-.

Comment 77

[Joe Cheng [:jcheng] (please needinfo)]

adding [POVB] to note that this is for the tarako branch

Comment 79

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T? to keep it in Tarako triage radar. Moving towards using blocking flag, therefore removing [tarako][POVB] whiteboard

Comment 80

[Joe Cheng [:jcheng] (please needinfo)]

triage: if the script source bug 944659 can make it in time, then this bug is not needed, wait for another week to decide on this bug

Comment 81

[[:fabrice] Fabrice Desré]

(In reply to Joe Cheng [:jcheng] from comment #80)
> triage: if the script source bug 944659 can make it in time, then this bug
> is not needed, wait for another week to decide on this bug

We discussed that with Thinker and bug 944659 won't make it. So let's try to make this one move!

Comment 82

[[:fabrice] Fabrice Desré]

(In reply to Arpad Borsos (Swatinem) from comment #69)
> (In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from
> comment #68)
> > Node must not be a dependency for the build system.
> 
> In that case esprima is out anyway.

Why? The minify demo runs in a browser without any node dependency.

Comment 83

[Arpad Borsos [:Swatinem]]

(In reply to Fabrice Desré [:fabrice] from comment #82)
> (In reply to Arpad Borsos (Swatinem) from comment #69)
> > (In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from
> > comment #68)
> > > Node must not be a dependency for the build system.
> > 
> > In that case esprima is out anyway.
> 
> Why? The minify demo runs in a browser without any node dependency.

I was assuming you wanted some kind of command line tool. That being said, maybe its possible to hook it up with the js shell.

Comment 84

[Nicolas B. Pierron [:nbp]]

I think comment 30 provides the best approach for verifying.

Then I do not see why we would have any problem related to the cross compilation.  The problem is checking it, but nothing forbids us to do this verification process outside of the normal checking process of Gecko, or to depend on a stable version of the xpcshell for that.

For non-cross-compiled builds, we can do that as part of the normal build process.
For cross-compiled once, we can use a native xpcshell to check that for us.

Currently Gaia is importing a copy of the xpcshell for its build system, to generate a profile.  Why not also to verify that we are correctly minimizing Gecko's files with the same binary?

Comment 85

[Joe Cheng [:jcheng] (please needinfo)]

Fabrice, wonder if this is still being worked on? or do we still need this in Tarako? Thanks

Comment 86

[[:fabrice] Fabrice Desré]

(In reply to Joe Cheng [:jcheng] from comment #85)
> Fabrice, wonder if this is still being worked on? or do we still need this
> in Tarako? Thanks

I'm not actively worked on that these days. If someone has cycles to help, they are welcome

Comment 87

[[:fabrice] Fabrice Desré]

Attachment: Part 4: enable by default on b2g

So we were much closer to something that works than I though. This patch turns on minification by default for b2g. I tested locally by setting export JS_BINARY=XXX in my .userconfig to use a host js shell.

Comment 88

[Mike Hommey [:glandium]]

Comment on attachment 8390279 [details] [diff] [review]
Part 4: enable by default on b2g

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

This shouldn't be enabled without a path to a js shell for validation.

Comment 89

[[:fabrice] Fabrice Desré]

Attachment: minichrome-b2g.patch

Addressing review comment.

Comment 90

[thomas tsai]

Fabrice, should the new patch be in WIP patch to verify? and Is Old patch obsolete?

Comment 91

[[:fabrice] Fabrice Desré]

Al, there should be no "WIP" patch at this stage. They already caused trouble with QA reporting bugs because of them.

Comment 92

[Mike Hommey [:glandium]]

Comment on attachment 8393019 [details] [diff] [review]
minichrome-b2g.patch

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

r=me with the following fixed. Please also refresh part 3 according to the review comments.

::: b2g/installer/Makefile.in
@@ +33,5 @@
>  MOZ_PKG_MANIFEST = package-manifest
>  endif
>  
>  MOZ_PACKAGER_MINIFY=1
> +ifdef JS_BINARY

JS_BINARY is defined in packager.mk, so this needs to be tested after including it.
MOZ_PACKAGER_MINIFY_JS is used in a recipe, which means it won't be a problem if it's defined after the include.

Comment 93

[James Zhang (Spreadtrum)]

Is there any risk to land this patch?

Comment 94

[[:fabrice] Fabrice Desré]

(In reply to James Zhang from comment #93)
> Is there any risk to land this patch?

Someone will address the review comments (especially on part 2) before we land.

Comment 95

[James Zhang (Spreadtrum)]

(In reply to Fabrice Desré [:fabrice] from comment #94)
> (In reply to James Zhang from comment #93)
> > Is there any risk to land this patch?
> 
> Someone will address the review comments (especially on part 2) before we
> land.

Hi Joe and Steven,
    Please find and ask reviewer to comment here, thanks.

Comment 96

[[:fabrice] Fabrice Desré]

James, we have someone that will take care of that.

Comment 97

[Gregory Szorc [:gps]]

To be clear, part 2 will need re-review by a build peer before it lands.

Comment 98

[Michael Shal [:mshal]]

Attachment: 0002-Bug-903149-Minify-the-chrome-js-code-enable-by-defau.patch

Updated with #c92. r+ carried forward.

Comment 99

[Michael Shal [:mshal]]

Attachment: 0001-Bug-903149-Part-3-Support-for-minifying-packaged-Jav.patch

Aside from #c57, I had to make the following changes:

1) _BINPATH is only defined in OSX, so I changed the js path to $(DIST)/bin/js

2) python/jsmin is added to the SEARCH_PATHS so that jsmin can be found from mach, otherwise ./mach valgrind-test fails. (recommended by gps)

3) mozharness sees the "SyntaxError:" message when we fail to minify a js file, and stops the build step (see bug 988603). I hacked around this by doing:

+                for line in e.output.splitlines():
+                    errors.warn(line.replace('Error', ' Error'))

The for loop is so each line is prefixed with "Warning:" so it's obvious to a human that it's all part of the same warning message, and the replace() on Error is to avoid mozharness detection.

Comment 100

[Mike Hommey [:glandium]]

Comment on attachment 8397580 [details] [diff] [review]
0001-Bug-903149-Part-3-Support-for-minifying-packaged-Jav.patch

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

::: python/mozbuild/mozpack/files.py
@@ +632,5 @@
> +            except subprocess.CalledProcessError as e:
> +                errors.warn('JS minification verification failed for %s:' %
> +                    (getattr(self._file, 'path', '<unknown>')))
> +                for line in e.output.splitlines():
> +                    errors.warn(line.replace('Error', ' Error'))

This is kind of awful. Why does mozharness barf on SyntaxError? Can't we fix that instead?

::: toolkit/mozapps/installer/packager.mk
@@ +704,5 @@
> +# (such as release automation) can provide their own js binary to enable
> +# verification when cross-compiling.
> +ifndef JS_BINARY
> +ifndef CROSS_COMPILE
> +JS_BINARY := $(wildcard $(DIST)/bin/js)

I'd rather keep that as a =

Comment 101

[Michael Shal [:mshal]]

Attachment: 0001-Bug-903149-Part-3-Support-for-minifying-packaged-Jav.patch

Updated with review comments: the mozharness issue is fixed so the replace() goes away, and JS_BINARY uses = instead of :=

r+ carried forward

Comment 102

[Michael Shal [:mshal]]

Attachment: 0002-Bug-903149-Minify-the-chrome-js-code-enable-by-defau.patch

Updated to account for JS_BINARY changing in Part 3. The 'ifdef JS_BINARY' evaluates to true even if the wildcard evaluates to an empty string, so we use 'ifneq (,$(JS_BINARY))' instead.

r+ carried forward.

Comment 103

[Michael Shal [:mshal]]

try: https://tbpl.mozilla.org/?tree=Try&rev=202a9f13e01c

inbound:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/19f34c6aeb28
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/92f909129ecb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/796ec394eef2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/84861ae010f9

Comment 104

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19f34c6aeb28
https://hg.mozilla.org/mozilla-central/rev/92f909129ecb
https://hg.mozilla.org/mozilla-central/rev/796ec394eef2
https://hg.mozilla.org/mozilla-central/rev/84861ae010f9

Comment 105

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/42b1c6b0a4cf
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/5254530a5926
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/fd535bd2499c
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/e635e1ce1612

Comment 106

[thomas tsai]

Hi Fabrice and Chris: omni.ja of tarako is still about 5MB. Is it correct size by this fix? And does it relate to https://bugzilla.mozilla.org/show_bug.cgi?id=956982?

Comment 107

[[:fabrice] Fabrice Desré]

(In reply to thomas tsai from comment #106)
> Hi Fabrice and Chris: omni.ja of tarako is still about 5MB. Is it correct
> size by this fix? And does it relate to
> https://bugzilla.mozilla.org/show_bug.cgi?id=956982?

No it's not related to this bug. Are you saying that pvt builds have a 5MB omni.ja? I don't think it's true.

Comment 108

[thomas tsai]

Current user build or user-debug build of omni.ja is about 5MB. What's the correct size? How to get the correct size?

Comment 109

[[:fabrice] Fabrice Desré]

(In reply to thomas tsai from comment #108)
> Current user build or user-debug build of omni.ja is about 5MB. What's the
> correct size? How to get the correct size?

You didn't answer my question: is that in a pvt build? (also, please use IRC to quickly solve this kind of questions!)

the correct size is ~4MB, to get that you need to configure your build to set JS_BINARY to a host JS shell.

Comment 110

[James Zhang (Spreadtrum)]

export JS_BINARY=1 ?

Comment 111

[[:fabrice] Fabrice Desré]

(In reply to James Zhang from comment #110)
> export JS_BINARY=1 ?

No, export JS_BINARY=/path/to/host/js

you can download js shell binaries from https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

pick the jsshell-linux package.

Comment 112

[James Zhang (Spreadtrum)]

Thomas, I think we need put jsshell-linux-x86_64.zip to device/sprd.
Because we have many hudson server, I have no permission to put it to server folder, and I can put it to device/srpd git repo.
Fabrice, is it ok?

Comment 113

[[:fabrice] Fabrice Desré]

That looks a bit strange to me to commit that file. Can't you download it on demand like we do for xulrunner in gaia? In any case, do what's possible for you!

Comment 114

[Mike Hommey [:glandium]]

Worst case, you could use xpcshell instead of js, and that should be in the xulrunner archive you already get for gaia.

Comment 115

[thomas tsai]

We can build it now. And partner has been updated, too. Thanks!

Comment 116

[James Zhang (Spreadtrum)]

Can we land this patch on v1.4 for dolphin and v2.0 for tarako?

Comment 117

[[:fabrice] Fabrice Desré]

(In reply to James Zhang from comment #116)
> Can we land this patch on v1.4 for dolphin and v2.0 for tarako?

This patch is in 2.0 (it landed in gecko31). I would not backport on 1.4, dolphin is not memory constrained like tarako is.

Comment 118

[chaofei.wu]

remake[about_RAM_performance]

Comment 119

[chaofei.wu]

v2.0m need this.

Comment 120

[Keven Kuo [:kkuo]]

(In reply to chaofei.wu from comment #119)
> v2.0m need this.

Please check https://bugzilla.mozilla.org/show_bug.cgi?id=903149#c104
This patch is in 2.0m already. Thanks

--
Keven