Regression across all gaia apps on Sep 16 2014

RESOLVED FIXED in 2.1 S5 (26sep)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: zbraniecki, Assigned: aus)

Tracking

({perf, regression})

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [systemsfe][p=3])

Attachments

(5 attachments)

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
See Also: → 1069450
[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)
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.
Duplicate of this bug: 1073009
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)
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.
Oh yeah, that definitely seems to work :)

"title": "startup > moz-app-visually-complete" > "mozPerfDurationsAverage": 914.4007810666665
Attachment #8495615 - Flags: review?(etienne)
Attachment #8495615 - Flags: review?(alive)
Flags: needinfo?(etienne)
Flags: needinfo?(alive)
Perf test results with PR applied.
Summary: Regression accross all gaia apps on Sep 16 2014 → Regression across all gaia apps on Sep 16 2014
* 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 on attachment 8495615 [details] [review]
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 on attachment 8495615 [details] [review]
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 on attachment 8495615 [details] [review]
Pull Request - Never take screenshot of closing AppWindow if part of homescreen

I meant f+ :)
Attachment #8495615 - Flags: review+ → feedback+
Comment on attachment 8495615 [details] [review]
Pull Request - Never take screenshot of closing AppWindow if part of homescreen

Now with tests and tests and goodness!
Attachment #8495615 - Flags: review?(etienne)
Target Milestone: 2.1 S3 (29aug) → 2.1 S6 (10oct)
Comment on attachment 8495615 [details] [review]
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+
Depends on: 1062136
Comment on attachment 8495615 [details] [review]
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)
Commit (master): https://github.com/mozilla-b2g/gaia/commit/2834baf4c7e34fe6ef335f0469f6d0f593c5922b

Fixed!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [systemsfe] → [systemsfe][p=3]
Target Milestone: 2.1 S6 (10oct) → 2.1 S5 (26sep)
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.
[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?
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 :)
big performance regression
blocking-b2g: 2.1? → 2.1+
Attachment #8495615 - Flags: approval-gaia-v2.1?(fabrice) → approval-gaia-v2.1+
Comment on attachment 8495615 [details] [review]
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+
blocking-b2g: 2.1+ → 2.0+
Posted file Pull Request - v2.1
Posted file Pull Request - v2.0
Please provide the bug reproduce steps and videos, which could help with our verification. thanks!
Flags: needinfo?(jocheng)
Flags: needinfo?(jocheng) → needinfo?(hlu)
(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.