TabsTray's listview doesn't do proper view recycling

VERIFIED FIXED in Firefox 14

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
10 months ago

People

(Reporter: lucasr, Assigned: sriram)

Tracking

({perf})

Trunk
Firefox 15
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox14 verified, firefox15 verified, firefox16 verified, firefox17 verified, blocking-fennec1.0 soft, fennec15+)

Details

(Whiteboard: tabs-ux)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
It should use correctly convertView and viewholder patterns. Otherwise it will just have pretty bad scrolling performance when you have more than a couple of tabs. Furthermore, inflating views on each getView() call is not memory efficient.
(Assignee)

Updated

6 years ago
Assignee: nobody → sriram

Updated

6 years ago
Blocks: 749606

Updated

6 years ago
blocking-fennec1.0: --- → ?

Updated

6 years ago
Keywords: perf
tracking-fennec: --- → 15+
blocking-fennec1.0: ? → soft
Let's use the view holder pattern here
Duplicate of this bug: 749606

Updated

6 years ago
No longer blocks: 749606
(Assignee)

Comment 3

6 years ago
Created attachment 619187 [details] [diff] [review]
Patch

This reuses the views as given by the Android. This doesn't use ViewHolder pattern, as the "chances of recycling" happening in tabs-tray is very less. If ViewHolders are needed, I would probably suggest create custom classes and using them -- like I had suggested or other lists.
Attachment #619187 - Flags: review?(mark.finkle)
(Reporter)

Comment 4

6 years ago
Comment on attachment 619187 [details] [diff] [review]
Patch

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

Using View Holder pattern will give us 5-7fps scrolling performance gain. It's definitely worth it. I honestly think doing custom classes is overkill for something as simple as a viewholder class (which only has a couple of view properties anyway). The level of code reuse is not big enough to be worth splitting this code into completely separate classes.

::: mobile/android/base/TabsTray.java
@@ +297,5 @@
>              RelativeLayout info = (RelativeLayout) convertView.findViewById(R.id.info);
>              info.setTag(String.valueOf(tab.getId()));
>              info.setOnClickListener(mOnInfoClickListener);
>  
>              assignValues(convertView, tab);

You have to change the code that updates the close button visibility to always set visibility of the button because you're now recycling views.
Attachment #619187 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 5

6 years ago
@lucasr:
What exactly is the difference between a ViewHolder and CustomClass that we are talking here?
They both are going to have the "same" number of views in it. You just give an option to instantiate the class -- which takes care of "inflation". Instead of creating numerous anonymous ViewHolders, it's better to create a class that is easily recognizable and reusable. And, by all certainty (as I've tested for the other patches in Bug 744537), custom classes are not an overkill!

Custom classes are cleaner abstraction over confusing ViewHolders.

And.. We are talking about 5-7 fps scrolling performance increase on a list, that's going to have a max of 5 rows, of which 3 rows are going to be shown always, and the user is not going to constantly keep scrolling back and forth. What that translates to is, the "view recycling kicks in when the user keeps scrolling the list back and forth". Do we really need to over-optimize for this edge scenario? Is there any test that shows users use more than 3 tabs all the time on mobile? I remember the last test pilot showed users using 1 tab most of the time.

