919777 - HomeListView is slow on startup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

HomeListView (on top sites and history pages) doesn't have a smooth scroll (at 60fps). With about 50+ entries, scrolling stutters when we try to scroll for the "first time". Once the entire list has been scrolled (with all the favicons loaded in memcache), the scrolling becomes smooth again.

Note: Though the idea of putting favicons in memcache has been there for a long time, this regression is new. I remember checking the list-view's scrolling speed and we never crossed the 16ms per frame mark (Enable "Profile GPU Rendering" > "As bars" under "Developer options").

Comment 1

[Sriram Ramasubramanian [:sriram]]

I feel the major problem here is that, the ListView is not gesture aware. So firing many AsyncTasks is causing a big problem, as when the favicons are loaded, they cause a re-layout for each row. It's better to batch the loading, and wait till the scrolling stops to change the icons. (Note: The scrolling gets smoother once the favicons are in mem-cache -- which doesn't involve AsyncTask).

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: fill_parent

By using fill_parent, the TextView is always a constant and would never need to be measured when the size of the view changes. 3ms win for each row while scrolling. That's a loooooot! :D

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #1)
> I feel the major problem here is that, the ListView is not gesture aware. So
> firing many AsyncTasks is causing a big problem, as when the favicons are
> loaded, they cause a re-layout for each row. It's better to batch the
> loading, and wait till the scrolling stops to change the icons. (Note: The
> scrolling gets smoother once the favicons are in mem-cache -- which doesn't
> involve AsyncTask).

The gesture awareness is more relevant to this issue than the batch loading btw. The Android framework batches re-layout requests under the hood. This means that if all rows request layout roughly at the same time, there will likely be only one layout round to account for that. Besides, doing re-layouts when the list is not scrolling is a non-issue for the most part.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 810804 [details] [diff] [review]
Patch: fill_parent

Review of attachment 810804 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

OMG FAST!
This works fine now. The issue with wrong favicon was that the ItemLoaders didn't care about other rows in the list. That's fixed now. Now the favicons are all right.

The other issue was that the old favicons are never cleared. Two reasons:
1. setImageResource(0) might not clear an ImageView.
http://stackoverflow.com/questions/2859212/how-to-clear-an-imageview-in-android
Which we are using in FaviconView.
2. If there is a failure in loading a favicon (like the db returned null), the ItemLoader never calls "displayItemPart()" with null or call an error handler. Smoothie needs some update.

Since it's a bit hard for me to create jars and not sure which path you like (call displayItemPart with null result / call an error handler), I just make sure clearImage() shows default favicon.

Performance note:
1. It's better to set the default favicon when the view is moved to heap. This way, we don't do a layout pass while scrolling.
2. The next time when the view is used, the favicon is default favicon -- just like what we want.

Comment 6

[Lucas Rocha (:lucasr)]

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

Review of attachment 812892 [details] [diff] [review]:
-----------------------------------------------------------------

The patch itself looks pretty good. I guess it all comes down to fixing the bugs in Smoothie now?

