750349 - Use an AbsListView setRecyclerListener on the TabsTray to prevent OOM

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

I/ActivityManager(  193): START {cmp=org.mozilla.fennec/org.mozilla.gecko.TabsTray} from pid 3946
I/GeckoViewsFactory( 3946): Creating custom Gecko view: TabsTray$TabsListContainer
I/GeckoViewsFactory( 3946): Creating custom Gecko view: LinkTextView
I/GeckoAppShell( 3946): Taking whole-screen screenshot, viewport: ImmutableViewportMetrics v=(0.0,0.0,720.0,1038.0) p=(720.0,1038.0) z=0.734694
I/ActivityManager(  193): Displayed org.mozilla.fennec/org.mozilla.gecko.TabsTray: +218ms
I/Gecko   ( 3946): Compositor: Composite took 34 ms.
E/GeckoAppShell( 3946): handleGeckoMessage throws java.lang.RuntimeException: too many PopLocalFrame calls
E/GeckoAppShell( 3946): java.lang.RuntimeException: too many PopLocalFrame calls
E/GeckoAppShell( 3946): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
E/GeckoAppShell( 3946): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:488)
E/GeckoAppShell( 3946): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:112)

STR: about ~15 about:home tabs

Comment 1

[Sriram Ramasubramanian [:sriram]]

The reason behind this OOM is:

Let's say there are 10 entries in the list and only 4 entries (1-4) are visible currently. When the user scrolls down, and say, entries 2-5 are visible to the user, a weakreference of entry #1 is added to a hashmap. This is for view recycling. When the user scrolls back up, android checks if it can find entry #1 in the hashmap -- and if it finds one, it passes the same in "convertView". This can be used to avoid inflations.

However, this doesn't solve the OOM problem. OOM happens because, many such tab rows are added to the hashmap -- and each one has a thumbnail with it. This would consume huge amount of memory.

This can be solved by implementing http://developer.android.com/reference/android/widget/AbsListView.html#setRecyclerListener%28android.widget.AbsListView.RecyclerListener%29 and clearing the thumbnail whenever the view goes into the recycle hashmap. Thereby the cached views use very less memory.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch nullifies the drawable in the thumbnail to conserve memory, whenever the row goes out of visibility.

This is even better, if used with "avoiding non-reinflation" as mentioned in Bug 747419.

Comment 3

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

Comment on attachment 619643 [details] [diff] [review]
Patch

[Triage Comment]

Comment 4

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

Comment on attachment 619643 [details] [diff] [review]
Patch

Waiting for dependent patches to land

Comment 5

[Aaron Train [:aaronmt]]

Just noting in here that I tried out Sriram's Try build and the the recycler greatly improves performance in the tabs-tray.

Comment 6

[Lucas Rocha (:lucasr)]

How many times is onMovedToScrapHeap() called while scrolling? This is called in the main thread I guess? I wonder that's the perf impact of calling findViewById() there...

Are thumbnails loaded asynchronously? Are you guaranteeing that any respective async task for loading the thumbnail image is cancelled as well?

Usually, you don't have to directly deal with the recycler if you're recycling views in the adapter. Is the TabsTray listview keeping enough scrap views in memory to cause an OOM situation? That's strange because AFAIK listview keeps the number of scrap views under control[1]. Does that mean that we have a hard limit on the number of tab thumbnails that we can have in memory? If that's the case, it would be good to have a good understanding of what this limit is. For instance, tablets will show more thumbnails than phone.

All in all, I'm finding a bit strange that we're OOM'ing even when the adapter is properly recycling views.

[1] https://github.com/android/platform_frameworks_base/blob/43ee0ab8777632cf171b598153fc2c427586d332/core/java/android/widget/AbsListView.java#L5996

Comment 7

[Sriram Ramasubramanian [:sriram]]

How many times is onMovedToScrapHeap() called while scrolling?
-- Everytime a View goes out of visible range, it gets added to the Scrap heap (as a weak reference) and this onMovedToScrapHeap() will be called.

This is called in the main thread I guess? I wonder that's the perf impact of calling findViewById() there...
-- Aaah.. Not again please. Let's move to to ViewHolder / CustomClass bug for discussions on findViewById() :(

