941969 - optimize JS for system app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

system app is currently blacklisted from JS aggregation due to a bug that has been resolved in April.

https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-optimize.js#L38

Aggregating JS seems to shave 1mb from memory usage of the system app and when compared two phones next to each other it seems to shave ~1s from startup time (Keon, master).

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: bug941969.diff

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8336478 [details] [diff] [review]
bug941969.diff

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

It hasn't been fixed by bug 839574. That bug is only about blacklisting apps.
But anyway, it seems to work fine now on desktop, device and firefox.

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

Do we have something blocking this patch to be merged?

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request

PR, carrying over r+. I'll wait for tests and land it

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

I tested it on Flame, master, using make test-perf with 10 runs. It shaved 3mb of ram.

How far back can we backport it?

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Zibi Braniecki [:gandalf] from comment #5)
> I tested it on Flame, master, using make test-perf with 10 runs. It shaved
> 3mb of ram.
> 
> How far back can we backport it?

I recommend 2.0/2.1/2.2, but that's subject to release manager approval.

Comment 7

[[:fabrice] Fabrice Desré]

(In reply to Zibi Braniecki [:gandalf] from comment #5)
> I tested it on Flame, master, using make test-perf with 10 runs. It shaved
> 3mb of ram.
> 
> How far back can we backport it?

How did you measure that?

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> (In reply to Zibi Braniecki [:gandalf] from comment #5)
> > I tested it on Flame, master, using make test-perf with 10 runs. It shaved
> > 3mb of ram.
> > 
> > How far back can we backport it?
> 
> How did you measure that?

I used make test-perf on Settings app, and it shows memory for Settings and System app. Does those result have any value?

I also tested bootstrap time using a stopwatch, and on Flame it shaves a little bit under 1s (5 runs, very consistently).

I remember that on Keon it was above 1s (see comment 0)

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8504040 [details] [review]
pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): None
[User impact] if declined: Performance/Memory cost
[Testing completed]: None
[Risk to taking this patch] (and alternatives if risky): Low, because any regressions will be very visible (all System JS is minified) and reverting is a one liner
[String changes made]: None

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Landed on master:
Commit: https://github.com/mozilla-b2g/gaia/commit/66e26679d4ecd7b1d0a013555e3b33871354046d
Merge: https://github.com/mozilla-b2g/gaia/commit/de254419f3553f48187d003ee8e38034b429f069

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

I also tested performance impact using the latest system test-perf (bug 1070615).

(SAMPLE=8)

startup > mozSystemLoadEnd    - 2857 -> 2538
startup > mozOsLogoEnd        - 15551 -> 15255
startup > mozHomescreenStart  - 14343 -> 13986

All results are statistically significant (p-value < 0.05)

So, it seems that on Flame we have ~300ms improvement. I expect a more significant one on lower-end phones.

Comment 12

[[:fabrice] Fabrice Desré]

(In reply to Zibi Braniecki [:gandalf] from comment #8)
> (In reply to Fabrice Desré [:fabrice] from comment #7)
> > (In reply to Zibi Braniecki [:gandalf] from comment #5)
> > > I tested it on Flame, master, using make test-perf with 10 runs. It shaved
> > > 3mb of ram.
> > > 
> > > How far back can we backport it?
> > 
> > How did you measure that?
> 
> I used make test-perf on Settings app, and it shows memory for Settings and
> System app. Does those result have any value?
> 
> I also tested bootstrap time using a stopwatch, and on Flame it shaves a
> little bit under 1s (5 runs, very consistently).
> 
> I remember that on Keon it was above 1s (see comment 0)

I don't see improvements in datazilla for now: https://datazilla.mozilla.org/b2g/?branch=master&device=flame-319MB&range=7&test=settings_memory&app_list=calendar,camera,clock,communications/contacts,communications/dialer,costcontrol,email%20FTU,fm,gallery,music,settings,sms,system_rss,video&app=settings&gaia_rev=1a85a538cb0a860b&gecko_rev=7090d5b4ed1e&plot=avg

Comment 13

[bhavana bajaj [:bajaj]]

(In reply to Fabrice Desré [:fabrice] from comment #12)
> (In reply to Zibi Braniecki [:gandalf] from comment #8)
> > (In reply to Fabrice Desré [:fabrice] from comment #7)
> > > (In reply to Zibi Braniecki [:gandalf] from comment #5)
> > > > I tested it on Flame, master, using make test-perf with 10 runs. It shaved
> > > > 3mb of ram.
> > > > 
> > > > How far back can we backport it?
> > > 
> > > How did you measure that?
> > 
> > I used make test-perf on Settings app, and it shows memory for Settings and
> > System app. Does those result have any value?
> > 
> > I also tested bootstrap time using a stopwatch, and on Flame it shaves a
> > little bit under 1s (5 runs, very consistently).
> > 
> > I remember that on Keon it was above 1s (see comment 0)
> 
> I don't see improvements in datazilla for now:
> https://datazilla.mozilla.org/b2g/?branch=master&device=flame-
> 319MB&range=7&test=settings_memory&app_list=calendar,camera,clock,
> communications/contacts,communications/dialer,costcontrol,email%20FTU,fm,
> gallery,music,settings,sms,system_rss,
> video&app=settings&gaia_rev=1a85a538cb0a860b&gecko_rev=7090d5b4ed1e&plot=avg

:gandalf, any comments here? Unless there is proven improvements, this is better riding the trains and getting fixed in 2.2 than being uplifted. Not sure if there is a reason on missing the improvements in datazilla, so please let us know...

Comment 14

[[:fabrice] Fabrice Desré]

This patch is breaking b2g desktop production builds, as well as flatfish ones. We need to backout.

Comment 15

[(Inactive after June) George Duan [:gduan] [:喬智]]

If scripts are merged into one, then we should try to fix original error from each script, or it may probably break entire app.

I tried to do below items with your patch and it works fine with GAIA_OPTIMIZE=1 on b2g-desktop,

1. Change loading order of bootstrap.js and its requiring script , bootstrap.js should put on the bottom. 
2. fix error for call_forwarding.js/icc.js/nfc_handover_manager.js which may have error on device without navigator.mozIccManager , mozMobileConnections ... etc.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

I had a work week and didn't manage to look into this. It's a one-liner so backing out should be trivial.

I can back it out or we can fix the real bug as :gduan suggests.

Let me know and I'll do this first thing in the morning.

Keeping NI to verify the memory/perf wins.

Comment 17

[:gerard-majax]

(In reply to Fabrice Desré [:fabrice] from comment #14)
> This patch is breaking b2g desktop production builds, as well as flatfish
> ones. We need to backout.

That is also breaking Mulet, as documented in bug 1082001 comment 25.

Comment 18

[Julien Wajsberg [:julienw]]

(In reply to Zibi Braniecki [:gandalf] from comment #4)
> Created attachment 8504040 [details] [review]
> pull request
> 
> PR, carrying over r+. I'll wait for tests and land it

Do we know why this has not been caught by Gaia Try? Are "B2G Desktop Opt" not the same than "B2G Desktop Production Builds"?

Comment 19

[(Inactive after June) George Duan [:gduan] [:喬智]]

I guess tbpl doesn't run marionette with GAIA_OPTIMIZE=1 or production.


10:25:11     INFO - Running command: ['make', 'test-integration', 'NPM_REGISTRY=http://npm-mirror.pub.build.mozilla.org', 'REPORTER=mocha-tbpl-reporter', 'TEST_MANIFEST=./shared/test/integration/tbpl-manifest.json'] in /builds/slave/test/gaia

Comment 20

[Kevin Grandon :kgrandon]

Backed out for comment 14, and being asked to do so over IRC. I guess we should implement the suggestions in comment 15.

We should also see if we can run marionette tests with a GAIA_OPTIMIZE=1 profile.

https://github.com/mozilla-b2g/gaia/commit/27a1d1baaa8e375b70e043efee67d5f2206c330b

Comment 21

[Julien Wajsberg [:julienw]]

I'd say even PRODUCTION=1, for all marionette tests.

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

Now that this patch is backed out, can we file follow up bugs?

Comment 23

[Kevin Grandon :kgrandon]

Comment on attachment 8504040 [details] [review]
pull request

Clearing approvals until it sticks in master.

Comment 25

[(Inactive after June) George Duan [:gduan] [:喬智]]

Steal it for bug 1094339.

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Julien Wajsberg [:julienw] from comment #21)
> I'd say even PRODUCTION=1, for all marionette tests.

We can't do PRODUCTION=1. Some apps needed for tests would not be built in with that config.
Unless we remote these test apps and have them dynamically installed in the marionette tests itself.

Comment 27

[Kevin Grandon :kgrandon]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #26)
> We can't do PRODUCTION=1. Some apps needed for tests would not be built in
> with that config.
> Unless we remote these test apps and have them dynamically installed in the
> marionette tests itself.

I would personally prefer to "install" these along with the apps property when we create the client. This should allow for smaller profile sizes and faster flash times for developers. For example:  https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/marionette/software_home_callscreen_test.js#L20

This should definitely be a follow-up though.

Comment 28

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

Comment 29

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8519690 [details] [review]
PR to master

Hi Alive,
this patch works fine on device and b2g-desktop with GAIA_OPTIMIZE=1, please kindly check.

Comment 30

[(Inactive after June) George Duan [:gduan] [:喬智]]

master: https://github.com/mozilla-b2g/gaia/commit/e632da1eebc3edefdd8947e977f1d386c0d4c794