Closed Bug 1216598 Opened 9 years ago Closed 9 years ago

Camera app is not minified due to use of ES6 features

Categories

(Firefox OS Graveyard :: Gaia::Camera, defect, P1)

Product:
Firefox OS Graveyard
Component:
Gaia::Camera
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: aosmond, Assigned: aosmond)

References

Details

Attachments

(2 files)

Profile data collected using 2.2 camera app on 2.2 and master gecko
1.05 MB, application/zip
Details
[gaia] aosmond:bug1216598 > mozilla-b2g:master
46 bytes, text/x-github-pull-request
justindarc
: review+
Details | Review 
The camera app has regressed on its critical path and fully loaded metric according to raptor data for all configurations of flame 319/512/1024. Since the app has obviously changed between 2.2 and master, I loaded the same 2.2 app on both 2.2 and mozilla central profile-enabled builds for a fairer comparison which suggests this is a platform regression (somewhere).

The time to lazy load to be executing onCriticalPathDone where it calls require, to where the ZoomController constructor is called (the first method called after all of the modules are loaded). 2.2 app on 2.2 gecko took 1147ms to lazy load, while the 2.2 app on master gecko took 1541ms. While the figures are noisy, it is consistent in my testing that master lags 2.2.

I would further note the app time is generally slower right from the get go, with code embedded in main.js. For example when the app starts up initially, it first creates some settings objects and then requests then camera. On 2.2 this is < 100ms and on master this is >130ms. This seems to hold true in general when looking at execution slices across multiple profiling attempts over the past week.

Relevant timestamps for 2.2:
7173 -- Create settings
7227 -- Requested camera
7736 -- onCriticalPathDone
8883 -- ZoomController

Relevant timestamps for master:

122378 -- Create settings
122489 -- Requested camera
123367 -- onCriticalPathDone
124908 -- ZoomController

App startup data which prompted this can be found at https://gist.github.com/eliperelman/b81a27b7f7f3b3f75f49. Since other apps (including some which have not changed much such as video and FM radio) are experiencing regressions, this may be related.
See Also: → 1210849
I do not see any dramatic regressions on Octane Codeload [1].  Would you have a JS Shell test case which can highlight the same regression?

Kannan, are we able to reproduce such issues on JS benchmarks?

Andrew, can you bisect gecko versions, because Gecko 2.2 does not mean anything for JS engine developers, and reporting a bug across 10 versions of Gecko is probably the best way to remove any attention from this bug.

[1] https://arewefastyet.com/#machine=28&view=single&suite=octane&subtest=CodeLoad
Flags: needinfo?(kvijayan)
Flags: needinfo?(aosmond)
Bobby/Thinker,

Any help the performance team can provide to bisect gecko versions is appreciated. Based on Ting's profile info for other apps (such as: https://bugzilla.mozilla.org/show_bug.cgi?id=1210440#c19), JS parsing/execution is taking longer. 

Andrew Osmond is neck deep into chasing down other findings and getting them fixed.