Are thumbnails loaded asynchronously?
-- Thumbnails are loaded from Tab object as "thumbnail.setImageDrawable(that_thumbnail)". They are fetched from Gecko asynchronously.

Are you guaranteeing that any respective async task for loading the thumbnail image is cancelled as well?
-- We don't need any such task.

Usually, you don't have to directly deal with the recycler if you're recycling views in the adapter.
-- Base adapter doesn't provide "recycling option". [ http://developer.android.com/reference/android/widget/BaseAdapter.html ]. And we don't need to deal with recycling if the listview is simple -- without images.

Is the TabsTray listview keeping enough scrap views in memory to cause an OOM situation?
-- There are 2 parts to this.
1. ScrapViews aren't controlled by us. And these views hold the thumbnails unnecessarily.
2. There is a chance that a new instance TabsTray getting created before old one is GC-ed, causing OOM (still more memory by Drawable in scrap view)

That's strange because AFAIK listview keeps the number of scrap views under control[1].
-- It does. But what if OOM happens "before" GC-ing?

Does that mean that we have a hard limit on the number of tab thumbnails that we can have in memory?
-- Nope, as far as TabsTray is concerned. But yes, for holding the thumbnails in Tab.java. We should probably find ways to do it. May be some sort of weak reference -- when dealing with 50+ tabs.

All in all, I'm finding a bit strange that we're OOM'ing even when the adapter is properly recycling views.
-- Adapter does'nt recycle views by itself. ListView does. However, only for less views in each rows. We have quite a lot of images with textured backgrounds. So, it holds up more memory.

Comment 8

[Sriram Ramasubramanian [:sriram]]

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

Comment 9

[Aaron Train [:aaronmt]]

Tried out a new inbound build with this fix, verified fixed

