Bug 817115 comment 23 and one more potential win.

300ms win on display of all album art and (mostly) semantics preserving. The one semantic change was not loading the default background image until we know an album doesn't have its own album art. In my testing, those never really showed up sooner anyway, so we were wasting image decoding work. Have one more change that alters semantics slightly, but not sure if it's a win yet ...

Yeah, the second change gets us 1 second :).

Comment on attachment 714691 [details] [diff] [review] Use CSS to position and clip album art Review of attachment 714691 [details] [diff] [review]: ----------------------------------------------------------------- The code for this looks good, though I wouldn't mind seeing displayAlbumArt removed entirely. I still need to test it on the phone against music with album art, though. r+ (hopefully) forthcoming! ::: apps/music/js/utils.js @@ +4,5 @@ > var span = document.createElement('span'); > span.textContent = str; > > if (escapeQuotes) > + return span.innerHTML.replace(/"/g, '"').replace(/'/g, '''); //" I'm guessing this comment works around some editor's broken syntax highlighting? A word or two about that would be helpful, since it took me a few minutes to realize why this was here.

Interdiff of changes to do this. Sometimes I really love JavaScript.

This is a 1 second startup win, tracking for uplift.

Comment on attachment 714691 [details] [diff] [review] Use CSS to position and clip album art Review of attachment 714691 [details] [diff] [review]: ----------------------------------------------------------------- You'll need to update the other places that show album art to handle things in the new way as well. The player shows the album art tiled, and the artists and albums pages show just the top left corner. (This is just with the placeholder art, though I'm sure real art is affected too.)

The focus of this bug is on improving startup time for v1.0.1. We need to limit risk in the changes here. I agree we should optimize the other sites in a followup. This updated patch removes the changes to the default background sizing rules, since they don't affect the tiles anyway.

(In reply to Jim Porter (:squib) from comment #3) > Comment on attachment 714691 [details] [diff] [review] > Use CSS to position and clip album art > > Review of attachment 714691 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: apps/music/js/utils.js > @@ +4,5 @@ > > var span = document.createElement('span'); > > span.textContent = str; > > > > if (escapeQuotes) > > + return span.innerHTML.replace(/"/g, '"').replace(/'/g, '''); //" > > I'm guessing this comment works around some editor's broken syntax > highlighting? A word or two about that would be helpful, since it took me a > few minutes to realize why this was here. That's correct. Sorry, I missed this comment before I posted v2, have added it locally.

Comment on attachment 714701 [details] [diff] [review] Load visible tiles before hidden ones Dominic, if you get back in time to review this, please do :). Jim and I are filling in for you. Whoever gets to it first.

Comment on attachment 714706 [details] [diff] [review] Use CSS to position and clip album art, v2 (Whoever gets to this first.)

Comment on attachment 714706 [details] [diff] [review] Use CSS to position and clip album art, v2 r+, looks good to me.

Comment on attachment 714701 [details] [diff] [review] Load visible tiles before hidden ones r+, looks good to me.

Chris and Jim, thanks for filling in this bug for me, I really appreciated. I have combined those two patches that you attached and sent a new one to github, also added little fixes to keep the same UX and visual. Please check to see I am combining right or not, if there is no problem, I think this patch should be good to go, thanks.

Comment on attachment 715080 [details] Link to github (combined patch) https://github.com/mozilla-b2g/gaia/commit/3d14b28f958f4fe9735bdef5dfde7ba9c8e03f43 Relatively low-risk patch that makes a big difference in music startup time.

Comment on attachment 714706 [details] [diff] [review] Use CSS to position and clip album art, v2 Clearing out review requests from me, since this is checked in now.

Comment on attachment 715080 [details] Link to github (combined patch) We also love when you do JS :) I have been told that perf are important. Let's push this fix on v1.0.1 and v1-train.

v1-train@b9f0f11b265f23adabbc1391a53da757e5200e9d

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #16) > Comment on attachment 715080 [details] > Link to github (combined patch) > > I have been told that perf are important. Let's push this fix on v1.0.1 and > v1-train. This seems to have only landed on v1-train.

Go ahead with uplift, like with other perf fixes, if there are regressions detected before 2/28 we'll have to backout.

v1.0.1: 249ef79362a3e501f7a9f3a2952e8b84195bbf98

does not make sense to create a regression issue.

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

This is a performance improvement, so you would need to measure startup time of the music app (with a realistic music collection installed) on builds before and after this patch landed.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #23) > This is a performance improvement, so you would need to measure startup time > of the music app (with a realistic music collection installed) on builds > before and after this patch landed. Chris, Please see Excel document titled "Music App Load Times.xlsx" From the data you will see the app load times have improved from older builds, but also please note that the new v1.0.1 build had major issues in having the music app even load. Please let me know if you have any questions.

Document attachment title in Comment 24 is actually "Excel document with Music App Load Time Data".

As per comment #23 by comparing before the patch is landed and after, the performance of the loading is increased on start up. Before Patch: Unagi-V1.1 Load Time -1083ms Unagi -V1.0.1 Load Time -1071ms After the Patch for the latest build Inari V1.0.1 Load time -641ms Unagi V1.1 Load time -676ms Environmental Variables: Unagi Build ID: 20130603070207 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/4785b1353fd7 Gaia: 4de4354e3a99f151a834743c7b03a041ac8db12f Inari Build ID: 20130603070208 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/42555e1e72fa Gaia: fcae23654296c9cc645c2b7e77a2c36bf494803a