PromptService list view has bad scroll performance

VERIFIED FIXED in Firefox 13

Status

()

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

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

({perf})

Trunk
Firefox 14
All
Android
Points:
---

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed, blocking-fennec1.0 +)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
It should be using convertView correctly (avoids inflating every row) and using a viewholder pattern to avoid calling findViewById too often.

Updated

5 years ago
Keywords: perf

Updated

5 years ago
blocking-fennec1.0: --- → ?
You mean select for helper popups like this one: http://people.mozilla.org/~mwargers/tests/forms/select/selectoptions_alot.html
How bad is scroll performance? How big of a list?
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.
(Assignee)

Comment 4

5 years ago
(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.
Assignee: nobody → lucasr.at.mozilla
blocking-fennec1.0: ? → +
(Assignee)

Comment 5

5 years ago
Created attachment 607945 [details] [diff] [review]
(1/3) Correctly use recycled view and apply ViewHolder pattern
Attachment #607945 - Flags: review?(wjohnston)
(Assignee)

Comment 6

5 years ago
Created attachment 607946 [details] [diff] [review]
(2/3) Factor out icon update into separate method
Attachment #607946 - Flags: review?(wjohnston)
(Assignee)

Comment 7

5 years ago
Created attachment 607947 [details] [diff] [review]
(3/3) Factor out checked state update into separate method
Attachment #607947 - Flags: review?(wjohnston)
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.
Attachment #607945 - Flags: review?(wjohnston) → review-
Created attachment 608014 [details]
Test case

This is just a long multiple select list with header rows scattered throughout.
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)
Duplicate of this bug: 739394
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
Created attachment 610062 [details] [diff] [review]
(1/3) Correctly use recycled view and apply ViewHolder pattern
Attachment #610062 - Flags: review?(wjohnston)
(Assignee)

Updated

5 years ago
Attachment #607945 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 610063 [details] [diff] [review]
(2/3) Factor out icon update into separate method
Attachment #610063 - Flags: review?(wjohnston)
(Assignee)

Updated

5 years ago
Attachment #607946 - Attachment is obsolete: true
Attachment #607946 - Flags: review?(wjohnston)
(Assignee)

Comment 15

5 years ago
Created attachment 610064 [details] [diff] [review]
(3/3) Factor out checked state update into separate method
Attachment #610064 - Flags: review?(wjohnston)
(Assignee)

Updated

5 years ago
Attachment #607947 - Attachment is obsolete: true
Attachment #607947 - Flags: review?(wjohnston)
Attachment #610062 - Flags: review?(wjohnston) → review+
Attachment #610063 - Flags: review?(wjohnston) → review+
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.
Attachment #610064 - Flags: review?(wjohnston) → review-
(Assignee)

Comment 17

5 years ago
Created attachment 610181 [details] [diff] [review]
Factor out checked state update into separate method
Attachment #610181 - Flags: review?(wjohnston)
(Assignee)

Updated

5 years ago
Attachment #610064 - Attachment is obsolete: true
Attachment #610181 - Flags: review?(wjohnston) → review+
Make sure you've tested this with some different select type popups and context menus!
(Assignee)

Comment 19

5 years ago
Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/96fcc34dbbc9
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb20be085b35
http://hg.mozilla.org/integration/mozilla-inbound/rev/130e702908ca
https://hg.mozilla.org/mozilla-central/rev/96fcc34dbbc9
https://hg.mozilla.org/mozilla-central/rev/bb20be085b35
https://hg.mozilla.org/mozilla-central/rev/130e702908ca
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
(Assignee)

Comment 21

5 years ago
Comment on attachment 610062 [details] [diff] [review]
(1/3) Correctly use recycled view and apply ViewHolder pattern

Mobile only. Release blocker.
Attachment #610062 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 22

5 years ago
Comment on attachment 610063 [details] [diff] [review]
(2/3) Factor out icon update into separate method

Mobile only. Release blocker.
Attachment #610063 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 23

5 years ago
Comment on attachment 610181 [details] [diff] [review]
Factor out checked state update into separate method

Mobile only. Release blocker.
Attachment #610181 - Flags: approval-mozilla-aurora?
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.
Status: RESOLVED → VERIFIED
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.
Attachment #610062 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #610063 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #610181 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/a6515bb070f4
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2ababf0ea20
https://hg.mozilla.org/releases/mozilla-aurora/rev/c883b789280f
status-firefox13: --- → fixed
status-firefox14: --- → fixed
You need to log in before you can comment on or make changes to this bug.