David Flanagan [:djf] Assignee
	Description • 10 years ago

The gallery app currently puts most of its initial JS code into one big deferred script.

This bug will change that to create a small non-deferred script that runs earlier in the startup process for the purpose of calling the MediaDB() constructor so that the MediaDB class can be opening the indexeddb while the rest of the app is loading.

In testing, this gives significant startup time improvement on a quad core nexus 5. I suspect that it will not make much difference on a dual core device like Flame, but will need to verify that before landing anything.

	David Flanagan [:djf] Assignee
	Comment 1 • 10 years ago

Baseline startup time is 1154ms on my Flame. 

Detailed numbers (copied from bug 1129568):

STARTUP (ms) t: 1229 t̄: 1154 σ: 59 n: 41 median: 1148 min: 1047 max: 1255

	David Flanagan [:djf] Assignee
	Comment 2 • 10 years ago

I've got a work in progress patch for this bug that is giving me startup times like these:

STARTUP (ms) t: 1087 t̄: 1114 σ: 27 n: 40 median: 1111 min: 1065 max: 1161

This patch actually also includes the fix from bug 1129563, so it would be better to compare these results to the results in that bug:

STARTUP (ms) t: 1112 t̄: 1134 σ: 57 n: 30 median: 1120 min: 1043 max: 1238

So this patch does seem to make a difference even on Flame.  I'd expect a bigger difference on quadcore, however.

Of all my startup time patches, this is the one with the biggest code change, and I want to keep experimenting with it to see if I can get something cleaner before putting up a PR for review.

	David Flanagan [:djf] Assignee
	Comment 4 • 10 years ago

I've gone ahead and attached my github pr. It is still a work in progress, however and is not ready for review or landing.

	David Flanagan [:djf] Assignee
	Comment 5 • 10 years ago

I've got a new version of the patch that I like better that has about the same performance impact on Flame

STARTUP (ms) t: 1094 t̄: 1117 σ: 29 n: 40 median: 1124 min: 1063 max: 1168

	David Flanagan [:djf] Assignee
	Comment 6 • 10 years ago

On the nexus 5 without this patch:

STARTUP (ms) t: 722 t̄: 800 σ: 129 n: 40 median: 719 min: 650 max: 1043 

With the patch:

STARTUP (ms) t: 669 t̄: 791 σ: 142 n: 40 median: 689 min: 649 max: 1037

I'm surprised to see that the patch doesn't make very much difference for average startup time. Though it does make a difference in median time.  The standard deviation in startup time is really high on this device, and I don't know why that is.

I'm hoping for synergistic effects between this patch and the one in bug 1029572. With both applied, maybe we'll get even better results.

	David Flanagan [:djf] Assignee
	Comment 7 • 10 years ago

I tried applying both patches and did not see any synergy:

STARTUP (ms) t: 711 t̄: 802 σ: 140 n: 40 median: 703 min: 626 max: 1048

This is worse than without either patch applied.

The nexus 5 does print errors to the console every couple of seconds - there is stuff in our port that still isn't working right. Maybe that explains large variability in startup times.

These trials are all with the second version of the patch I've written for this bug. It is cleaner, but it does run more non-deferred code when the app first lauches. Perhaps it is too heavy to get a good effect. I should also test the lighter but uglier version of the patch on nexus 5.

	David Flanagan [:djf] Assignee
	Comment 8 • 10 years ago

I'm going to try a third time here, creating a new patch that builds on the patch in bug 1129572.

	David Flanagan [:djf] Assignee
	Comment 9 • 10 years ago

Since this new patch is built on top of the patch in bug 1129572, the timing data from that bug will be our baseline for this bug:

nexus5 8s interval: STARTUP (ms) t: 467 t̄: 511 σ: 66 n: 100 median: 491 min: 407 max: 669 outliers: 5
Flame  5s interval: STARTUP (ms) t: 820 t̄: 978 σ: 150 n: 45 median: 908 min: 820 max: 1286 outliers: 1

	David Flanagan [:djf] Assignee
	Comment 10 • 10 years ago

With the patch I get:

nexus5: STARTUP (ms) t: 503 t̄: 513 σ: 42 n: 100 median: 505 min: 431 max: 619 outliers: 4
Flame:  STARTUP (ms) t: 892 t̄: 987 σ: 160 n: 39 median: 912 min: 783 max: 1257 outliers: 0