To me, custom classes are better than ViewHolders any day. And, striving for 5-7fps performance increment on a list with 5 entries at max is an overkill.
(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> To me, custom classes are better than ViewHolders any day. And, striving for
> 5-7fps performance increment on a list with 5 entries at max is an overkill.

I tested with 15-30 tabs scrolling back and forth a few times and it was painful; Fennec choked and died on OOM.
(Reporter)

Comment 7

6 years ago
(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> @lucasr:
> What exactly is the difference between a ViewHolder and CustomClass that we
> are talking here?
> They both are going to have the "same" number of views in it. You just give
> an option to instantiate the class -- which takes care of "inflation".
> Instead of creating numerous anonymous ViewHolders, it's better to create a
> class that is easily recognizable and reusable. And, by all certainty (as
> I've tested for the other patches in Bug 744537), custom classes are not an
> overkill!

The custom classes you're adding are overkill. You're adding two top-level dummy classes which simply replace one inflate() and a couple of findViewById() calls. This creates an unnecessary level of indirection in the code i.e. I'd have to look into 2 extra files to understand how each tab of AwesomeBarTabs work. Adapters are generally responsible for inflating and recycling views for AdapterViews. This is where you expect this kind of code to be anyway. Replacing viewHolder.titleText.setText() with view.setTitle() is not a good enough reason to create a whole new top-level dummy class.

I'd probably be more inclined to use custom views if the rows required more intelligence and were more complex to implement. But those are extreme cases.

If you feel that the view holder creation code duplication is non-ideal right now, you could easily just add a method to AwesomeBarTabs that inflates the layout, creates the view holder, and does all the findViewById() calls in one single place. You'd remove the code duplication and have a clearly named code chunk describing the operation. No need to add to add two top-level classes just for that.

As I said, a better (and maybe necessary) code split for AwesomeBarTabs right now is to have each tab implementation in its own class as we'll probably have to implement each tab as fragments in order to use ViewPager to get all the tab swiping goodness in the future.

> Custom classes are cleaner abstraction over confusing ViewHolders.

View Holder pattern is a clear standard in the Android dev community. There's nothing confusing about it really.

> And.. We are talking about 5-7 fps scrolling performance increase on a list,
> that's going to have a max of 5 rows, of which 3 rows are going to be shown
> always, and the user is not going to constantly keep scrolling back and
> forth. What that translates to is, the "view recycling kicks in when the
> user keeps scrolling the list back and forth". Do we really need to
> over-optimize for this edge scenario? Is there any test that shows users use
> more than 3 tabs all the time on mobile? I remember the last test pilot
> showed users using 1 tab most of the time.
> 
> To me, custom classes are better than ViewHolders any day. And, striving for
> 5-7fps performance increment on a list with 5 entries at max is an overkill.

I'd probably agree with you if we were talking about a secondary/rarely used part of our UI. However, the tabs tray is a core part of the UX and it has to be very robust. Furthermore, I expect user behaviour to be different on tablets. i.e. users will likely add more tabs than on phones. So, I still think we need view holder pattern for tabs.
(Assignee)

Comment 8

6 years ago
This is the ViewHolder pattern as mentioned in: http://developer.android.com/resources/samples/ApiDemos/src/com/example/android/apis/view/List14.html

public View getView(int position, View convertView, ViewGroup parent) {
    ViewHolder holder;

    if (convertView == null) {
        convertView = mInflater.inflate(R.layout.list_item_icon_text, null);

        holder = new ViewHolder();
        holder.text = (TextView) convertView.findViewById(R.id.text);
        holder.icon = (ImageView) convertView.findViewById(R.id.icon);

        convertView.setTag(holder);
    } else {
        holder = (ViewHolder) convertView.getTag();
    }

    // Bind the data efficiently with the holder.
    holder.text.setText(DATA[position]);
    holder.icon.setImageBitmap((position & 1) == 1 ? mIcon1 : mIcon2);

    return convertView;
}

static class ViewHolder {
    TextView text;
    ImageView icon;
}

---------

This is the change I am asking for:

public View getView(int position, View convertView, ViewGroup parent) {
    CustomClass holder;

    if (convertView == null) {
        holder = new CustomClass(getApplicationContext());
        convertView = holder;
    } else {
        holder = (CustomClass) convertView;
    }

    // Bind the data efficiently with the holder.
    holder.text.setText(DATA[position]);
    holder.icon.setImageBitmap((position & 1) == 1 ? mIcon1 : mIcon2);

    return convertView;
}

static class CustomClass {
    public TextView text;
    public ImageView icon;

    CustomClass(Context context) {
        LayoutInflater.from(context).inflate(R.layout.list_item_icon_text, null);

        text = (TextView) findViewById(R.id.text);
        icon = (ImageView) findViewById(R.id.icon);
    }
}

And if you look in closely,
"If you feel that the view holder creation code duplication is non-ideal right now, you could easily just add a method to AwesomeBarTabs that inflates the layout, creates the view holder, and does all the findViewById() calls in one single place." is what the constructor of the CustomClass does.

The only additional thing I would want is,

instead of : holder.text.setText("xyz"), 
I would like to have: holder.setTitle("xyz") or holder.setName("xyz") and so on, as the situation demands,

just to make it easier to comprehend in a faster skimming.

ViewHolder in android community is pretty prevalent. It's just an introduction and easy helper for people to start writing efficient code. I want to take it one step further and bring in "encapsulation" from OOPS to it. :)
(Assignee)

Comment 9

6 years ago
Created attachment 619717 [details] [diff] [review]
Patch

This patch uses ViewHolder pattern and avoids re-inflation by re-using views from convertView.

This patch also avoids the findViewById() used in the patch for Bug 750349.
Attachment #619187 - Attachment is obsolete: true
Attachment #619717 - Flags: review?(mark.finkle)
Attachment #619717 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 10

6 years ago
Created attachment 619721 [details] [diff] [review]
Patch: Option 2

This patch does the same thing as the previous one.
I've moved findViewById() into the constructor, and cleaned up some parts of the existing code.
Attachment #619721 - Flags: review?(mark.finkle)
Attachment #619721 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Updated

6 years ago
Attachment #619721 - Attachment description: Patch: Option 2 and a half → Patch: Option 2
(Reporter)

Comment 11

6 years ago
Comment on attachment 619721 [details] [diff] [review]
Patch: Option 2

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

::: mobile/android/base/TabsTray.java
@@ +65,5 @@
>  
>          mList.setRecyclerListener(new RecyclerListener() {
>              @Override
>              public void onMovedToScrapHeap(View view) {
> +                ((TabsRow) view.getTag()).thumbnail.setImageDrawable(null);

nit: I usually try to avoid those nested castings as they are kind of hard to read.

@@ +217,5 @@
>          }
>      }
>  
> +    // ViewHolder for a row in the list
> +    private class TabsRow {

Maybe TabRow is a better name?

@@ +224,5 @@
> +        ImageView thumbnail;
> +        ImageView selectedIndicator;
> +        ImageButton close;
> +
> +        public TabsRow() { }

Is this constructor actually required?

@@ +240,5 @@
> +        private Context mContext;
> +        private ArrayList<Tab> mTabs;
> +        private LayoutInflater mInflater;
> +        private View.OnClickListener mOnInfoClickListener;
> +        private Button.OnClickListener mOnCloseClickListener;

It would be nice if this change (moving properties up in the class) would be done in a separate patch as it's unrelated to the main point your patch.

@@ +300,5 @@
>              mTabs.remove(tab);
>          }
>  
> +        public void assignValues(TabsRow row, Tab tab) {
> +            if (row == null || tab == null)

Can the row or tab actually be null? In which cases?

@@ +324,5 @@
> +            if (mTabs.size() > 1) {
> +                row.close.setTag(String.valueOf(row.id));
> +                row.close.setVisibility(View.VISIBLE);
> +            } else {
> +                row.close.setVisibility(View.GONE);

Maybe move the setVisibility() call out of the 'if' like this?

  row.close.setVisibility(mTabs.size() > 1 ? View.VISIBLE : View.GONE);

Not a big deal though.

@@ +336,5 @@
> +                convertView = mInflater.inflate(R.layout.tabs_row, null);
> +                convertView.setOnClickListener(mOnInfoClickListener);
> +
> +                row = new TabsRow(convertView);
> +                row.close.setOnClickListener(mOnCloseClickListener);

Maybe move that line into TabsRow constructor?
Attachment #619721 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 619717 [details] [diff] [review]
Patch

let's use option 2
Attachment #619717 - Attachment is obsolete: true
Attachment #619717 - Flags: review?(mark.finkle)
Attachment #619717 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 619721 [details] [diff] [review]
Patch: Option 2

I agree with Lucas' review comments. His r+ is good enough.
Attachment #619721 - Flags: review?(mark.finkle) → feedback+
(Assignee)

Comment 14

6 years ago
Created attachment 620460 [details] [diff] [review]
Patch

Make changes to the patch as per review comments.
Attachment #619721 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
(In reply to Lucas Rocha (:lucasr) from comment #11)
> Comment on attachment 619721 [details] [diff] [review]
> 
> @@ +240,5 @@
> > +        private Context mContext;
> > +        private ArrayList<Tab> mTabs;
> > +        private LayoutInflater mInflater;
> > +        private View.OnClickListener mOnInfoClickListener;
> > +        private Button.OnClickListener mOnCloseClickListener;
> 
> It would be nice if this change (moving properties up in the class) would be
> done in a separate patch as it's unrelated to the main point your patch.
> 

I don't know to split patches. So I would like to make this small change with this patch itself, though irrelevant.

> @@ +300,5 @@
> >              mTabs.remove(tab);
> >          }
> >  
> > +        public void assignValues(TabsRow row, Tab tab) {
> > +            if (row == null || tab == null)
> 
> Can the row or tab actually be null? In which cases?

It's just a preventive check thats there for a long time now.

> 
> @@ +336,5 @@
> > +                convertView = mInflater.inflate(R.layout.tabs_row, null);
> > +                convertView.setOnClickListener(mOnInfoClickListener);
> > +
> > +                row = new TabsRow(convertView);
> > +                row.close.setOnClickListener(mOnCloseClickListener);
> 
> Maybe move that line into TabsRow constructor?

Do you mean the OnClickListeners? They are a part of TabsAdapter. Hence do it here.
Carrying forward the r+ from lucasr; landed on m-c:
https://hg.mozilla.org/mozilla-central/rev/e1bd646b8f44
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox14: --- → affected
status-firefox15: --- → fixed
Depends on: 750349
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Comment on attachment 620460 [details] [diff] [review]
Patch

[Triage Comment] blocking another blocker and it's a low risk
Attachment #620460 - Flags: approval-mozilla-aurora+

Updated

6 years ago
Status: RESOLVED → VERIFIED
status-firefox14: fixed → verified
status-firefox15: fixed → verified
status-firefox16: --- → verified
status-firefox17: --- → verified
You need to log in before you can comment on or make changes to this bug.