Closed
Bug 1069452
Opened 10 years ago
Closed 10 years ago
Regression across all gaia apps on Sep 16 2014
Categories
(Firefox OS Graveyard :: Performance, defect)
Tracking
(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)
People
(Reporter: zbraniecki, Assigned: aus)
References
Details
(Keywords: perf, regression, Whiteboard: [systemsfe][p=3])
Attachments
(5 files)
(deleted),
text/x-github-pull-request
|
etienne
:
review+
alive
:
feedback+
etienne
:
feedback+
bajaj
:
approval-gaia-v2.0+
fabrice
:
approval-gaia-v2.1+
|
Details |
(deleted),
application/json
|
Details | |
(deleted),
application/json
|
Details | |
(deleted),
text/x-github-pull-request
|
Details | |
(deleted),
text/x-github-pull-request
|
Details |
There's a regression in range of 100ms on Sept 16th 2014.
Read more here: https://groups.google.com/d/msg/mozilla.dev.gaia/vstguNLgDek/bDPqJVZrkucJ
Reporter | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
[Blocking Requested - why for this release]:
I think this particular regression is mine and was caused by bug 1044125. Taking.
Assignee: nobody → aus
Status: NEW → ASSIGNED
Keywords: perf,
regression
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Whiteboard: [systemsfe]
Target Milestone: --- → 2.1 S3 (29aug)
Assignee | ||
Comment 2•10 years ago
|
||
Here's my current thinking on what's going on:
It's possible that we're no longer preventing _setVisible from being called multiple times. Before we essentially ended up with a guard against this by checking screenshotOverlayState in setVisible, showFrame and hideFrame. I think it would be possible to add similar checks back in based on checking frame visibility.
Going to try and whip something up that prevents multiple calls that would toggle visibility to the same state.
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → affected
Assignee | ||
Comment 4•10 years ago
|
||
An update for everyone:
I added guards in _showFrame and _hideFrame in AppWindow so that we don't call _setVisible more than we should and that seems to get us ~30ms gain in perf. Still short of the 100ms regression.
I then removed our getScreenshot() call from the _closed handler in AppWindow and that get's us back down to (and then some) by saving ~90ms.
At this point I believe that we'll have to find a better moment to take the screenshot, or, change the way we run perf tests for applications. The cold start-up time is being influenced by the previous test run and that shouldn't happen. But then again, we switch applications all the time now so, this should be something that is as fast as possible.
These tests were done against the Contacts app on a Flame w/319M profile (v180 Firmware, latest Gaia + Gecko).
Etienne, Alive, I'm open to suggestions as to a better moment to update our screenshot. I think if we were to mimic the behavior we had before, we would want it to get in _hideFrame instead of _closed. I'm unsure if that will be terrible for transitions however.
Flags: needinfo?(etienne)
Flags: needinfo?(alive)
Assignee | ||
Comment 5•10 years ago
|
||
It looks like this could be related to the fact that there was also a guard against taking a screenshot when the bottom most window is the homescreen in setVisible. I'm trying implement a similar guard in our _closed handler in the AppWindow.
Assignee | ||
Comment 6•10 years ago
|
||
Oh yeah, that definitely seems to work :)
"title": "startup > moz-app-visually-complete" > "mozPerfDurationsAverage": 914.4007810666665
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8495615 -
Flags: review?(etienne)
Attachment #8495615 -
Flags: review?(alive)
Flags: needinfo?(etienne)
Flags: needinfo?(alive)
Assignee | ||
Comment 8•10 years ago
|
||
Perf test results with PR applied.
Assignee | ||
Comment 9•10 years ago
|
||
Summary: Regression accross all gaia apps on Sep 16 2014 → Regression across all gaia apps on Sep 16 2014
Comment 10•10 years ago
|
||
* Why we didn't take screenshot of homescreen is because of the lack of getScreenshot API on mozbrowser. It didn't support PNG before; and homescreen used to be transparent background before. Tim had already provided a patch to gecko, but we didn't change gaia side for a long time because there is no requirement before.
* I want to reflect the API change in gaia side(https://bugzilla.mozilla.org/show_bug.cgi?id=1072781), but it seems we need to find a proper timing for homescreen to take screenshot if we really it. Otherwise, taking screenshot while homescreen enters closed state will slow down the loading of current loading app.
* Maybe we could delay the take screenshot by querying System.isBusyLoading() for homescreen.
Comment 11•10 years ago
|
||
Comment on attachment 8495615 [details]
Pull Request - Never take screenshot of closing AppWindow if part of homescreen
LGTM, see if etienne has some opinions; if not then we could go with unit tests.
Attachment #8495615 -
Flags: review?(alive) → feedback+
Comment 12•10 years ago
|
||
Comment on attachment 8495615 [details]
Pull Request - Never take screenshot of closing AppWindow if part of homescreen
yep, let's do this, with tests :)
Attachment #8495615 -
Flags: review?(etienne) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8495615 [details]
Pull Request - Never take screenshot of closing AppWindow if part of homescreen
I meant f+ :)
Attachment #8495615 -
Flags: review+ → feedback+
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8495615 [details]
Pull Request - Never take screenshot of closing AppWindow if part of homescreen
Now with tests and tests and goodness!
Attachment #8495615 -
Flags: review?(etienne)
Assignee | ||
Updated•10 years ago
|
Target Milestone: 2.1 S3 (29aug) → 2.1 S6 (10oct)
Comment 15•10 years ago
|
||
Comment on attachment 8495615 [details]
Pull Request - Never take screenshot of closing AppWindow if part of homescreen
a small comment on one test but r=me with this explained/addressed
Attachment #8495615 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8495615 [details]
Pull Request - Never take screenshot of closing AppWindow if part of homescreen
-- Request for 2.0 Approval --
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.
[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Landing of bug 1062136 on 2.0 (patch partially based on 1044125 which has a known performance regression addressed by this bug)
[User impact] if declined: ~80-100ms slow down on cold app launch times.
[Testing completed]: On Flame Device (319M) running RUNS=30 APP=communications/contacts make test-perf + Manual testing on same device.
[Risk to taking this patch] (and alternatives if risky): Actually, this is a very low risk performance fix, I'm very confident about it.
[String changes made]: None.
-- Request for v2.1 Approval --
[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Landing of bug 1044125 (Memory Footprint Reduction)
[User impact] if declined: ~80-100ms slowdown on cold app start-up.
[Testing completed]: On Flame Device (319M) running RUNS=30 APP=communications/contacts make test-perf + Manual testing on same device.
[Risk to taking this patch] (and alternatives if risky): Actually, this is a very low risk performance fix, I'm very confident about it.
[String changes made]: None.
Attachment #8495615 -
Flags: approval-gaia-v2.1?(fabrice)
Attachment #8495615 -
Flags: approval-gaia-v2.0?(bbajaj)
Assignee | ||
Comment 17•10 years ago
|
||
Commit (master): https://github.com/mozilla-b2g/gaia/commit/2834baf4c7e34fe6ef335f0469f6d0f593c5922b
Fixed!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-v2.0:
--- → affected
status-b2g-v2.0M:
--- → affected
Resolution: --- → FIXED
Whiteboard: [systemsfe] → [systemsfe][p=3]
Target Milestone: 2.1 S6 (10oct) → 2.1 S5 (26sep)
Assignee | ||
Comment 18•10 years ago
|
||
Datazilla is showing all cold load times to be trending down ~100ms since this fix landed.
https://datazilla.mozilla.org/b2g/?branch=master&device=flame-319MB&range=30&test=startup_%3E_moz-app-visually-complete&app_list=communications/contacts&app=communications/contacts&gaia_rev=0d38e2046b38aba0&gecko_rev=9b9e61eb8505&plot=avg&err_bars=1
I would like to land this on 2.1 as soon as possible to get a few days of perf test data available by Friday.
Comment 19•10 years ago
|
||
[Blocking Requested - why for this release]:
+1 to 2.1 uplift. datazilla on 2.1 also shows a slow increase of ~100ms in 30 days so it would be good to get this tested there.
https://datazilla.mozilla.org/b2g/?branch=v2.1&device=flame-319MB&range=30&test=startup_%3E_moz-app-visually-complete&app_list=calendar,camera,clock,communications/contacts,communications/dialer,costcontrol,email%20FTU,fm,gallery,music,settings,sms,video&app=communications/contacts&gaia_rev=59e5c2467b7b8219&gecko_rev=1d4b0fc511dc&plot=avg
Also, calendar app has gone missing, and i filed bug 1067625 to track that.
blocking-b2g: --- → 2.1?
Reporter | ||
Comment 20•10 years ago
|
||
hey guys, this is awesome! Thanks so much for tracking it down :) If you have any clue on what might have caused another 100ms regression one day earlier (bug 1069450), please, help me :)
Updated•10 years ago
|
Attachment #8495615 -
Flags: approval-gaia-v2.1?(fabrice) → approval-gaia-v2.1+
Comment 22•10 years ago
|
||
Comment on attachment 8495615 [details]
Pull Request - Never take screenshot of closing AppWindow if part of homescreen
Given this is a fallout from 2.0 and there are proven performace improvements with this patch , I am approving this on 2.0 as well. If there are any fallouts lets backout immediately.
Attachment #8495615 -
Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
Updated•10 years ago
|
blocking-b2g: 2.1+ → 2.0+
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
Comment 25•10 years ago
|
||
Comment 26•10 years ago
|
||
Comment 27•10 years ago
|
||
Please provide the bug reproduce steps and videos, which could help with our verification. thanks!
Flags: needinfo?(jocheng)
Updated•10 years ago
|
Flags: needinfo?(jocheng) → needinfo?(hlu)
Comment 28•10 years ago
|
||
(In reply to Shine from comment #27)
> Please provide the bug reproduce steps and videos, which could help with our
> verification. thanks!
Hi Shine,
This is perf issue, and it could be tracking on datazilla. Please see comment 18 and comment 19.
Flags: needinfo?(hlu)
You need to log in
before you can comment on or make changes to this bug.
Description
•