723077 - Bad performance with a lot of options in a select list

Status: RESOLVED FIXED

Description

[Martijn Wargers (zombie)]

Attachment: testcase

See testcase, steps to reproduce:
- Tap on combobox
- Tap on one option 1 (or higher) in the select helper popup

Expected result:
- Select helper popup opens up immediately, after tapping on an option, the value in the combobox is changed immediately.

Actual result:
- Select helper popup opens up after 500ms or so, after tapping on an option, the value in the combobox is changed after 2s or so.

This was tested on the LG Optimus Black, Android 2.2.2, portrait mode.

On stock browser, this is much quicker. Although, it has to be said, that the stock browser has difficulty loading the page at all, it seems.

On my EeeTransformer TF101, Fennec seems to crash when quickly scrolling through the options in the select helper popup.

Comment 1

[Aaron Train [:aaronmt]]

Reproducible on the Galaxy Nexus, quite a slow-down. (Nightly 02/01 13.0a1).

Comment 2

[Aaron Train [:aaronmt]]

It seems like something is ballooning in memory in creating the select lists and causing what I continue to get below with decently sized select lists:

For example, just on the language selection menu on http://www.wikipedia.org/, triggers tons of these

E/dalvikvm-heap( 1884): Out of memory on a 316944-byte allocation.
I/dalvikvm( 1884): "GeckoLooper Thread" prio=5 tid=10 RUNNABLE
I/dalvikvm( 1884):   | group="main" sCount=0 dsCount=0 obj=0x41826118 self=0x1f3698
I/dalvikvm( 1884):   | sysTid=1896 nice=0 sched=0/0 cgrp=default handle=1365800
I/dalvikvm( 1884):   | schedstat=( 0 0 0 ) utm=3540 stm=285 core=1
I/dalvikvm( 1884):   at android.graphics.Bitmap.nativeCreate(Native Method)
I/dalvikvm( 1884):   at android.graphics.Bitmap.createBitmap(Bitmap.java:605)
I/dalvikvm( 1884):   at android.graphics.Bitmap.createBitmap(Bitmap.java:585)
I/dalvikvm( 1884):   at android.view.View.buildDrawingCache(View.java:10528)
I/dalvikvm( 1884):   at android.view.View.getDrawingCache(View.java:10378)
I/dalvikvm( 1884):   at android.view.ViewGroup.drawChild(ViewGroup.java:2756)
I/dalvikvm( 1884):   at android.widget.ListView.drawChild(ListView.java:3223)
I/dalvikvm( 1884):   at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2495)
I/dalvikvm( 1884):   at android.widget.AbsListView.dispatchDraw(AbsListView.java:2092)
I/dalvikvm( 1884):   at android.widget.ListView.dispatchDraw(ListView.java:3218)
I/dalvikvm( 1884):   at android.view.View.draw(View.java:10985)
I/dalvikvm( 1884):   at android.widget.AbsListView.draw(AbsListView.java:3398)
I/dalvikvm( 1884):   at android.view.ViewGroup.drawChild(ViewGroup.java:2899)
I/dalvikvm( 1884):   at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2495)
I/dalvikvm( 1884):   at android.view.View.draw(View.java:10883)
I/dalvikvm( 1884):   at android.view.ViewGroup.drawChild(ViewGroup.java:2899)
I/dalvikvm( 1884):   at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2495)
I/dalvikvm( 1884):   at android.view.ViewGroup.drawChild(ViewGroup.java:2897)
I/dalvikvm( 1884):   at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2495)
I/dalvikvm( 1884):   at android.view.ViewGroup.drawChild(ViewGroup.java:2897)
I/dalvikvm( 1884):   at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2495)
I/dalvikvm( 1884):   at android.view.ViewGroup.drawChild(ViewGroup.java:2897)
I/dalvikvm( 1884):   at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2495)
I/dalvikvm( 1884):   at android.view.View.draw(View.java:10883)
I/dalvikvm( 1884):   at android.widget.FrameLayout.draw(FrameLayout.java:450)
I/dalvikvm( 1884):   at com.android.internal.policy.impl.PhoneWindow$DecorView.draw(PhoneWindow.java:2106)
I/dalvikvm( 1884):   at android.view.ViewRootImpl.draw(ViewRootImpl.java:2005)
I/dalvikvm( 1884):   at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:1613)
I/dalvikvm( 1884):   at android.view.ViewRootImpl.handleMessage(ViewRootImpl.java:2418)
I/dalvikvm( 1884):   at android.os.Handler.dispatchMessage(Handler.java:99)
I/dalvikvm( 1884):   at android.os.Looper.loop(Looper.java:137)
I/dalvikvm( 1884):   at org.mozilla.gecko.GeckoAppShell$LooperThread.run(GeckoAppShell.java:185)
I/dalvikvm( 1884):

Comment 3

[Aaron Train [:aaronmt]]

Tried this out out curiosity on Chrome Beta, non issue. Tested with the same language menu selection on Wikipedia.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch

On my phone, the test case takes about 2 seconds on average, including 1.5 seconds looping through the <option> elements in JavaScript and 0.5 seconds of JSON generation / parsing and/or message-passing latency.

This patch does some micro-optimizations to the JS code, reducing the DOM walking time from 1.5 seconds to about 0.5 seconds (so the total time is reduced from 2 seconds to about 1 second):

* Inside the loop, swap the "if" cases so the more common one is first.
* Inline some functions.
* Pass isGroup/inGroup back to the caller to avoid computing them twice
* Cache the .length properties to avoid fetching them on each iteration

It would be nice to improve this further, but I think the next big step should be to speed up our JSON messaging code.

Comment 5

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a45cecfa8801

Marking this as fixed although there is still some noticeable slowness.  We can do further optimizations in follow-up bugs.

Comment 6

[Ed Morley [:emorley]]

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

Comment 7

[Aaron Train [:aaronmt]]

Aurora nomination?

Looks good on M-C.

Verified on M-C
Nightly (02/12) - Samsung Nexus S
Mozilla/5.0 (Android; Mobile: rv:13.0a1) Gecko/13.0a1 Firefox/13.0a1

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 596050 [details] [diff] [review]
patch

[Approval Request Comment]
User impact if declined: Performance when using some web forms.

Testing completed (on m-c, etc.): Landed on m-c February 10.

Risk to taking this patch (and alternatives if risky): Low-risk and mobile-only; this just refactors some functions for speed, with no functional changes.

String changes made by this patch: None.

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 596050 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora 12.