1010604 - Multiple applications regressing on cold launch May 14, 2014

Status: RESOLVED DUPLICATE of bug 950673

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect, P1)

Description

[:Eli Perelman]

Multiple applications showing regression in cold launch time on Datazilla, many between 200 and 500ms. Bisecting using a Flame as I do not have a hamachi.

Gecko last-known-good: 6df3da8
Gaia last-known-good: 461ea9a

Gecko first-known-bad: d6d77be
Gaia first-known-bad: 70953f7

-------
Running b2gperf against Gecko last-known-good, Gaia last-known-good:

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:772, mean:781, std: 32, max:872, min:749
-------
Running b2gperf against Gecko first-known-bad, Gaia last-known-good:

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:773, mean:778, std: 31, max:858, min:746
-------
Running b2gperf against Gecko last-known-good, Gaia first-known-bad:

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:840, mean:835, std: 23, max:865, min:775
-------
Running b2gperf against Gecko first-known-bad, Gaia first-known-bad:

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:841, mean:860, std: 46, max:983, min:820
-------

It appears that the regression cause is via Gaia. Bisecting:


git bisect start 70953f7 461ea9a

Bisecting: 18 revisions left to test after this (roughly 4 steps)
[7a5ab0709189556f9b212126d41c052b26e98ae7] Bug 983616 - [Search] Pass in region for faster marketplace searches. r=daleharvey

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:785, mean:784, std: 21, max:819, min:748

git bisect good

Bisecting: 9 revisions left to test after this (roughly 3 steps)
[5311c60bf2ff1a5486e80eefcb8ca2a937638679] Merge pull request #19022 from mnjul/bug_1003770_shrinking_ui_test

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:777, mean:777, std: 23, max:834, min:745

git bisect good

Bisecting: 5 revisions left to test after this (roughly 2 steps)
[655a1eb2e9f5f155142560f7d1ebbada390776dc] Merge pull request #19193 from luke-chang/1009379_stingray_change_appentrypoint_naming

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:769, mean:780, std: 24, max:842, min:759

git bisect good

Bisecting: 3 revisions left to test after this (roughly 2 steps)
[e56693762282b3960625ab2dd0089f42d0254dc2] Merge pull request #19219 from etiennesegonzac/bug-1009950-edge-gestures-on

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:821, mean:831, std: 33, max:918, min:789

git bisect bad

Bisecting: 0 revisions left to test after this (roughly 0 steps)
[1297d3048b2891a7a6e8a0f77a07d03facbaf83b] Bug 1009950 - Turning the app-to-app edge gesture on by default.

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:845, mean:885, std: 111, max:1206, min:819

git bisect bad

1297d3048b2891a7a6e8a0f77a07d03facbaf83b is the first bad commit
commit 1297d3048b2891a7a6e8a0f77a07d03facbaf83b
Author: Etienne Segonzac <[e-mail]>
Date:   Tue May 13 17:08:45 2014 -0700

    Bug 1009950 - Turning the app-to-app edge gesture on by default.

:040000 040000 8caac156568479800b864b65bf1ab1ea3b6a7228 17487013305c22b935c216e0cdceb3d92ec259f8 M    apps
:040000 040000 0740c786271660d1fac993fc6cca2b6b65797c18 daabe16c9539860eed6f863295528ba0b438da80 M    build

Comment 1

[:Eli Perelman]

Etienne, it seems that this commit is causing almost all applications to regress their cold launch time according to Datazilla. Could you please provide some insight into whether you can fix this or back it out?

Comment 2

[Etienne Segonzac (:etienne)]

Attachment: 0001-Bug-1010604-Making-it-less-costly-to-enter-edge-mode.patch

Hey Eli, can you help me test if this patch helps?