Comment 10

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #7)
> This is called in the main thread I guess? I wonder that's the perf impact
> of calling findViewById() there...
> -- Aaah.. Not again please. Let's move to to ViewHolder / CustomClass bug
> for discussions on findViewById() :(

Don't really know what you mean by "not again" here.
 
> Are thumbnails loaded asynchronously?
> -- Thumbnails are loaded from Tab object as
> "thumbnail.setImageDrawable(that_thumbnail)". They are fetched from Gecko
> asynchronously.

So, the Tab instance holds a reference to the thumbnail image, right?

> Usually, you don't have to directly deal with the recycler if you're
> recycling views in the adapter.
> -- Base adapter doesn't provide "recycling option". [
> http://developer.android.com/reference/android/widget/BaseAdapter.html ].
> And we don't need to deal with recycling if the listview is simple --
> without images.

Not true. All adapters provide a way to recycle views through getView() (i.e. the convertView argument). You should never inflate views on every getView() call, this will just create tons of views that will be thrown away almost immediately while scrolling. This neither memory efficient nor good for scrolling performance.

> Is the TabsTray listview keeping enough scrap views in memory to cause an
> OOM situation?
> -- There are 2 parts to this.
> 1. ScrapViews aren't controlled by us. And these views hold the thumbnails
> unnecessarily.

IIUC, Tab instances hold a hard reference to the thumbnail drawable used in the adapter. That probably means that even though you're setting setImageDrawable(null) in the non-visible row, the thumbnail drawable will not be garbage collected anyway.

> 2. There is a chance that a new instance TabsTray getting created before old
> one is GC-ed, causing OOM (still more memory by Drawable in scrap view)

According to the OOM exception docs, it will be "Thrown when the Java Virtual Machine cannot allocate an object because it is out of memory, and no more memory could be made available by the garbage collector."

This means that there will always be a at least one round of gargage collection before OOM exception is thrown. And, as I said, there's a hard reference to the drawable in each Tab instance. Meaning that it's unlikely that the drawable will be GC-ed even if you setImageDrawable(null) on scrap views. Or are we duplicating the drawables somewhere?

> That's strange because AFAIK listview keeps the number of scrap views under
> control[1].
> -- It does. But what if OOM happens "before" GC-ing?

As I said, this is very unlikely (if not impossible). GC will always free as much memory as possible before OOM is thrown (at least according to official docs).

> Does that mean that we have a hard limit on the number of tab thumbnails
> that we can have in memory?
> -- Nope, as far as TabsTray is concerned. But yes, for holding the
> thumbnails in Tab.java. We should probably find ways to do it. May be some
> sort of weak reference -- when dealing with 50+ tabs.

Yeah, I can't think of a good reason to keep those thumbnails references in Tab all the time for every tab.
 
> All in all, I'm finding a bit strange that we're OOM'ing even when the
> adapter is properly recycling views.
> -- Adapter does'nt recycle views by itself. ListView does. However, only for
> less views in each rows. We have quite a lot of images with textured
> backgrounds. So, it holds up more memory.

I still find this patch somehow suspicious. The most likely suspect for the OOM issues (especially when you have tons of tabs) seems to actually be the thumbnail drawable in each Tab instance. There are some issues with the assumptions your used to write this patch here.

Am I missing something?

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #1)
> The reason behind this OOM is:
> 
> Let's say there are 10 entries in the list and only 4 entries (1-4) are
> visible currently. When the user scrolls down, and say, entries 2-5 are
> visible to the user, a weakreference of entry #1 is added to a hashmap. This
> is for view recycling. When the user scrolls back up, android checks if it
> can find entry #1 in the hashmap -- and if it finds one, it passes the same
> in "convertView". This can be used to avoid inflations.

This is not exactly how ListView works. It doesn't try to match the exact view for a specific position in the recycler. It only uses the item position to know which type of row should be recycled. If you look into RecycleBin source code[1], you'll see that the pool of scrapviews is actually an array of lists of views for each row type. So, convertView is only guaranteed to be of same type of the inflated view for that specific position. This is why we still have to update the actual content of the recycled row.

> However, this doesn't solve the OOM problem. OOM happens because, many such
> tab rows are added to the hashmap -- and each one has a thumbnail with it.
> This would consume huge amount of memory.

As I said in my previous comment, the thumbnail drawables are actually held by each Tab instance. Using those thumbnail drawables in each row view shouldn't cause it to get duplicated in memory I suppose? The scrapviews don't really seem to be the actual cause of the OOM exception here. Again, am I missing something?

[1] https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/AbsListView.java#L5783

Comment 12

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #11)

> This is not exactly how ListView works. It doesn't try to match the exact
> view for a specific position in the recycler. It only uses the item position
> to know which type of row should be recycled. If you look into RecycleBin
> source code[1], you'll see that the pool of scrapviews is actually an array
> of lists of views for each row type. So, convertView is only guaranteed to
> be of same type of the inflated view for that specific position. This is why
> we still have to update the actual content of the recycled row.

https://github.com/android/platform_frameworks_base/blob/master/core/java/android/widget/AbsListView.java#L5881

As you can see here, it tries to find a View based on the "position" requested. In case it cannot find one, it'll give you a View of similar "type". That's the reason we need to be assigning the values everytime we get a convertView!

> As I said in my previous comment, the thumbnail drawables are actually held
> by each Tab instance. Using those thumbnail drawables in each row view
> shouldn't cause it to get duplicated in memory I suppose? The scrapviews
> don't really seem to be the actual cause of the OOM exception here. Again,
> am I missing something?

Does my patch fix the issue? [https://bugzilla.mozilla.org/show_bug.cgi?id=750349#c9]

Comment 13

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #10)
> 
> Not true. All adapters provide a way to recycle views through getView()
> (i.e. the convertView argument). You should never inflate views on every
> getView() call, this will just create tons of views that will be thrown away
> almost immediately while scrolling. This neither memory efficient nor good
> for scrolling performance.

All adapters provide a way to recycle views through getView = yes!
But it doesn't "recycle by itself". It's not an adapter's property to do it. It's upto the developer to see if he wants to use the view given by the adapter or not. So by default adapter "does not" do effective recycling. It should be manually done.

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/218a34d41e5b

Comment 15

[(no longer active)]

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

Comment 16

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/efcadd56464e