::: mobile/android/base/home/HomeListView.java
@@ +127,5 @@
>  
> +    public void setDefaultItemManager(ItemLoader<?, ?> itemLoader) {
> +        ItemManager.Builder builder = new ItemManager.Builder(itemLoader);
> +        builder.setPreloadItemsEnabled(true).setPreloadItemsCount(10);
> +        builder.setThreadPoolSize(3);

That looks a bit too aggressive. We should aim for the lowest possible numbers that give us the performance we want. For instance, I suspect that a pool with 2 threads is enough, no?

@@ +238,5 @@
> +        public Bitmap loadItem(String url) {
> +            Bitmap favicon = Favicons.getFaviconFromMemCache(url);
> +            if (favicon == null) {
> +                final ContentResolver cr = mContext.getContentResolver();
> +                final Bitmap faviconFromDb = BrowserDB.getFaviconForUrl(cr, url);

We'll have to use the new Favicon API here once it lands. We should probably hold on landing this patch until bug 914296 lands (which should be at any time now). But this is probably the code we want to uplift if we decide to get this patch in Aurora.

::: mobile/android/base/home/LastTabsPage.java
@@ +288,5 @@
> +            super(context);
> +        }
> +
> +        @Override
> +        public String getItemParams(Adapter adapter, int position) {

I wonder if this should be the default implementation of getItemParams() in HomeItemLoader to avoid some code duplication.

::: mobile/android/base/home/MostRecentPage.java
@@ +368,5 @@
> +    private static class MostRecentItemLoader extends HomeListView.HomeItemLoader {
> +        public MostRecentItemLoader(Context context) {
> +            super(context);
> +         }
> + 

nit: remove trailing space.

::: mobile/android/base/home/TopSitesPage.java
@@ +643,5 @@
>  
> +    private class VisitedItemLoader extends HomeListView.HomeItemLoader {
> +        public VisitedItemLoader(Context context) {
> +            super(context);
> +         }

nit: indentation is a bit off here.

@@ +644,5 @@
> +    private class VisitedItemLoader extends HomeListView.HomeItemLoader {
> +        public VisitedItemLoader(Context context) {
> +            super(context);
> +         }
> + 

nit: remove trailing space.

@@ +647,5 @@
> +         }
> + 
> +        @Override
> +        public String getItemParams(Adapter adapter, int position) {
> +            if (position < mMaxGridEntries) {

Is this actually necessary? Doesn't the adapter naturally cap the position for us? I mean, position 0 in the list is, in practice, position + mMaxGridEntries in the cursor. If Smoothie is not following the number of items provided by the adapter, this is probably a bug we need to fix.

::: mobile/android/base/home/TwoLinePageRow.java
@@ +174,5 @@
>          // No need to do extra work if the URL associated with this view
>          // hasn't changed.
> +        //if (TextUtils.equals(mPageUrl, url)) {
> +        //    return;
> +        // }

Remove?

::: mobile/android/base/widget/FaviconView.java
@@ +160,5 @@
>      /**
>       * Clear image and background shown by this view.
>       */
>      public void clearImage() {
> +        setImageResource(R.drawable.favicon);

Why not just keeping the image blank? Are you setting the default icon here just to show default icon while the user flings/scrolls the list? Wouldn't it cause the list to first show the default icon and then the actual icon?

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> Created attachment 812892 [details] [diff] [review]
> Patch
> 
> OMG FAST!
> This works fine now. The issue with wrong favicon was that the ItemLoaders
> didn't care about other rows in the list. That's fixed now. Now the favicons
> are all right.
> 
> The other issue was that the old favicons are never cleared. Two reasons:
> 1. setImageResource(0) might not clear an ImageView.
> http://stackoverflow.com/questions/2859212/how-to-clear-an-imageview-in-
> android
> Which we are using in FaviconView.
> 2. If there is a failure in loading a favicon (like the db returned null),
> the ItemLoader never calls "displayItemPart()" with null or call an error
> handler. Smoothie needs some update.

Interesting. My assumption has been that the default state would be set by the adapter and there was no need to call displayItem if the loading failed. But calling displayItemPart even on loading failure does make sense. I'll update it accordingly.

> Since it's a bit hard for me to create jars and not sure which path you like
> (call displayItemPart with null result / call an error handler), I just make
> sure clearImage() shows default favicon.

I think the right thing to do here is to leave clearImage() unchanged and change Smoothie to always call displayItem, even if loadItem returns null (so that you can then set the default favicon accordingly).

> Performance note:
> 1. It's better to set the default favicon when the view is moved to heap.
> This way, we don't do a layout pass while scrolling.
> 2. The next time when the view is used, the favicon is default favicon --
> just like what we want.

Interesting point but the listview will have to do an internal layout round while scrolling anyway because of view recycling i.e. it will keep detaching and re-attaching views as you scroll. The Adapter's getView() is usually called while the listview is recycling/relayouting its children. So, doing things that trigger re-layouts in the Adapter's getView() is fine because the generated child is not attached to its parent while it's being created or recycled in the adapter. In other words, calling clearImage() and the like in getView() is fine.

Comment 9

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

This bug is now focused on Fx27. Bug 925163 will handle Fx26.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Is this still seen? Shall we untrack it?

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #10)
> Is this still seen? Shall we untrack it?

What's blocking us from working on it?

Comment 12

[Sriram Ramasubramanian [:sriram]]

We are actually faster now, with the thumbnail/favicon improvement and drawing directly to Canvas patches.
At this point, we don't need smoothie. I can push the other patch.

Comment 13

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

ping

Comment 14

[Sriram Ramasubramanian [:sriram]]

pong
(will push it today)

Comment 15

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/fx-team/rev/6fe1a0a24617

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6fe1a0a24617