Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: vingtetun)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18+ fixed, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [FFOS_perf] QARegressExclude)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Updated

6 years ago
blocking-b2g: --- → tef?
Bug 813396 landed opt-in support for concatenation on gaia/master:
https://github.com/mozilla-b2g/gaia/commit/53c83ac323095856522bc00aa41d20fda1594224

Only calendar was opted in.

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

Comment 2

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

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

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

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

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

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

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

This seems to have a positive impact on all apps. So let's add that to the build system directly.
Assignee: nobody → 21
Attachment #710697 - Flags: review?(poirot.alex)
Comment on attachment 710697 [details] [diff] [review]
Patch - add a simple minifier in the build system

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

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

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

With something similar to what is done for l10n module.
Something like this:
let scope = {};
Services.scriptloader.loadSubScript('file:///' + GAIA_DIR +
    '/build/jsmin.js?reload=' + new Date().getTime(), scope);
const { JSMin } = scope;
Attachment #710697 - Flags: review?(poirot.alex) → review+
I would like to land that on v1-train. This makes app launch faster globally. The risks is that some JS is written in a way that the minifier does not like and an error can happens. The minifier used is normally not very risky (it does not rename anything for example, just remove whitespaces and comments) but this risk always exists... So if something wrong happens once this patch lands it will always be possible to add an application to a blacklist that will resolves the issue immediatly.
Created attachment 710730 [details] [diff] [review]
Patch v2 - add a simple minifier in the build system

Here the patch that address the review comments.
Attachment #710697 - Attachment is obsolete: true
Attachment #710730 - Flags: review+
Any information about performances ?
We are concern that if this patch lands we will have all errors pointed on line 1 and it will be difficult to see were the bug comes from.
Flags: needinfo?(21)
The minifier maintains newlines.
Also, it's capable of generating source-map info for tools that consume it.
(In reply to David Scravaglieri [:scravag] from comment #8)
> Any information about performances ?
> We are concern that if this patch lands we will have all errors pointed on
> line 1 and it will be difficult to see were the bug comes from.

I'm concerned about the losing some informations as well. Someone needs to spend time playing with the map generated by the minifier but it could be done in a followup.
Flags: needinfo?(21)
fba586f468f6ac035f5c3e61e25e43d8fea7896f
As soon the bug 837139 lands I will be able to give more informations about the win here...
Created attachment 712504 [details] [diff] [review]
Patch - Use closure in order to apply more simple optimizations
Interesting, I think I posted
If we do end up taking this bug for v1.0.1, we'll need to take bug 839574 as well.
Blocks: 814273
(Reporter)

Updated

6 years ago
Blocks: 817095
Given where we are in the cycle, we'll track instead of block a specific release.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #13)
> As soon the bug 837139 lands I will be able to give more informations about
> the win here...

Looks like it landed.  What's the news?
This does not seem to be in v1-train yet, while it's tracking+.
status-b2g18: --- → affected
Flags: needinfo?(jhford)
Whiteboard: [FFOS_perf]
note: we need the uplift of Bug 812396 at first.
blocking-b2g: - → tef?
Uplifted Bug 812396 myself, so please go ahead (hoping there is no conflict).
So, for John, the first patch landed in fba586f468f6ac035f5c3e61e25e43d8fea7896f

I'll resolve this bug and file a new one for the second patch, to make things clearer.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 849873
Blocks: 839574
(re: tef?, do we know how much perf win this gives us?)
(tef- for now as looks like we've already encountered one regression, bug 839574.  so unless the perf wins are amazing I don't think we want the risk)
blocking-b2g: tef? → -
The results are quite different depending on the app.

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

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

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

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

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

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

ask for tef? again then.
No longer blocks: 839574
blocking-b2g: - → tef?
Depends on: 842249, 839574
Okay thanks, yeah that's quite significant!   So I'm thinking that we don't want bug 840455 or bug 841852 on v1.0.1 because I'd prefer that the minifier be enabled in all builds for v1.0.1 by default to give us a better chance of catching any regressions introduced by this patch quicker (at the cost of reduced debugability, feedback on this trade off more than welcome!)
blocking-b2g: tef? → tef+
More information on this:
with bug 840455 and bug 841852, the optimizer is still turned on by default with |make production| or |make dogfood|.

I admit I don't know much how your builds are done :)
Flags: needinfo?(mvines)
We use the stock android build system with no build.sh thinger.  PRODUCTION=1 is enabled by gaia/Android.mk for -user and -userdebug builds, which is almost enough.  I'd like this enabled for -eng builds as well on v1.0.1 cause some devs like to use -eng still, so they can, ummm, enjoy any bustage as well.
Flags: needinfo?(mvines)
Blocks: 840455
status-b2g18-v1.0.1: --- → affected
v1-train: 41fe27646971241cae65a51712fb0f1611a9fc38
v1.0.1: 0bf2642f7ee4d053f7662dcd0c817105900ed726
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → fixed
Flags: needinfo?(jhford)

Comment 31

6 years ago
Does not make sense to create a regression issue.
Flags: in-moztrap-

Updated

6 years ago
Whiteboard: [FFOS_perf] → [FFOS_perf] QARegressExclude

Comment 32

6 years ago
Cannot verify, need steps to blackbox test this issue.
nothing to test here, just that the apps start faster than before.
You need to log in before you can comment on or make changes to this bug.