706819 - Displaying the TabsTray (Tab Menu) takes too long

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

With multiple tabs open, tap on the "tabs" button in the URL bar. The tabs menu should display right away, but takes almost 2 secs to appear on my Nexus One. In logcat I see:

I/ActivityManager(   98): Starting: Intent { flg=0x40000000 cmp=org.mozilla.fennec_mfinkle/org.mozilla.gecko.TabsTray } from pid 2267
D/PhoneWindow( 2267): couldn't save which view has focus because the focused view org.mozilla.gecko.gfx.LayerView@4058e0d8 has no id.
D/dalvikvm( 2267): GC_EXTERNAL_ALLOC freed 40K, 54% free 3424K/7431K, external 12549K/14597K, paused 54ms
W/ActivityManager(   98): Activity pause timeout for HistoryRecord{40553d68 org.mozilla.fennec_mfinkle/.App}
I/GeckoApp( 2267): pause
D/GeckoInputConnection( 2267): IME: finishComposingText
D/GeckoInputConnection( 2267): IME: finishComposingText
I/ActivityManager(   98): Displayed org.mozilla.fennec_mfinkle/org.mozilla.gecko.TabsTray: +918ms

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Note: I am using build 20111201 and this did not happen yesterday.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Aaron noticed that if you tap multiple times, quickly before the tab menu opens, you can actually open multiple copies of the window. We should defend against this too.

Comment 3

[Aaron Train [:aaronmt]]

Also see:

E/CursorWindow(  793): need to grow: mSize = 1048576, size = 230918, freeSpace() = 100998, numRows = 28
E/CursorWindow(  793): not growing since there are already 28 row(s), max size 1048576

Filed bug 706822 for the AwesomeScreen; probably both related.

Comment 4

[Aaron Train [:aaronmt]]

http://hg.mozilla.org/projects/birch/pushloghtml?fromchange=8e9a466b85bb&tochange=24f415e415f2

Comment 5

[Lucas Rocha (:lucasr)]

Blassey, maybe caused by one your screenshot patches?

Comment 6

[Aaron Train [:aaronmt]]

Something overnight fix this?

Seems fixed in todays tinderbox-builds.

Comment 7

[Sriram Ramasubramanian [:sriram]]

This was fixed as a part of Brad's fix for saving the screenshot in background thread. I was about to close this :)

Comment 8

[Aaron Train [:aaronmt]]

This still happens after you switch a tab and then proceed to open the tray. Any follow-up attempts to open the tray are fine, it is only during the initial attempt after you switch a tab.

Comment 9

[Aaron Train [:aaronmt]]

