Minify the chrome js code

RESOLVED FIXED in Firefox OS v1.3T

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

unspecified
mozilla31
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3T+, b2g-v1.3T fixed, b2g-v1.4 ?, b2g-v2.0 ?)

Details

(Whiteboard: [MemShrink:P1], URL)

Attachments

(5 attachments, 12 obsolete attachments)

7.72 KB, text/x-csrc
Details
17.78 KB, patch
glandium
: review+
Details | Diff | Splinter Review
852 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
13.13 KB, patch
mshal
: review+
Details | Diff | Splinter Review
1011 bytes, patch
mshal
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 787790 [details]
the minifier used.
> 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!
Whiteboard: [MemShrink]
That's a surprisingly large improvement for just stripping comments and whitespace.  What's the file size reduction like?
(Assignee)

Comment 4

4 years ago
(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.
This is great -- what's the next step?
Whiteboard: [MemShrink] → [MemShrink:P1]
(Assignee)

Comment 6

4 years ago
(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...
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

4 years ago
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.
Component: General → Build Config
Flags: needinfo?(ryanvm)
Flags: needinfo?(jimb)
Flags: needinfo?(emorley)
Product: Boot2Gecko → Core

Comment 9

4 years ago
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?
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.
Created attachment 797441 [details] [diff] [review]
Part 1: Add jsmin Python package

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

Updated

4 years ago
Assignee: nobody → gps
Created attachment 797442 [details] [diff] [review]
Part 2: Add jsmin to virtualenv
Created attachment 797443 [details] [diff] [review]
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
(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.
Assignee: gps → nobody
Comment on attachment 797441 [details] [diff] [review]
Part 1: Add jsmin Python package

It would maybe be better to import a released version?
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.
(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 :-)
Flags: needinfo?(emorley)
Flags: needinfo?(ryanvm)
Created attachment 799818 [details] [diff] [review]
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.
Attachment #799818 - Flags: review?(mh+mozilla)

Updated

4 years ago
Attachment #797441 - Attachment is obsolete: true

Updated

4 years ago
Assignee: nobody → gps
Created attachment 799819 [details] [diff] [review]
Part 2: Add jsmin to virtualenv
Attachment #799819 - Flags: review?(mh+mozilla)

Updated

4 years ago
Attachment #797442 - Attachment is obsolete: true
Created attachment 799820 [details] [diff] [review]
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.
Attachment #799820 - Flags: review?(mh+mozilla)

Updated

4 years ago
Attachment #797443 - Attachment is obsolete: true
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.
Attachment #799818 - Flags: review?(mh+mozilla) → review+
Attachment #799819 - Flags: review?(mh+mozilla) → review+
Attachment #799820 - Flags: review?(mh+mozilla) → review+
(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.
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.
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 -  "}]
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
Created attachment 799897 [details] [diff] [review]
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
Attachment #799897 - Flags: review?(jorendorff)
(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 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.
Attachment #799897 - Flags: review?(jorendorff)
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).
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(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.
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
Flags: needinfo?(jimb)
Created attachment 802588 [details] [diff] [review]
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.
Attachment #802588 - Flags: review?(mh+mozilla)

Updated

4 years ago
Attachment #799820 - Attachment is obsolete: true
(Assignee)

Comment 32

4 years ago
Great stuff Gregory! Could we implement our own minifier by re-serializing the AST?
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...
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.
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 on attachment 802588 [details] [diff] [review]
Part 3: Minify packaged JavaScript

Gonna cancel this until more science is conducted.
Attachment #802588 - Flags: review?(mh+mozilla)
(Assignee)

Comment 37

4 years ago
(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.
jorendorff pointed out bug 590755 on IRC.

So many choices.
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.
(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.

Updated

4 years ago
Depends on: 524858
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.
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

4 years ago
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.
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.
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

4 years ago
(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.
Source maps are used by devtools, but not exceptions, which is what we care about here.

Comment 48

4 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
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

4 years ago
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?
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.*
Created attachment 803323 [details] [diff] [review]
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
Attachment #803323 - Flags: review?(mh+mozilla)

Updated

4 years ago
Attachment #802588 - Attachment is obsolete: true
Here is a try push with minification enabled everywhere:

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

Updated

4 years ago
Attachment #799897 - Attachment is obsolete: true
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.
Attachment #803323 - Flags: review?(mh+mozilla) → feedback+
(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...
Created attachment 803400 [details] [diff] [review]
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
Attachment #803400 - Flags: review?(mh+mozilla)

Updated

4 years ago
Attachment #803323 - Attachment is obsolete: true
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)
Attachment #803400 - Flags: review?(mh+mozilla) → review+
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.
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.
There's not much risk landing an opt-in feature, even now.
(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.
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.
(Assignee)

Comment 63

4 years ago
(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.
(Assignee)

Updated

4 years ago
Blocks: 929945

Updated

4 years ago
Assignee: gps → nobody
(Assignee)

Comment 64

4 years ago
(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?
I believe the last working theory was "use esprima."
(Assignee)

Comment 66

4 years ago
(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;
(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?
Node must not be a dependency for the build system.
(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.
blocking-b2g: --- → 1.3?
Whiteboard: [MemShrink:P1] → [MemShrink:P1][tarako]
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.
(Assignee)

Comment 71

4 years ago
(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!
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
(Assignee)

Updated

4 years ago
Assignee: nobody → fabrice
Fabrice, per triage, it is believed that after bug 944659 lands, this bug may no longer be needed. Can you please confirm this? thanks
Flags: needinfo?(fabrice)
(Assignee)

Comment 74

4 years ago
(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.
Flags: needinfo?(fabrice)
Whiteboard: [MemShrink:P1][tarako] → [MemShrink:P1][tarako-p2]
leaving this as 1.3? for now until fabrice can assess the risks on bug 944659 after landing.  ni? fabrice to follow up.
Flags: needinfo?(fabrice)
spoke to Fabrice, and he's fine with this landing for the Tarako branch when that happens.  1.3-.
blocking-b2g: 1.3? → -
Flags: needinfo?(fabrice)
adding [POVB] to note that this is for the tarako branch
Whiteboard: [MemShrink:P1][tarako-p2] → [MemShrink:P1][tarako-p2][POVB]
Duplicate of this bug: 68686
triage: 1.3T? to keep it in Tarako triage radar. Moving towards using blocking flag, therefore removing [tarako][POVB] whiteboard
blocking-b2g: - → 1.3T?
Whiteboard: [MemShrink:P1][tarako-p2][POVB] → [MemShrink:P1]
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
(Assignee)

Comment 81

4 years ago
(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!
blocking-b2g: 1.3T? → 1.3T+
(Assignee)

Comment 82

4 years ago
(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.
(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.
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?
Fabrice, wonder if this is still being worked on? or do we still need this in Tarako? Thanks
Flags: needinfo?(fabrice)
(Assignee)

Comment 86

4 years ago
(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
Flags: needinfo?(fabrice)
(Assignee)

Comment 87

4 years ago
Created attachment 8390279 [details] [diff] [review]
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.
Attachment #8390279 - Flags: review?(mh+mozilla)
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.
Attachment #8390279 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 89

4 years ago
Created attachment 8393019 [details] [diff] [review]
minichrome-b2g.patch

Addressing review comment.
Attachment #8390279 - Attachment is obsolete: true
Attachment #8393019 - Flags: review?(mh+mozilla)

Comment 90

4 years ago
Fabrice, should the new patch be in WIP patch to verify? and Is Old patch obsolete?
Flags: needinfo?(fabrice)
(Assignee)

Comment 91

4 years ago
Al, there should be no "WIP" patch at this stage. They already caused trouble with QA reporting bugs because of them.
Flags: needinfo?(fabrice)
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.
Attachment #8393019 - Flags: review?(mh+mozilla) → review+
Is there any risk to land this patch?
Flags: needinfo?(fabrice)
(Assignee)

Comment 94

4 years ago
(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.
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(styang)
Flags: needinfo?(jcheng)
(Assignee)

Comment 96

4 years ago
James, we have someone that will take care of that.
Flags: needinfo?(styang)
Flags: needinfo?(jcheng)
To be clear, part 2 will need re-review by a build peer before it lands.
Created attachment 8397578 [details] [diff] [review]
0002-Bug-903149-Minify-the-chrome-js-code-enable-by-defau.patch

Updated with #c92. r+ carried forward.
Attachment #8393019 - Attachment is obsolete: true
Attachment #8397578 - Flags: review+
Created attachment 8397580 [details] [diff] [review]
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.
Attachment #803400 - Attachment is obsolete: true
Attachment #8397580 - Flags: review?(mh+mozilla)
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 =
Attachment #8397580 - Flags: review?(mh+mozilla) → review+

Updated

4 years ago
Depends on: 990616
Created attachment 8400816 [details] [diff] [review]
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
Attachment #8397580 - Attachment is obsolete: true
Attachment #8400816 - Flags: review+
Created attachment 8400818 [details] [diff] [review]
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.
Attachment #8397578 - Attachment is obsolete: true
Attachment #8400818 - Flags: review+
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
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(Assignee)

Comment 105

4 years ago
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
status-b2g-v1.3T: --- → fixed

Comment 106

4 years ago
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?
Flags: needinfo?(fabrice)
Flags: needinfo?(catlee)
(Assignee)

Comment 107

4 years ago
(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.
Flags: needinfo?(fabrice)

Comment 108

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

Comment 109

4 years ago
(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.
export JS_BINARY=1 ?
Flags: needinfo?(fabrice)
(Assignee)

Comment 111

4 years ago
(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.
Flags: needinfo?(fabrice)
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?
Flags: needinfo?(ttsai)
Flags: needinfo?(fabrice)
(Assignee)

Comment 113

4 years ago
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!
Flags: needinfo?(fabrice)
Worst case, you could use xpcshell instead of js, and that should be in the xulrunner archive you already get for gaia.

Comment 115

4 years ago
We can build it now. And partner has been updated, too. Thanks!
Flags: needinfo?(ttsai)

Updated

4 years ago
Flags: needinfo?(catlee)
Can we land this patch on v1.4 for dolphin and v2.0 for tarako?
status-b2g-v1.4: --- → ?
status-b2g-v2.0: --- → ?
Flags: needinfo?(fabrice)
(Assignee)

Comment 117

4 years ago
(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.
Flags: needinfo?(fabrice)

Comment 118

3 years ago
remake[about_RAM_performance]

Comment 119

3 years ago
v2.0m need this.

Comment 120

3 years ago
(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
You need to log in before you can comment on or make changes to this bug.