Last Comment Bug 735636 - PromptService list view has bad scroll performance
: PromptService list view has bad scroll performance
Status: VERIFIED FIXED
: perf
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: Firefox 14
Assigned To: Lucas Rocha (:lucasr)
:
Mentors:
: 739394 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-14 04:50 PDT by Lucas Rocha (:lucasr)
Modified: 2012-04-04 10:08 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
+


Attachments
(1/3) Correctly use recycled view and apply ViewHolder pattern (3.85 KB, patch)
2012-03-21 07:34 PDT, Lucas Rocha (:lucasr)
wjohnston2000: review-
Details | Diff | Review
(2/3) Factor out icon update into separate method (3.82 KB, patch)
2012-03-21 07:34 PDT, Lucas Rocha (:lucasr)
no flags Details | Diff | Review
(3/3) Factor out checked state update into separate method (3.57 KB, patch)
2012-03-21 07:34 PDT, Lucas Rocha (:lucasr)
no flags Details | Diff | Review
Test case (4.80 KB, text/html)
2012-03-21 10:31 PDT, Wesley Johnston (:wesj)
no flags Details
(1/3) Correctly use recycled view and apply ViewHolder pattern (7.72 KB, patch)
2012-03-28 03:48 PDT, Lucas Rocha (:lucasr)
wjohnston2000: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
(2/3) Factor out icon update into separate method (3.92 KB, patch)
2012-03-28 03:48 PDT, Lucas Rocha (:lucasr)
wjohnston2000: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
(3/3) Factor out checked state update into separate method (3.46 KB, patch)
2012-03-28 03:49 PDT, Lucas Rocha (:lucasr)
wjohnston2000: review-
Details | Diff | Review
Factor out checked state update into separate method (3.57 KB, patch)
2012-03-28 10:09 PDT, Lucas Rocha (:lucasr)
wjohnston2000: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Lucas Rocha (:lucasr) 2012-03-14 04:50:02 PDT
It should be using convertView correctly (avoids inflating every row) and using a viewholder pattern to avoid calling findViewById too often.
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-16 07:17:56 PDT
You mean select for helper popups like this one: http://people.mozilla.org/~mwargers/tests/forms/select/selectoptions_alot.html
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-16 11:45:00 PDT
How bad is scroll performance? How big of a list?
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-16 15:16:46 PDT
On the Asus Transformer TF101 tablet, I've seen it causing the "Appliction doesn not respond, close it?" dialog to appear, while scrolling in the select form helper popup on the testcase.
Comment 4 Lucas Rocha (:lucasr) 2012-03-19 02:46:42 PDT
(In reply to Mark Finkle (:mfinkle) from comment #2)
> How bad is scroll performance? How big of a list?

It's not necessarily related to the size of the list. Basically, ListView recycles a lot of views while scrolling and the ListView adapter used in PromptService is not using the recycled view (aka convertView) and inflating rows on every getView() call.

It should be straightforward to change PromptService to use convertView and ViewHolder pattern.
Comment 5 Lucas Rocha (:lucasr) 2012-03-21 07:34:36 PDT
Created attachment 607945 [details] [diff] [review]
(1/3) Correctly use recycled view and apply ViewHolder pattern
Comment 6 Lucas Rocha (:lucasr) 2012-03-21 07:34:44 PDT
Created attachment 607946 [details] [diff] [review]
(2/3) Factor out icon update into separate method
Comment 7 Lucas Rocha (:lucasr) 2012-03-21 07:34:53 PDT
Created attachment 607947 [details] [diff] [review]
(3/3) Factor out checked state update into separate method
Comment 8 Wesley Johnston (:wesj) 2012-03-21 10:30:36 PDT
Comment on attachment 607945 [details] [diff] [review]
(1/3) Correctly use recycled view and apply ViewHolder pattern

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

Sounds like (from irc), we need to tweak this a bit.

::: mobile/android/base/PromptService.java
@@ +490,5 @@
> +            if (convertView == null) {
> +                int resourceId = mResourceId;
> +                if (item.isGroup) {
> +                    resourceId = R.layout.list_item_header;
> +                }

Docs say "Note: You should check that this view is non-null and of an appropriate type before using." How do we know whether the convertView we get is a header row or not? I guess we need to inform the Adapter of how many row types we have? I'll throw up a demo with a lot of rows including some header ones.
Comment 9 Wesley Johnston (:wesj) 2012-03-21 10:31:11 PDT
Created attachment 608014 [details]
Test case

This is just a long multiple select list with header rows scattered throughout.
Comment 10 Wesley Johnston (:wesj) 2012-03-21 16:45:07 PDT
Comment on attachment 607947 [details] [diff] [review]
(3/3) Factor out checked state update into separate method

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

::: mobile/android/base/PromptService.java
@@ +509,5 @@
> +        private void maybeUpdateCheckedState(int position, PromptListItem item, TextView t) {
> +            if (item.isGroup)
> +                return;
> +
> +            CheckedTextView ct = (CheckedTextView) t;

I'm not sure all of these are checkedTextViews (i.e. context menus). Return if this is null, but I have a feeling it will actually throw instead and the gigantic try catch I had before was saving us, so we might be better to do a fancier check.... (I like instanceOf, but blassey tells me if you're using instanceOf, you're doing it wrong)
Comment 11 Wesley Johnston (:wesj) 2012-03-26 14:06:47 PDT
*** Bug 739394 has been marked as a duplicate of this bug. ***
Comment 12 Lucas Rocha (:lucasr) 2012-03-28 02:52:31 PDT
(In reply to Wesley Johnston (:wesj) from comment #10)
> Comment on attachment 607947 [details] [diff] [review]
> (3/3) Factor out checked state update into separate method
> 
> Review of attachment 607947 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/PromptService.java
> @@ +509,5 @@
> > +        private void maybeUpdateCheckedState(int position, PromptListItem item, TextView t) {
> > +            if (item.isGroup)
> > +                return;
> > +
> > +            CheckedTextView ct = (CheckedTextView) t;
> 
> I'm not sure all of these are checkedTextViews (i.e. context menus). Return
> if this is null, but I have a feeling it will actually throw instead and the
> gigantic try catch I had before was saving us, so we might be better to do a
> fancier check.... (I like instanceOf, but blassey tells me if you're using
> instanceOf, you're doing it wrong)

IIUC, the view will always be a CheckedTextView if it's not a group because we're using Android's select_dialog_multichoice and select_dialog_singlechoice layout.
Comment 13 Lucas Rocha (:lucasr) 2012-03-28 03:48:37 PDT
Created attachment 610062 [details] [diff] [review]
(1/3) Correctly use recycled view and apply ViewHolder pattern
Comment 14 Lucas Rocha (:lucasr) 2012-03-28 03:48:51 PDT
Created attachment 610063 [details] [diff] [review]
(2/3) Factor out icon update into separate method
Comment 15 Lucas Rocha (:lucasr) 2012-03-28 03:49:03 PDT
Created attachment 610064 [details] [diff] [review]
(3/3) Factor out checked state update into separate method
Comment 16 Wesley Johnston (:wesj) 2012-03-28 09:16:04 PDT
Comment on attachment 610064 [details] [diff] [review]
(3/3) Factor out checked state update into separate method

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

::: mobile/android/base/PromptService.java
@@ +530,5 @@
> +        private void maybeUpdateCheckedState(int position, PromptListItem item, TextView t) {
> +            if (item.isGroup)
> +                return;
> +
> +            CheckedTextView ct = (CheckedTextView) t;

Context menus use android.R.layout.select_dialog_item which I don't think is a CheckedTextView.
Comment 17 Lucas Rocha (:lucasr) 2012-03-28 10:09:21 PDT
Created attachment 610181 [details] [diff] [review]
Factor out checked state update into separate method
Comment 18 Wesley Johnston (:wesj) 2012-03-28 10:11:26 PDT
Make sure you've tested this with some different select type popups and context menus!
Comment 21 Lucas Rocha (:lucasr) 2012-04-03 03:02:55 PDT
Comment on attachment 610062 [details] [diff] [review]
(1/3) Correctly use recycled view and apply ViewHolder pattern

Mobile only. Release blocker.
Comment 22 Lucas Rocha (:lucasr) 2012-04-03 03:03:03 PDT
Comment on attachment 610063 [details] [diff] [review]
(2/3) Factor out icon update into separate method

Mobile only. Release blocker.
Comment 23 Lucas Rocha (:lucasr) 2012-04-03 03:03:10 PDT
Comment on attachment 610181 [details] [diff] [review]
Factor out checked state update into separate method

Mobile only. Release blocker.
Comment 24 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-04-03 06:46:26 PDT
Regarding testcase http://people.mozilla.org/~mwargers/tests/forms/select/selectoptions_alot.html
This seems definitely fixed to me in yesterday's trunk build on the Samsung Galaxy Nexus.
Comment 25 Alex Keybl [:akeybl] 2012-04-03 15:09:14 PDT
Comment on attachment 610062 [details] [diff] [review]
(1/3) Correctly use recycled view and apply ViewHolder pattern

[Triage Comment]
Mobile only & blocking Fennec 1.0. Approved for Aurora 13.

Note You need to log in before you can comment on or make changes to this bug.