(I'd like to try this before backing out.)

Comment 3

[:Eli Perelman]

Sure thing, I'll apply this locally and run the same tests against the Flame and measure their impact. I'll post a follow-up report.

Comment 4

[:Eli Perelman]

Comment on attachment 8422827 [details] [diff] [review]
0001-Bug-1010604-Making-it-less-costly-to-enter-edge-mode.patch

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

After applying the patch:

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:846, mean:847, std: 26, max:892, min:800

It appears that the patch didn't have any negligible effect on the cold launch performance.

Comment 5

[Etienne Segonzac (:etienne)]

Attachment: Patch v2

Last try before backout.
(I'm leaving the office, so feel free to backout if needed)

Comment 6

[Etienne Segonzac (:etienne)]

Attachment: Patch v3

Getting somewhere, looks like toggling a css class on #screen is extremely expensive.

This patch is just a proof of concept to confirm that this is the issue. If it yields good result I'll figure out a production ready version.

Also I think the patch from bug 950673 would also fix the issue here.

Comment 7

[:Eli Perelman]

Comment on attachment 8422927 [details] [diff] [review]
Patch v3

Results from v3 Patch:

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:841, mean:843, std: 20, max:892, min:809

Still seems to be about the same.

Comment 8

[Etienne Segonzac (:etienne)]

(In reply to Eli Perelman, :Eli from comment #7)
> Comment on attachment 8422927 [details] [diff] [review]
> Patch v3
> 
> Results from v3 Patch:
> 
> B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:841,
> mean:843, std: 20, max:892, min:809
> 
> Still seems to be about the same.

:/
Just to make sure I'm looking at the right thing, if you disable the edge gestures in Settings -> Developer -> Window Manager, does it fix the launch regression?

Comment 9

[Etienne Segonzac (:etienne)]

Bug 950673 looks ready to land, everything is going to be amazing, thanks for hanging on a bit longer.

Comment 10

[William Lachance (:wlach)]

Eideticker is picking this one up too e.g. for the contacts app:

http://eideticker.mozilla.org/b2g/#/inari/b2g-contacts-startup/timetostableframe

Before: http://eideticker.mozilla.org/b2g/#/inari/b2g-contacts-startup/timetostableframe (2.82 seconds to fully display contacts)
After: http://eideticker.mozilla.org/b2g/detail.html?id=85defc78dc6411e3853610ddb19eacac#/framediffsums (3.63 seconds to fully display contacts

Comment 11

[:Eli Perelman]

(In reply to Etienne Segonzac (:etienne) from comment #8)
> Just to make sure I'm looking at the right thing, if you disable the edge
> gestures in Settings -> Developer -> Window Manager, does it fix the launch
> regression?

It does, here are the results:

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:791, mean:804, std: 32, max:904, min:775

Comment 12

[Etienne Segonzac (:etienne)]

Attachment: Gaia branch

Brillant, since bug 950673 delays the appopen event while the app is cold loading no extra change should be required to fix this bug.

Eli can you give this branch a spin before I mark the bug as a duplicate?

Comment 13

[:Eli Perelman]

Etienne, I'm not sure why, but I couldn't get my phone to boot past the Mozilla logo screen with your branch. I could only get it to boot when I removed the lines [1] from the branch.

That being said, with those lines removed, here are the results of the test:

B2GPerfRunner INFO    | Results for Settings, cold_load_time: median:829, mean:829, std: 31, max:931, min:791

You may want to experiment with those lines, and in the meantime back out the commit until we are confident this has been resolved.

[1] https://github.com/etiennesegonzac/gaia/blob/356c91f113d018b4a0a1ab15dc2bc81eee3181a8/apps/system/js/app_transition_controller.js#L336-L345

Comment 14

[:Eli Perelman]

Comment on attachment 8424912 [details]
Gaia branch

See comment 13.

Comment 15

[Mason Chang [Inactive] [:mchang]]

From :huseby, I tried commenting out window.addEventListener('appopen', this); in apps/system/js/edge_swipe_detector.js. At least for the settings app on a hamachi, cold launch time went from:

Results for Settings, cold_load_time: median:1940, mean:1948, std: 45, max:2136, min:1869, all:2136,1954,1940,2035,1939,1937,1948,1869,1922,1930,1905,1956,1921,2008,1957,1945,1928,1925,1941,1916,1987,1953,1941,1944,1936,1924,1939,1940,1942,1940


To:

Results for Settings, cold_load_time: median:1514, mean:1478, std: 107, max:1662, min:1239, all:1662,1249,1517,1511,1519,1579,1511,1519,1519,1528,1509,1530,1522,1528,1514,1506,1515,1239,1263,1242,1525,1518,1504,1496,1243,1526,1503,1506,1532,1511

Comment 16

[Dave Huseby [:huseby]]

So that was the easy part.  now we need to know why having the event listener and/or what it does adds 400ms to app launch times.  I'm guessing they want the edge detector to work.  We now have to figure out how to do it without regressing launch times.

Comment 17

[Dave Huseby [:huseby]]

I couldn't sleep, so I took a crack at this bug.  I built master for my hamachi and used b2gperf to pinpoint the relative costs of different parts of the appopen event handler.

with no modifications, app launch measured at: 1940ms
I then removed the line: this.screen.classList.add('edges'); from the event handler and measured: 1470ms
I then removed the line: window.addEventListener('appopen', this); from the init fn and measured: 1230ms
I then reverted my changes and just reordered the case statements so that the 'appopen' case was first and measured: 1930ms.

The bulk of the overhead (nearly 500ms) is caused by the modification of the classList.  Then another 200ms comes from the overhead of dispatching the 'appopen' event itself.

I think one possible fix would be to move the edge detection initialization into an idle callback.  And instead of relying on 'appopen', you just adjust the class list in the initialization code.  That should eliminate all of the overhead from the app launch time.

Comment 18

[Etienne Segonzac (:etienne)]

(In reply to Dave Huseby [:huseby] from comment #17)
> I couldn't sleep, so I took a crack at this bug.  I built master for my
> hamachi and used b2gperf to pinpoint the relative costs of different parts
> of the appopen event handler.
> 
> with no modifications, app launch measured at: 1940ms
> I then removed the line: this.screen.classList.add('edges'); from the event
> handler and measured: 1470ms
> I then removed the line: window.addEventListener('appopen', this); from the
> init fn and measured: 1230ms
> I then reverted my changes and just reordered the case statements so that
> the 'appopen' case was first and measured: 1930ms.
> 
> The bulk of the overhead (nearly 500ms) is caused by the modification of the
> classList.  Then another 200ms comes from the overhead of dispatching the
> 'appopen' event itself.

Yeah this is consistent with what I saw in the 3 first patches tested for this bug.

> 
> I think one possible fix would be to move the edge detection initialization
> into an idle callback.  And instead of relying on 'appopen', you just adjust
> the class list in the initialization code.  That should eliminate all of the
> overhead from the app launch time.

Or simply land bug 950673 which delays the appopen dispatch until the app is loaded on cold launch?

Comment 19

[Etienne Segonzac (:etienne)]

(In reply to Eli Perelman, :Eli from comment #13)
> Etienne, I'm not sure why, but I couldn't get my phone to boot past the
> Mozilla logo screen with your branch. I could only get it to boot when I
> removed the lines [1] from the branch.

Which renders the patch useless since we don't delay the appopen dispatch :)

Comment 20

[Dave Huseby [:huseby]]

(In reply to Etienne Segonzac (:etienne) from comment #18)
> Or simply land bug 950673 which delays the appopen dispatch until the app is
> loaded on cold launch?

yes, this sounds like the right way to go.

Comment 21

[Dave Huseby [:huseby]]

I just re-ran b2perf with the patch from Bug 950673 applied.  I measured: 1450ms on my hamachi running master.

Comment 22

[Etienne Segonzac (:etienne)]

(In reply to Dave Huseby [:huseby] from comment #21)
> I just re-ran b2perf with the patch from Bug 950673 applied.  I measured:
> 1450ms on my hamachi running master.

From 1940ms? Sounds promising :)

BTW I'm also removing 2 of the reflows occurring during the app launch as part of bug 1003165 (the bug is about the transition back to the homescreen, but the fix works both ways).

Comment 23

[Etienne Segonzac (:etienne)]

B2Gperf results on master now than Bug 950673 landed:

Edges on:
Results for phone, cold_load_time: median:901, mean:910, std: 33, max:1005, min:881, all:1005,925,903,896,881,900,913,885,898,903

Edges off:
Results for phone, cold_load_time: median:904, mean:912, std: 34, max:1006, min:876, all:1006,893,909,907,898,925,876,902,884,923

Marking as duplicate accordingly.

Comment 24

[Dave Huseby [:huseby]]

Nice job!