Thanks
Hema
Flags: needinfo?(tlee)
Flags: needinfo?(bchien)
See Also: → 1216406
Ting would give explanation.
Flags: needinfo?(tlee) → needinfo?(janus926)
(In reply to Hema Koka [:hema] from comment #2)
> Any help the performance team can provide to bisect gecko versions is
> appreciated. Based on Ting's profile info for other apps (such as:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1210440#c19), JS
> parsing/execution is taking longer. 

It takes longer since there're more JS...
Flags: needinfo?(janus926)
(In reply to Ting-Yu Chou [:ting] from comment #4)
> (In reply to Hema Koka [:hema] from comment #2)
> > Any help the performance team can provide to bisect gecko versions is
> > appreciated. Based on Ting's profile info for other apps (such as:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1210440#c19), JS
> > parsing/execution is taking longer. 
> 
> It takes longer since there're more JS...

Sorry I should've read this bug thoroughly. What I meant was for bug 1210440 comment 19 since Music NGA creates more JS which for sure takes longer on parsing/execution. For the issue here, I have the same question as bug 1210440 comment 24, you did the test by:

  v2.2 Camera + v2.5 Gaia + v2.5 Gecko, or
  v2.2 Camera + v2.2 Gaia + v2.5 Gecko
[Amended from comment #6]

(In reply to Ting-Yu Chou [:ting] from comment #5)

> Sorry I should've read this bug thoroughly. What I meant was for bug 1210440
> comment 19 since Music NGA creates more JS which for sure takes longer on
> parsing/execution. For the issue here, I have the same question as bug
> 1210440 comment 24, you did the test by:
> 
>   v2.2 Camera + v2.5 Gaia + v2.5 Gecko, or
>   v2.2 Camera + v2.2 Gaia + v2.5 Gecko

As Nicolas's comment #1, if JS benchmark doesn't change too much, it explains js engine is NOT critical part. 

I also attached chart in bug 1210849, comment 9 to point out potential issue. I found increasing trend in camera since Aug. So we need to isolate problem between v2.5 Gaia and v2.5 Gecko.
(In reply to Bobby Chien [:bchien] from comment #7)
> [Amended from comment #6]
> 
> (In reply to Ting-Yu Chou [:ting] from comment #5)
> 
> > Sorry I should've read this bug thoroughly. What I meant was for bug 1210440
> > comment 19 since Music NGA creates more JS which for sure takes longer on
> > parsing/execution. For the issue here, I have the same question as bug
> > 1210440 comment 24, you did the test by:
> > 
> >   v2.2 Camera + v2.5 Gaia + v2.5 Gecko, or
> >   v2.2 Camera + v2.2 Gaia + v2.5 Gecko
> 
> As Nicolas's comment #1, if JS benchmark doesn't change too much, it
> explains js engine is NOT critical part. 
> 
> I also attached chart in bug 1210849, comment 9 to point out potential
> issue. I found increasing trend in camera since Aug. So we need to isolate
> problem between v2.5 Gaia and v2.5 Gecko.

Bobby, in this bug I believe already isolated the problem to 2.5 Gecko by running the 2.2 camera application on 2.5 Gecko (as per the description and the profiler results I collected and attached above). It slowed down. If I run the 2.5 camera app on 2.2, launch time speeds up. Russ confirmed the same except with the video app: https://groups.google.com/forum/#!msg/mozilla.dev.fxos/HeGci2jormU/aDXWXDe5BAAJ.

I agree there are optimizations we can do in the app and if you look at the latest raptor results, you will see we are making strides there. If I run the optimized 2.5 app on 2.2 gecko, it still launches faster than on 2.5 gecko.
So as it turns out, this does not seem to be a Javascript engine problem, although it is sort of related which is why it *looked* like it. The optimizer in the gaia build process is no longer minifying files.

Bisecting on gaia and going line by line showed that this change is what broke it for the camera app:

https://github.com/mozilla-b2g/gaia/commit/dddca9ffc5dfad2f074099b3ca27f5ba353a3f2b?diff=split#diff-0204b25273d129bbd0aad7c979eb69c4R235

It appears the optimizer does not support the new "fat arrow" syntax and chokes. There appears to be other syntaxes it doesn't like since removing all of the fat arrow use doesn't solve the problem. I have yet to identify them.
Component: JavaScript Engine → Gaia::Build
Product: Core → Firefox OS
To elaborate further on comment 9, the fat arrow syntax has made its way into shared JavaScript code. I suspect this is how it has propagated into many/all the apps and make it look like a platform regression.
Thanks for chasing that Andrew. That's pretty ridiculous that we didn't notice minification failing :(
Full support does not appear to be present in uglify2 but what they have might be enough if we are willing to use the beta release. See https://github.com/mishoo/UglifyJS2/issues/448.

A comment on that github issue suggests we could use babel.io as a workaround to convert the syntax to something the formal releases support. See https://github.com/mishoo/UglifyJS2/issues/448#issuecomment-125928374.
Flags: needinfo?(aosmond)
(In reply to [:fabrice] Fabrice Desré from comment #11)
> Thanks for chasing that Andrew. That's pretty ridiculous that we didn't
> notice minification failing :(

Fabrice, well, it doesn't affect *every* file :). And digging further, while it appears arrow functions kicked it off in camera, it doesn't seem to be consistent on master today. It is possible this was a problem and got fixed and something else is going wrong related to minification.

For example, build_stage/camera/js/main.js doesn't get minified -- this is a huge file for us, by far the biggest, and I'm certain it is one of the major reasons why *camera* does badly.

Looking elsewhere, I see build_stage/music/js/nfc_share.js does not minify. Very small, simple file and it uses arrow functions. But build_stage/music/js/db.js *does* get minified and it uses arrow functions. No idea what happened there! If I change nfc_share.js, I can't get it to minify for the life of me, even with make clean, even though a standalone uglify command seems to work.

I may have also got overexcited. Why does the 2.2 app perform worse on master, and master app on 2.2 perform better? Are there shared files not bundled into the app? We may be dealing with multiple, entirely unrelated points of regression.

At the very least, I don't think it is specific to my environment. I've confirmed the camera and music application.zip from a prebuilt flame-kk image are consistent with my local build results thus far.
From comment 9, the generated main.js used to minify. I don't know what changed when but if I move the inclusion into the <head></head> tags (like it should have been from the build files), it minifies it now.

What does not minify and remains yet a mystery:

js/controllers/preview-gallery.js
shared/js/blobview.js
shared/js/media_frame.js
shared/js/video_player.js

These are all lazy loaded by the app code. However all other lazy loaded files are minified, including other controllers, views, libs, shared files, etc.
Updated to the latest gaia master as I forgot to merge in my latest patch on Friday. Now js/controllers/preview-gallery.js is minified but the new controller js/controllers/media.js is not. Moving the <script> tag for js/main.js to the <head> tag and removing the one instance of fat arrows in js/controllers/media.js caused it to be minified by the build.

Only remaining unminified files are the three shared js files in comment 14.
In comment 15, I should clarify that media.js was composed of a spun off bit from preview-gallery.js. I guess that's why preview-gallery started being minified -- the problematic code was moved into a different file and it made the build happy.
So, should we update the title of this bug to - "uglifyjs fails to minify some JS" and update STR to which files we know don't minify in 2.5 and did minify in 2.2?

Babel.io + Uglify may be the long term solution since we will be in a permanent state of using es6/es7 syntax that Gecko supports, and Uglifyjs will always be behind.
Babel seems to be supporting things roughly at the same time or earlier than Gecko, so it would give us a good chance that we can transpile our code to something that uglifyjs can ready.

The downside of that is that babel will transpile to es5 which may be significantly slower than native es6, so it would be nice to maintain a good list of transformers in babel so that we really only transform stuff that uglify cannot handle.

And of course, using uglify harmony would be better because it means that a lot of syntax that we use in Gaia would not have to be transformed to es5 polyfills (which would be slower).

tl;dr - I'm voting for babel.io -> uglify2(harmony).
Maybe you could switch to something other than uglify ? For example JSMin still works well in the SMS app (we don't minify lazy loaded files though).
(In reply to Julien Wajsberg [:julienw] from comment #18)
> Maybe you could switch to something other than uglify ? For example JSMin
> still works well in the SMS app (we don't minify lazy loaded files though).

Yeah, that's what I'm thinking on switching the app to.
Assignee: nobody → aosmond
Component: Gaia::Build → Gaia::Camera
jsmin doesn't seem to support es6 and may be unsafe: https://code.google.com/p/v8/issues/detail?id=4064
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20)
> jsmin doesn't seem to support es6 and may be unsafe:
> https://code.google.com/p/v8/issues/detail?id=4064

Since most apps seem to be relying upon jsmin (camera being one of the exceptions), that is alarming.
> js/controllers/preview-gallery.js
> shared/js/blobview.js
> shared/js/media_frame.js
> shared/js/video_player.js

I was able to use UglifyJS2 harmony branch to minify three out of those files. blobview cannot be minified because uglifyjs2 doesn't recognize default function params.

After playing with it, I believe we should just switch to uglifyjs2 now for minification of all apps, and help it gain full es6 syntax coverage.
Are you sure we're talking about the same jsmin here ?

The JSMin we're using is inherently safe because it doesn't try to be clever, merely removing comments, some CR and LF and spaces [1]. Especially we don't rename anything.

Moreover our build is looking whether we have the same AST before and after JSMin (I think in tests, not sure). [2]

[1] https://github.com/twolfson/node-jsmin2
[2] https://github.com/mozilla-b2g/gaia/blob/8aa0b41b5dd20755d9bc45fdb2ef43e25e0d0724/build/webapp-optimize.js#L348-L352
Gandalf, I'm quite against using uglifyjs. It takes significantly longer to minify than JSMin and from my previous tests does not bring noticeable improvements in performance.
No, I guess I meant the original jsmin. 

> It takes significantly longer to minify than JSMin and from my previous tests does not bring noticeable improvements in performance.

I'm willing to give it a try. I got uglifyjs2 with harmony branch and your jsmin and will test the three scenarios against a couple apps and will post here.
Flags: needinfo?(hkoka)
I played with that, but because so much of camera is lazy loaded, there's very little impact of GAIA_OPTIMIZE on Camera app (less than 20ms on Flame).

I reviewed more apps and wrote a proposal that could help us get more out of minification: https://groups.google.com/d/msg/mozilla.dev.fxos/iaU1OphVQDw/xvWDcnRiCwAJ
On the part about how to minify ES6 code while using r.js for combining modules:

Email uses some new ES features, like fat arrow, and uses esomin, which is in the gaia tree, to do the minification. It just removes comments, keeps white space, no mangling. For email, we prefer that since we can get intelligible line numbers if something does go wrong. It uses esprima and escodgen by default, but you could pass it a function that calls out to jsmin instead.

So if using r.js to combine modules, I suggest using esomin with the default esprima/escodgen path or with jsmin, if jsmin fits your needs. uglify does not seem to be keeping up with ES6 changes.

esomin in tree:
https://github.com/mozilla-b2g/gaia/blob/7b18598668d1285ae10f15b8e9a06d8ee4c32e84/build/esomin.js

This shows how email uses it in its build:
https://github.com/mozilla-b2g/gaia/blob/7b18598668d1285ae10f15b8e9a06d8ee4c32e84/apps/email/build/build.js#L6
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #26)
> I played with that, but because so much of camera is lazy loaded, there's
> very little impact of GAIA_OPTIMIZE on Camera app (less than 20ms on Flame).
> 
> I reviewed more apps and wrote a proposal that could help us get more out of
> minification:
> https://groups.google.com/d/msg/mozilla.dev.fxos/iaU1OphVQDw/xvWDcnRiCwAJ

See also bug 1215513 I filed recently, with a POC patch which fails with most apps ;)
Attachment #8680208 - Flags: review?(jdarcangelo)
Attachment #8680208 - Flags: review?(jdarcangelo) → review+
I have a branch that does what I suggested in the group - https://github.com/zbraniecki/gaia/tree/1218988-js-bundling-groups (bug 1218988)

It uses jsmin for now, and it allows for bundling JS files into groups for lazy loading and minifies the whole group together. What's the reason you decided to use eosmin instead of jsmin here?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #30)
> I have a branch that does what I suggested in the group -
> https://github.com/zbraniecki/gaia/tree/1218988-js-bundling-groups (bug
> 1218988)
> 
> It uses jsmin for now, and it allows for bundling JS files into groups for
> lazy loading and minifies the whole group together. What's the reason you
> decided to use eosmin instead of jsmin here?

Discussed offline, the performance advantage between eosmin and jsmin was minimal given we already lazy load so much on demand and there is already no perceptible cost to that lazy loading given how long camera operations already take. Eosmin preserves the ability to debug JS errors by line number with reloading, and there are no concerns regarding ES6 safety. We'll switch camera over to whatever unified system is ready post 2.5.
Blocks: 1210849
Summary: [B2G][Flame] Lazy loading JavaScript has regressed from 2.2 with same app → Camera app is not minified due to use of ES6 features
https://github.com/mozilla-b2g/gaia/commit/4ecb77aedf49df8ae4a293bd59b2569be683ab87
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: needinfo?(kvijayan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: