Closed Bug 1010604 Opened 11 years ago Closed 11 years ago

Multiple applications regressing on cold launch May 14, 2014

Categories

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

x86
macOS
defect

Tracking

(blocking-b2g:2.0+)

RESOLVED DUPLICATE of bug 950673
2.0 S2 (23may)
blocking-b2g 2.0+

People

(Reporter: Eli, Assigned: etienne)

References

Details

(Keywords: perf, regression, Whiteboard: [c=automation p= s= u=2.0][systemsfe])

Attachments

(1 file, 3 obsolete files)

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 <etienne@segonzac.info> 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
Keywords: perf
Whiteboard: [c=automation p= s= u=]
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?
Flags: needinfo?(etienne)
Blocks: 1009950
Hey Eli, can you help me test if this patch helps? (I'd like to try this before backing out.)
Assignee: nobody → etienne
Attachment #8422827 - Flags: feedback?(eperelman)
Flags: needinfo?(etienne)
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 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.
Attachment #8422827 - Flags: feedback?(eperelman) → feedback-
Attached patch Patch v2 (obsolete) (deleted) — Splinter Review
Last try before backout. (I'm leaving the office, so feel free to backout if needed)
Attachment #8422827 - Attachment is obsolete: true
blocking-b2g: --- → 2.0?
Keywords: regression
Attached patch Patch v3 (obsolete) (deleted) — Splinter Review
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.
Attachment #8422868 - Attachment is obsolete: true
Attachment #8422927 - Flags: feedback?(eperelman)
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.
Attachment #8422927 - Flags: feedback?(eperelman) → feedback-
Status: NEW → ASSIGNED
blocking-b2g: 2.0? → 2.0+
Whiteboard: [c=automation p= s= u=] → [c=automation p= s= u=2.0]
Whiteboard: [c=automation p= s= u=2.0] → [c=automation p= s= u=2.0][systemsfe]
(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?
Flags: needinfo?(eperelman)
Component: Gaia → Gaia::System::Window Mgmt
Bug 950673 looks ready to land, everything is going to be amazing, thanks for hanging on a bit longer.
Depends on: 950673
(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
Flags: needinfo?(eperelman)
Attached file Gaia branch (deleted) —
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?
Attachment #8422927 - Attachment is obsolete: true
Attachment #8424912 - Flags: feedback?(eperelman)
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
Flags: needinfo?(etienne)
Comment on attachment 8424912 [details] Gaia branch See comment 13.
Attachment #8424912 - Flags: feedback?(eperelman) → feedback-
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
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.
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.
(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?
Flags: needinfo?(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 :)
(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.
I just re-ran b2perf with the patch from Bug 950673 applied. I measured: 1450ms on my hamachi running master.
Flags: needinfo?(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).
Flags: needinfo?(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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Nice job!
Target Milestone: --- → 2.0 S2 (23may)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: