840627 - Icons in the settings app takes 200ms in the startup time

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I have been told that just changing the sprite should help there.

Comment 2

[Etienne Segonzac (:etienne)]

I should have a patch later today.

Comment 3

[Etienne Segonzac (:etienne)]

Attachment: WIP

WIP - it's consistently shaving 180ms off startup time and I can't see any visual degradation (tested on unagi and otoro).

(apply with git am)

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 713371 [details] [diff] [review]
WIP

Have you tried to keep the same-moz-image-rect rule so you will be able to simply switch the rule on a :active?

Also you are sure that the delay is related to the alpha channel? If not I would like to see if we can keep use one format for all apps. (in the worst case maybe something can be done in gecko).

Comment 5

[Gabriele Svelto [:gsvelto]]

FYI we noticed in other bugs that re-compressing existing assets can often speed up their load time significantly. As a quick test I've tried re-compressing the settings assets using the tools/png_recompress.sh script in gaia and obtained files which were 1/3 of their original size and which yielded an overall startup time reduction of ~90ms.

So please make sure you re-compress all new PNG files before committing them; alternatively we might consider running re-compression as part of the build process (see the comments in bug 836377) or just create a ticket to manually re-compress everything just before release.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Gabriele Svelto [:gsvelto] from comment #5)
> FYI we noticed in other bugs that re-compressing existing assets can often
> speed up their load time significantly. As a quick test I've tried
> re-compressing the settings assets using the tools/png_recompress.sh script
> in gaia and obtained files which were 1/3 of their original size and which
> yielded an overall startup time reduction of ~90ms.
> 
> So please make sure you re-compress all new PNG files before committing
> them; alternatively we might consider running re-compression as part of the
> build process (see the comments in bug 836377) or just create a ticket to
> manually re-compress everything just before release.

I think this is related to alpha channel and not directly to the size of the image.

Comment 7

[Etienne Segonzac (:etienne)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6)
> (In reply to Gabriele Svelto [:gsvelto] from comment #5)
> > FYI we noticed in other bugs that re-compressing existing assets can often
> > speed up their load time significantly. As a quick test I've tried
> > re-compressing the settings assets using the tools/png_recompress.sh script
> > in gaia and obtained files which were 1/3 of their original size and which
> > yielded an overall startup time reduction of ~90ms.
> > 
> > So please make sure you re-compress all new PNG files before committing
> > them; alternatively we might consider running re-compression as part of the
> > build process (see the comments in bug 836377) or just create a ticket to
> > manually re-compress everything just before release.
> 
> I think this is related to alpha channel and not directly to the size of the
> image.

Yep, the size does matter but removing the alpha channel is what got us the biggest gain.

Comment 8

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

Here we go, cleaned up the CSS a bit and tried again with
- optimized pngs (with pngcrush)
- jpgs
- gifs

And gif wins! (I can get the same performance gain with mid-quality jpgs but the icons get "noisy").

And FTR I consistently see a >180ms startup time improvement with this patch.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 713934 [details] [diff] [review]
Patch proposal

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

\o/
|o/
/o/
/o|


Can you open a bug against Core::GFX to be sure it is tracked?

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/e324e749cfd7ccbf7b0345d3f171dba5c8c695e9

Comment 11

[Etienne Segonzac (:etienne)]

Was it voluntary to land the patch without the added performance test?

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 713934 [details] [diff] [review]
Patch proposal

Not a blocker, but IF we think this is low risk, we're approving for uplift to v1-train and v1.0.1. Also adding qawanted/verifyme to get extra eyes on these icon changes.

Comment 13

[Patryk Adamczyk [:patryk] UX]

Next time please don't modify graphics without letting visual design know. NO UX people are CC'd. Also I don't see any screenshots of before and after. 

If this looks bad, we'll need to revert it. I know we need to be as fast as Android on the same hardware, but that also means looking as good as Android.

Comment 14

[Patryk Adamczyk [:patryk] UX]

Next time please don't modify graphics without letting visual design know. NO UX people are CC'd. Also I don't see any screenshots of before and after. 

If this looks bad, we'll need to revert it. I know we need to be as fast as Android on the same hardware, but that also means looking as good as Android.

Comment 15

[Alex Keybl [:akeybl]]

Patryk is going to take a look at the changes on master before we uplift, to make sure we haven't taken any visual regressions.

Comment 16

[Etienne Segonzac (:etienne)]

(In reply to Patryk Adamczyk [:patryk] UX from comment #14)
> Next time please don't modify graphics without letting visual design know.
> NO UX people are CC'd.

You're right sorry about that.

> Also I don't see any screenshots of before and after. 

I'll add them right now.

Comment 17

[Etienne Segonzac (:etienne)]

Attachment: Screenshot before

Comment 18

[Etienne Segonzac (:etienne)]

Attachment: Screenshot after

Comment 19

[Patryk Adamczyk [:patryk] UX]

Hey Etienne, it looks like in the after screenshot all the icons moved. 1px down, 1px to the left. So they are not ideally spaced anymore.

Comment 20

[Etienne Segonzac (:etienne)]

Attachment: Followup patch proposal

Comment 21

[Etienne Segonzac (:etienne)]

Attachment: Screenshot after followup

Comment 22

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/b5ef5ba42d3f867138334e69037f1c569533a949

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We have a regression reported from this patch, please to be backing out :(

> Upon more careful inspection, I see a regression w/ this patch where tapping
> "Wi-Fi" after a cold-start of Settings doesn't work.  Sometimes I can get
> the "Wi-Fi" item to respond after a bit of Settings app
> scrolling/navigating.  No solid STR yet.

Comment 24

[Etienne Segonzac (:etienne)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #23)
> We have a regression reported from this patch, please to be backing out :(
> 

Are we sure about the regression window, this patch was only css... but we had a bunch of other more invasive patchs landed in the settings app...

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Apparently it was bug 840322.

Comment 26

[David Scravaglieri [:scravag]]

Perf must be nominated for tef+

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #25)
> Apparently it was bug 840322.

If the reason this caused regressions (or didn't cause them, if that's what comment 25 implies) is no longer present, let's get this perf win uplifted again to branches.

Comment 28

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

So if I'm reading this right, I should be able to compare the difference between MC build settings and nightly build settings.  To note : My camera only handles 30 frames per second.

Prepatch: v101 2/22 build
https://www.youtube.com/watch?v=jiYSvrdgKxE
2 seconds Frame 21 is when I touched the settings icon
4 seconds Frame 22 is when the settings comes up.

2 seconds and 1 frame total.

Postpatch: MC 2/22 build
https://www.youtube.com/watch?v=Qy7QpagXZdc
Frame 19 is when I touched the settings icon
2 seconds Frame 8 is when the settings comes up (brightness level changes)

1 second and 19 seconds total

12 frames *2 = .24 difference roughly which is equivalent to what this bug is trying to fix. ~200 ms.

I am unsure but I am finding that the settings app is having issues.  I am not sure if it is related to this bug as of yet.  bug 843846

Comment 29

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

based on https://bugzilla.mozilla.org/show_bug.cgi?id=840322#c43, I'm removing mvines.  I think the intent was to get info from mwu?

Comment 30

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Bug 843846 is unrelated due to regression range check on bug 843846.  Marking this as verified.

Comment 31

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train (squashed): aa103c97804cc24d9ff3a6b2cccba45c0d7c4525
v1.0.1: e601b5234f7ff71514225be12118234e26f2de62

Comment 32

[Anthony Ricaud (:rik)]

I've opened bug 840627 to track the slowness of alpha images.