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)
Tracking
(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
Reporter | ||
Comment 1•11 years ago
|
||
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)
Priority: -- → P1
Assignee | ||
Comment 2•11 years ago
|
||
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)
Reporter | ||
Comment 3•11 years ago
|
||
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.
Reporter | ||
Comment 4•11 years ago
|
||
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-
Assignee | ||
Comment 5•11 years ago
|
||
Last try before backout.
(I'm leaving the office, so feel free to backout if needed)
Attachment #8422827 -
Attachment is obsolete: true
Updated•11 years ago
|
blocking-b2g: --- → 2.0?
Keywords: regression
Assignee | ||
Comment 6•11 years ago
|
||
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)
Reporter | ||
Comment 7•11 years ago
|
||
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-
Updated•11 years ago
|
Status: NEW → ASSIGNED
blocking-b2g: 2.0? → 2.0+
Whiteboard: [c=automation p= s= u=] → [c=automation p= s= u=2.0]
Updated•11 years ago
|
Blocks: edge-gestures
Updated•11 years ago
|
Whiteboard: [c=automation p= s= u=2.0] → [c=automation p= s= u=2.0][systemsfe]
Assignee | ||
Comment 8•11 years ago
|
||
(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)
Updated•11 years ago
|
Component: Gaia → Gaia::System::Window Mgmt
Assignee | ||
Comment 9•11 years ago
|
||
Bug 950673 looks ready to land, everything is going to be amazing, thanks for hanging on a bit longer.
Depends on: 950673
Comment 10•11 years ago
|
||
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
Reporter | ||
Comment 11•11 years ago
|
||
(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)
Assignee | ||
Comment 12•11 years ago
|
||
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)
Reporter | ||
Comment 13•11 years ago
|
||
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)
Reporter | ||
Comment 14•11 years ago
|
||
Comment on attachment 8424912 [details]
Gaia branch
See comment 13.
Attachment #8424912 -
Flags: feedback?(eperelman) → feedback-
Comment 15•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 18•11 years ago
|
||
(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)
Assignee | ||
Comment 19•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
I just re-ran b2perf with the patch from Bug 950673 applied. I measured: 1450ms on my hamachi running master.
Flags: needinfo?(etienne)
Assignee | ||
Comment 22•11 years ago
|
||
(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)
Assignee | ||
Comment 23•11 years ago
|
||
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
Comment 24•11 years ago
|
||
Nice job!
Updated•10 years ago
|
Target Milestone: --- → 2.0 S2 (23may)
You need to log in
before you can comment on or make changes to this bug.
Description
•