Closed Bug 840627 Opened 12 years ago Closed 12 years ago

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

Categories

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

defect
Not set
normal

Tracking

(blocking-b2g:tef+, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

VERIFIED FIXED
blocking-b2g tef+
Tracking Status
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: vingtetun, Assigned: etienne)

References

Details

(Whiteboard: [FFOS_perf])

Attachments

(4 files, 2 obsolete files)

I have been told that just changing the sprite should help there.
I should have a patch later today.
Attached patch WIP (obsolete) (deleted) — Splinter Review
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)
Assignee: nobody → etienne
Attachment #713371 - Flags: feedback?(kaze)
Attachment #713371 - Flags: feedback?(21)
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).
Attachment #713371 - Flags: feedback?(21)
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.
blocking-b2g: --- → tef?
(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.
(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.
Attached patch Patch proposal (deleted) — Splinter Review
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.
Attachment #713371 - Attachment is obsolete: true
Attachment #713371 - Flags: feedback?(kaze)
Attachment #713934 - Flags: review?(21)
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?
Attachment #713934 - Flags: review?(21) → review+
Was it voluntary to land the patch without the added performance test?
blocking-b2g: tef? → -
tracking-b2g18: --- → +
Keywords: qawanted, verifyme
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.
Attachment #713934 - Flags: approval-gaia-v1+
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.
blocking-b2g: - → tef?
tracking-b2g18: + → ---
Keywords: qawanted, verifyme
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.
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.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
Flags: needinfo?(padamczyk)
Keywords: qawanted, verifyme
(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.
Attached image Screenshot before (deleted) —
Attached image Screenshot after (obsolete) (deleted) —
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.
Flags: needinfo?(padamczyk)
Attached patch Followup patch proposal (deleted) — Splinter Review
Attachment #715135 - Flags: review?(21)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached image Screenshot after followup (deleted) —
Attachment #714444 - Attachment is obsolete: true
Attachment #715135 - Flags: review?(21)
Attachment #715135 - Flags: review+
Attachment #715135 - Flags: approval-gaia-v1+
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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...
Flags: needinfo?(mvines)
Apparently it was bug 840322.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Perf must be nominated for tef+
blocking-b2g: - → tef?
(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.
blocking-b2g: tef? → tef+
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
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?
Flags: needinfo?(mvines)
Bug 843846 is unrelated due to regression range check on bug 843846. Marking this as verified.
Status: RESOLVED → VERIFIED
Keywords: qawanted
Keywords: verifyme
v1-train (squashed): aa103c97804cc24d9ff3a6b2cccba45c0d7c4525 v1.0.1: e601b5234f7ff71514225be12118234e26f2de62
I've opened bug 840627 to track the slowness of alpha images.
Depends on: 944306
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: