1107636 - Replace top level RelativeLayout with something less expensive

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

We currently don't seem to use any of the top level relativelayout's view properties, and given how relativelayouts are computationally expensive when measuring, it's best we replace this with something else.

Comment 1

[Richard Newman [:rnewman]]

Blocks or dupes Bug 1106931?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

It looks like we now have:

<OuterLayout> (a RelativeLayout)
  ...
  <view class="org.mozilla.gecko.GeckoApp$MainLayout"> (a RelativeLayout)
    <RelativeLayout id="gecko_layout">
       ...
    ...

The elements of OuterLayout are all match_parent in both dimensions with the exception of the Toast, which only uses gravity and should be controllable in a FrameLayout.

MainLayout is not as easy a fix, but it seems to be replaceable by a Frame/LinearLayout combination. It gets messy though as the gecko_layout is under the tab strip, but the tab strip is contained in a browser_chrome layout. This could also be a good place for a custom layout (which could make things even more performant).

gecko_layout also is mostly match_parent in both dimensions and seems replaceable with a FrameLayout.

Worth noting that we should profile our changes to ensure they improve performance.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #2)
> The elements of OuterLayout are all match_parent in both dimensions with the
> exception of the Toast, which only uses gravity and should be controllable
> in a FrameLayout.

Good call! I killed OuterLayout in bug 1217109. If a FrameLayout is sufficient then maybe we can get rid of it completely by using <merge>. The content layout is already a FrameLayout:
http://android-developers.blogspot.de/2009/03/android-layout-tricks-3-optimize-by.html

If I remember correctly it's currently only used as reference point for the Snackbar (mRootLayout). But you can pass almost any view to the Snackbar and it will go up the tree to find a suitable parent.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Oh, another thing: I was thinking about adding a CoordinatorLayout[1] somewhere in the layout tree. This would add swipe-to-dismiss functionality to Snackbars automatically. Maybe that's something worth looking into as well here.

[1] http://developer.android.com/reference/android/support/design/widget/CoordinatorLayout.html

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1107636 - Replace top-level RelativeLayout with <merge>. r=sebastian

Bug 1107636 - Replace top-level RelativeLayout with <merge>. r=sebastian

Using systrace, I took 2 traces before and after. Using the slowest
of both traces, the first three measure passes had the following
improvements in CPU time:
  1) 20.583ms -> 13.149ms
  2) 1.695ms -> 0.827ms (1/2)
  3) 22.146ms -> 11.602ms (1/2)

This is to be expected – RelativeLayout requires two measure passes
so it makes sense to approximately half the measure times.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1107636 - Use MarginLayoutParams where applicable. r=sebastian

Bug 1107636 - Use MarginLayoutParams where applicable. r=sebastian

MarginLayoutParams is less specific than RelativeLayout.LayoutParams
and is easier to make changes with.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1107636 - Change gecko_layout to FrameLayout. r=sebastian

Bug 1107636 - Change gecko_layout to FrameLayout. r=sebastian

Changes on first three measure passes (CPU time):
  1) 13.149ms -> 11.934ms
  2) .827ms -> .582ms
  3) 11.602 -> 12.199ms

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8684483 [details]
MozReview Request: Bug 1107636 - Replace top-level RelativeLayout with <merge>. r=sebastian

https://reviewboard.mozilla.org/r/24583/#review22181

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8684484 [details]
MozReview Request: Bug 1107636 - Use MarginLayoutParams where applicable. r=sebastian

https://reviewboard.mozilla.org/r/24585/#review22183

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8684485 [details]
MozReview Request: Bug 1107636 - Change gecko_layout to FrameLayout. r=sebastian

https://reviewboard.mozilla.org/r/24587/#review22185

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Nice series of changes! I like that you took the time to measure the improvements. :)

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

re CoordinatorLayout (comment 4), filed bug 1223048.

I filed bug 1223053 to take care of the remaining GeckoApp$MainLayout.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Barbara, I think this needs an Aha card. It should land in 45.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/6a84303852998c3e07371f8f9b26a1218c961b95
Bug 1107636 - Replace top-level RelativeLayout with <merge>. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/a1efec5e79de59e32dc745e34683e0cccc28aa17
Bug 1107636 - Use MarginLayoutParams where applicable. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/ced71b78683d6c82817ab9be00fe922a57414bdb
Bug 1107636 - Change gecko_layout to FrameLayout. r=sebastian

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/96655290bcd9 for android mochitest bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=5633882&repo=fx-team

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Full stack:

11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): java.lang.ClassCastException: android.widget.RelativeLayout$LayoutParams cannot be cast to android.widget.FrameLayout$LayoutParams
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.widget.FrameLayout.onMeasure(FrameLayout.java:311)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.View.measure(View.java:15848)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.widget.RelativeLayout.measureChildHorizontal(RelativeLayout.java:728)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.widget.RelativeLayout.onMeasure(RelativeLayout.java:477)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.View.measure(View.java:15848)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:5012)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.widget.FrameLayout.onMeasure(FrameLayout.java:310)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.View.measure(View.java:15848)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:5012)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.widget.LinearLayout.measureChildBeforeLayout(LinearLayout.java:1404)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.widget.LinearLayout.measureVertical(LinearLayout.java:695)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.widget.LinearLayout.onMeasure(LinearLayout.java:588)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.View.measure(View.java:15848)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:5012)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.widget.FrameLayout.onMeasure(FrameLayout.java:310)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at com.android.internal.policy.impl.PhoneWindow$DecorView.onMeasure(PhoneWindow.java:2189)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.View.measure(View.java:15848)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.ViewRootImpl.performMeasure(ViewRootImpl.java:1905)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.ViewRootImpl.measureHierarchy(ViewRootImpl.java:1104)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:1284)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1004)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:5481)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.Choreographer$CallbackRecord.run(Choreographer.java:749)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.Choreographer.doCallbacks(Choreographer.java:562)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.Choreographer.doFrame(Choreographer.java:532)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:735)
11:19:28     INFO -  11-09 11:18:16.726 E/GeckoCrashHandler(  747): 	at android.os.Handler.handleCallback(Handler.java:730)

which doesn't tell us where these RelativeLayout.LayoutParams are being added from. Presumably, we new some RelativeLayout.LayoutParams somewhere, set it as the layout params for a view, and then when the system relayouts out, we crash. Leave NI to continue investigations.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65706273a312

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The try push in comment 17 changes the RelativeLayout in web_app to a FrameLayout because the id matches one we changed in a previous commit. Waiting on try...

Comment 19

[Barbara Bermes [:barbara] - NI please!]

(In reply to Michael Comella (:mcomella) from comment #13)
> Barbara, I think this needs an Aha card. It should land in 45.

Could you please help me come up with a feature card title for this, i.e. so everyone looking at the board will understand what this means?

Even if this change is not visible to the user. Should I call it "Improved CPU times due to layout changes (RelativeLayout)"

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Barbara Bermes [:barbara] from comment #19)
> Could you please help me come up with a feature card title for this, i.e. so
> everyone looking at the board will understand what this means?

"Improved application layout performance (RelativeLayout)"

It's hard to say if the impact is negligible or not. I measured that initial layout performance (at startup) is at least somewhat improved, however, the tool I used shows the layout in chunks so it is difficult to sum together the entire process to identify if it was negligible. Additionally, these changes will help if the layout changes a lot or there are many of these views, but I don't think our layout changes much and we have one view of this layout.

Maybe this doesn't need an Aha card – I'm not sure I know what the qualifications are.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f52018d39cbb

Comment 22

[Barbara Bermes [:barbara] - NI please!]

> Maybe this doesn't need an Aha card – I'm not sure I know what the
> qualifications are.

I might leave it for now, normally it's something that should qualify a good reason to be worked on to align with our themes and goals of the company, and in a sense important enough, i.e. so that we can share the result with users, legal and marketing etc.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d47c336b7293

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8af3206963bb

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/446d2b181d40c90d7b012246259399b6e3d9051b
Bug 1107636 - Replace top-level RelativeLayout with <merge>. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/014b8c296cec893ebad9df2c18b874360d99e74b
Bug 1107636 - Use MarginLayoutParams where applicable. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/2b6f2bcfe767c326e67e8d89b2b3cb7d9f9ec849
Bug 1107636 - Change gecko_layout to FrameLayout. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/c575729788a7bb3c6632a4a29063c0d5c4150629
Bug 1107636 - Replace RelativeLayout with FrameLayout in WebApp. r=me

https://hg.mozilla.org/integration/fx-team/rev/9145f78bb39c3fbc447e3d0b743e46615f405ffc
Bug 1107636 - Remove RelativeLayout param from within FrameLayout. r=me

https://hg.mozilla.org/integration/fx-team/rev/6e940538a1718ca8f2fba26b50dc1a44046ac916
Bug 1107636 - Change RelativeLayout params -> FrameLayout params. r=me

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out for robocheck2 failures: https://treeherder.mozilla.org/logviewer.html#?job_id=5684438&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/5f9371f97e1e

Comment 27

[:Margaret Leibovic]

(In reply to Barbara Bermes [:barbara] from comment #22)
> > Maybe this doesn't need an Aha card – I'm not sure I know what the
> > qualifications are.
> 
> I might leave it for now, normally it's something that should qualify a good
> reason to be worked on to align with our themes and goals of the company,
> and in a sense important enough, i.e. so that we can share the result with
> users, legal and marketing etc.

The main reason I see for an Aha card for this is to make sure the engineering time is accounted for in Aha.

That being said, I don't know that this is a top priority, so hopefully that's only a small amount of engineering time :)

Comment 28

[Geoff Brown [:gbrown]]

(In reply to Wes Kocher (:KWierso) from comment #26)
> All backed out for robocheck2 failures:
> https://treeherder.mozilla.org/logviewer.html#?job_id=5684438&repo=fx-team
> 
> https://hg.mozilla.org/integration/fx-team/rev/5f9371f97e1e

Most of the logcats end with something like:

16:52:25     INFO -  11-11 16:51:05.421 D/GeckoToolbar( 2168): onTabChanged: THUMBNAIL
16:52:25     INFO -  11-11 16:51:05.421 D/GeckoBrowserApp( 2168): BrowserApp.onTabChanged: 0: THUMBNAIL
16:52:25     INFO -  11-11 16:51:15.312 I/Robocop ( 2168): PaintExpecter: no longer listening for events
16:52:25     INFO -  11-11 16:51:15.968 I/dalvikvm-heap( 2168): Grow heap (frag case) to 11.459MB for 2887696-byte allocation

just before the crash reporter is started. I'm not sure how relevant that is to the crash. Certainly the crash is happening around the end of the loadAndPaint() call at http://hg.mozilla.org/mozilla-central/annotate/4294bf91174b/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testCheck2.java#l30.

Comment 29

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Nick also pointed out this line [1]:
  View geckoLayout = mActivity.findViewById(R.id.gecko_layout);

What do we do with that layout?

[1]: https://dxr.mozilla.org/mozilla-central/rev/bc74dbdea094059d5f1d353a2585b4f6352b6ec4/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/FennecNativeDriver.java#75

Comment 30

[Barbara Bermes [:barbara] - NI please!]

(In reply to :Margaret Leibovic from comment #27)
> (In reply to Barbara Bermes [:barbara] from comment #22)
> > > Maybe this doesn't need an Aha card – I'm not sure I know what the
> > > qualifications are.
> > 
> > I might leave it for now, normally it's something that should qualify a good
> > reason to be worked on to align with our themes and goals of the company,
> > and in a sense important enough, i.e. so that we can share the result with
> > users, legal and marketing etc.
> 
> The main reason I see for an Aha card for this is to make sure the
> engineering time is accounted for in Aha.
> 
> That being said, I don't know that this is a top priority, so hopefully
> that's only a small amount of engineering time :)

You are right, we need to know what people work on, and if somebody is working on that, we should understand why and how it compares to other P* cards we have in Aha.

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I'm aware that this needs to be done but it's not prioritized – removing NI.

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d62b52800709

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I won't be getting to this for a while.

Comment 34

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.