Closed Bug 1129584 Opened 9 years ago Closed 9 years ago
improve startup time by initializing Media
DB as early as possible during startup
46 bytes, text/x-github-pull-request
|Details | Review|
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.
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
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.
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.
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
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.
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.
I'm going to try a third time here, creating a new patch that builds on the patch in bug 1129572.
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
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.
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
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
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.
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
Attachment #8559523 - Attachment is obsolete: true
Comment on attachment 8560795 [details] [review] [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.
Attachment #8560795 - Flags: review?(pdahiya)
Comment on attachment 8560795 [details] [review] [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!
Attachment #8560795 - Flags: review?(pdahiya) → review+
Add to bug 1110590 to track in further release.
As David's suggestion, nominated as v3.0? feature.
feature-b2g: --- → 3.0?
Review comments addressed. Ready for checkin.
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/dd359acf46e317a9864be9eaaa5311c1f119e986
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.