(In reply to Aaron Train [:aaronmt] from comment #8)
> This still happens after you switch a tab and then proceed to open the tray.
> Any follow-up attempts to open the tray are fine, it is only during the
> initial attempt after you switch a tab.

When I do this, I see in console:

W/ActivityManager( 2385): Activity pause timeout for HistoryRecord{40866168 org.mozilla.fennec/.App}
I/GeckoApp( 3048): pause

Related?

Comment 10

[Sriram Ramasubramanian [:sriram]]

This is because, the activity goes to background, and entering "pause" state. In pause state, we are getting a screenshot of the screen. This is creating the lag in showing the TabsTray. This still depends on bug 706901.

Comment 11

[Sriram Ramasubramanian [:sriram]]

From profiling between showTabs() in GeckoApp and finishActivity() in TabsTray, I found these:
* JSONObject init() is taking around 300ms - this is done for every message obtained from Gecko in handleMessage().
* ActionBar is created and then hidden - creation takes around 124ms. We should probably hide it in XML itself and see if there is a win.
* Pausing an activity (onPause()) takes a 106ms. The new activity is stalled by this amount from even starting to create. We should see if we want to take screenshots on onPause() if we are calling TabsTray.
* Getting DisplayMetrics on onCreate() is costly (80 ms). It's better to have a separate class that caches these values when application starts, resumes and pauses. Other activities can take the values from this class.
* getAndProcessThumbnails() runs both on main thread and looper thread. How can this be possible? :(

Other than this, assignValues() is called many times to update just the thumbnail. However, assignValues() "redraws" every element in that particular row of the tab. This causes sluggishness. If we can be more judicial on just which particular UI element to update, we can save few ms and be more responsive.

Comment 12

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

I tried hiding the action bar in XML. There is some win due to it - as the ActionBar is not created and then hidden during onCreate() - hidden without even creating.

With 2 tabs, on Nexus S running ICS, opening TabsTray three times (first one is done when a page is loading):

< without this fix >
I/ActivityManager(  154): Displayed org.mozilla.fennec_sriramramasubramanian/org.mozilla.gecko.TabsTray: +930ms
I/ActivityManager(  154): Displayed org.mozilla.fennec_sriramramasubramanian/org.mozilla.gecko.TabsTray: +286ms
I/ActivityManager(  154): Displayed org.mozilla.fennec_sriramramasubramanian/org.mozilla.gecko.TabsTray: +307ms

< with this fix >
I/ActivityManager(  154): Displayed org.mozilla.fennec_sriramramasubramanian/org.mozilla.gecko.TabsTray: +622ms
I/ActivityManager(  154): Displayed org.mozilla.fennec_sriramramasubramanian/org.mozilla.gecko.TabsTray: +222ms
I/ActivityManager(  154): Displayed org.mozilla.fennec_sriramramasubramanian/org.mozilla.gecko.TabsTray: +315ms

However, this doesn't come for free. Android adds a small menu button to the three-button bar at the bottom on ICS. I'll upload a screenshot of the same soon. If that is fine, we can have this patch, thereby TabsTray will be faster.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Attachment: Screenshot: Small menu button at the bottom

There is a small menu button at the bottom, if the WIP mentioned above is applied to ICS on Galaxy Nexus.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Hmm, that saves about 300ms on the first open. Inflating the actionbar?

I don't know if UX will go for the small menu button. Is there anything we can do that avoids the button, but still improves speed?

Comment 15

[Lucas Rocha (:lucasr)]

Sriram, while you're at it, I noticed that the TabsAdapter doesn't implement the getView() properly. It shouldn't always inflate the view if the passed convertView != null. In that case, it should just populate convertView with the tab values. That might give us some performance boost. Not sure how much though.

Comment 16

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #14)
> Hmm, that saves about 300ms on the first open. Inflating the actionbar?
> 
> I don't know if UX will go for the small menu button. Is there anything we
> can do that avoids the button, but still improves speed?

To avoid the small menu button, I didn't hide the action bar in XML initially - and did it in Java. This had increased the startup by 300ms.
I couldn't see other ways of going around the small menu button :(

Comment 17

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #15)
> Sriram, while you're at it, I noticed that the TabsAdapter doesn't implement
> the getView() properly. It shouldn't always inflate the view if the passed
> convertView != null. In that case, it should just populate convertView with
> the tab values. That might give us some performance boost. Not sure how much
> though.

My initial code had the option of inflating the views only if convertView is not null. And there was some performance gain (as the inflation reduces). However, back then, Ian wanted to bolden the highlighted tab. This caused other tabs to be boldened as well. So, I had to remove that re-using option (as it is only a reference). I am thinking of doing "select item" on the list and changing the behavior of the list's row to add the small tab when selected. If that works, then we can use the convertView as Lucas mentioned.

Comment 18

[Sriram Ramasubramanian [:sriram]]

http://www.blogcdn.com/www.engadget.com/media/2012/02/01gwnexus20212.jpg - Google wallet's startup screen has the small little menu button at the bottom on Galaxy Nexus.

Comment 19

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Defer refreshing thumbnails

This patch defers refreshing the thumbnails to when the list is fully inflated. The idea is that the Tabs will have a thumbnail with them that will be shown during onCreate(). The refreshing is triggered after the list is shown.
I get a performance win of 20-50 ms with no thumbnails being shown (due to some bug at this point). I expect more performance win with this.

Comment 20

[Madhava Enros [:madhava]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #19)
> Created attachment 594258 [details] [diff] [review]
> WIP: Defer refreshing thumbnails
> 
> This patch defers refreshing the thumbnails to when the list is fully
> inflated. The idea is that the Tabs will have a thumbnail with them that
> will be shown during onCreate(). The refreshing is triggered after the list
> is shown.
> I get a performance win of 20-50 ms with no thumbnails being shown (due to
> some bug at this point). I expect more performance win with this.

Does this mean the thumbnails will change after they're already visible?

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Madhava Enros [:madhava] from comment #20)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #19)
> > Created attachment 594258 [details] [diff] [review]
> > WIP: Defer refreshing thumbnails
> > 
> > This patch defers refreshing the thumbnails to when the list is fully
> > inflated. The idea is that the Tabs will have a thumbnail with them that
> > will be shown during onCreate(). The refreshing is triggered after the list
> > is shown.
> > I get a performance win of 20-50 ms with no thumbnails being shown (due to
> > some bug at this point). I expect more performance win with this.
> 
> Does this mean the thumbnails will change after they're already visible?

That already happens. Refreshing the thumbnails is async, so the updates arrive after the panel is shown. This patch just delays starting the refresh until after the panel is visible.

Comment 22

[Sriram Ramasubramanian [:sriram]]

This patch gives me a performance win of 250-300ms with thumbnails enabled. I don't see that big difference in getting the thumbnails being refreshed after being shown. The user wouldn't see any hindrance with this.

I/ActivityManager(  154): Displayed org.mozilla.fennec_sriramramasubramanian/org.mozilla.gecko.TabsTray: +376ms

with 5 tabs open. :)

I would be happy to push this and the ActionBar hiding in XML patches and see how users feel in Beta. This is a really good performance win.

I am trying Lucas's suggestion of reusing the views while inflating. That needs more plumbing than needed (like the RelativeLayout supporting "checked" state). I would be able to try that out, if we want to reduce the display time even further (to be less than 250ms that we would have with these patches).

Comment 23

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Finally I found a way to hide the actionbar without showing the small menu button. This code hides it in the style -- instead of "no action bar" approach that aadds the menu button.
I haven't seen much of a performance win. Somewhere some code change has brought down the Ts to show TabsTray. :D
This patch now saves around 50-100ms. I would like to have this patch anyways to align with other activities handling ActionBar in XML.

Comment 24

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ca39e09c5a3f

The bug is not fixed yet. This is just a part of it.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 594258 [details] [diff] [review]
WIP: Defer refreshing thumbnails

Given the better performance while opening, we might not need this.

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/ca39e09c5a3f