So again moving the critical startup code into non-deferred scripts to try to get thumbnails created sooner doesn't seem to improve anything.

	David Flanagan [:djf] Assignee
	Comment 11 • 10 years ago

If I keep the new restructured code with the critical MediaDB initialization stuff first, but defer it along with the rest of the JS code, then I do see a speedup. I guess breaking the JS into two files was not a good thing. I'm seeing a 30ms speedup on nexus 5

nexus5: STARTUP (ms) t: 462 t̄: 481 σ: 39 n: 100 median: 478 min: 393 max: 588 outliers: 3

	David Flanagan [:djf] Assignee
	Comment 12 • 10 years ago

I just pulled the application.zip file for gallery off of the nexus 5 and was very surprised to discover that none of the JS files are concatenated or minimized. So I've been trying to do performance testing on a non-production version of the app

	David Flanagan [:djf] Assignee
	Comment 13 • 10 years ago

Okay, so if I build with PRODUCTION=1 then I get the concatenated and minified JS, which is what I thought I was testing.  

If I go back to the patch from bug 1129572 and retest that I get this as the new baseline:

STARTUP (ms) t: 480 t̄: 450 σ: 33 n: 50 median: 448 min: 378 max: 559 outliers: 4

And if I test this patch with non-deferred scripts for the critical startup code, I get 1 63kb file of non-deferred JS and one 55kb file of deferred JS with these startup stats which are worse than baseline:

STARTUP (ms) t: 475 t̄: 472 σ: 35 n: 50 median: 474 min: 397 max: 547 outliers: 3

But if I use this patch and keep all the scripts deferred, I do see a speedup of 13ms or so:

STARTUP (ms) t: 431 t̄: 437 σ: 34 n: 50 median: 435 min: 376 max: 503 outliers: 1

Keeping everything deferred is clearly the way to go. This patch does give some speedup, but probably not enough to justify the uplift risk, since it is a pretty major restructuring of the application code.

	David Flanagan [:djf] Assignee
	Comment 14 • 10 years ago

Finally, I need to test the impact of combining the 1129568 (remove tablet media queries) patch with 1129572 and with this patch.

1129572 + 1129568: STARTUP (ms) t: 451 t̄: 448 σ: 35 n: 50 median: 453 min: 366 max: 514 outliers: 2
1129584 + 1129572 + 1129568: STARTUP (ms) t: 435 t̄: 439 σ: 38 n: 100 median: 438 min: 362 max: 510 outliers: 3

So basically, the patch in bug 1129568 makes no difference in startup time in production builds

	David Flanagan [:djf] Assignee
	Comment 16 • 10 years ago

Comment on attachment 8560795 [details]
[PullReq] davidflanagan:bug1129584.3 to mozilla-b2g:master

Punam,

This is a pretty big patch that moves the gallery app's initialization code around so that the app starts a little faster. In the process it also makes things a little more rational, and is probably a good step toward refactoring the startup process.

I'm not sure that the gain in startup speed is worth the risk of uplifting this to 2.2, but I do think that it is worth landing it on master just because it improves the code organization.

Note that there are two commits in the PR. The first one is just a duplicate of the patch in bug 1129572, which I do hope to uplift to 2.2. Jim is reviewing that one, so you only need to review the second commit.

	Punam Dahiya [:pdahiya]
	Comment 17 • 10 years ago

Comment on attachment 8560795 [details]
[PullReq] davidflanagan:bug1129584.3 to mozilla-b2g:master

Hi David
I reviewed and tested the patch and the code is much more logically organized in startup.js. It's definitely a good refactoring start.

I see start time improvement, should be because of using thumbnails.js where it creates thumbnails as soon as db can be enumerated. A nit noted in github, it's good to land. Thanks!

	Bobby Chien
	Comment 18 • 10 years ago

Add to bug 1110590 to track in further release.

	Bobby Chien
	Comment 19 • 10 years ago

As David's suggestion, nominated as v3.0? feature.

	David Flanagan [:djf] Assignee
	Comment 20 • 10 years ago

Review comments addressed. Ready for checkin.

	GitHub Autolander Bot
	Comment 21 • 10 years ago

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/dd359acf46e317a9864be9eaaa5311c1f119e986