Last Comment Bug 719493 - Switching tabs is too slow (can take 5-10 seconds)
: Switching tabs is too slow (can take 5-10 seconds)
Status: VERIFIED FIXED
: perf
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 major (vote)
: Firefox 12
Assigned To: :Margaret Leibovic
:
Mentors:
: 720202 (view as bug list)
Depends on:
Blocks: 721214 721220
  Show dependency treegraph
 
Reported: 2012-01-19 10:36 PST by Olli Pettay [:smaug]
Modified: 2012-03-01 07:51 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
verified
beta+
11+


Attachments
WIP (18.22 KB, patch)
2012-01-22 22:34 PST, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch (18.38 KB, patch)
2012-01-23 10:02 PST, :Margaret Leibovic
mbrubeck: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2012-01-19 10:36:30 PST
I installed Nightly Native Fennec and was pleased to notice the bugs
I've reported earlier seem to be gone.
But this time tab switching is so slow that it makes using more than one tab very hard.
Comment 1 :Margaret Leibovic 2012-01-20 09:48:56 PST
This is likely the result of the same message passing slowness causing bug 719494 (in this case, we're waiting on Tab:Select/Tab:Selected). There may be a similar solution here, although we need to be careful to make sure the browser deck and the Java UI don't get out of sync.
Comment 2 Matt Brubeck (:mbrubeck) 2012-01-22 09:23:00 PST
*** Bug 720202 has been marked as a duplicate of this bug. ***
Comment 3 Aaron Train [:aaronmt] 2012-01-22 12:01:09 PST
Reproduced this on my Galaxy SII which is fairly new (~1.2Ghz) phone, switching tabs takes about ~5 seconds.
Comment 4 :Margaret Leibovic 2012-01-22 22:32:07 PST
Updating the summary, since this affects multiple devices.
Comment 5 :Margaret Leibovic 2012-01-22 22:34:27 PST
Created attachment 590632 [details] [diff] [review]
WIP

(This applies on top of my patch for bug 719494)

This seems to work mostly well, but I've gotten into a state where the gecko content is out of sync with the Java UI, so it needs some more work. I just wanted to get some feedback in case I'm doing anything obviously wrong!
Comment 6 :Margaret Leibovic 2012-01-23 10:02:07 PST
Created attachment 590762 [details] [diff] [review]
patch

Doh, I forgot to send a Tab:Selected message to gecko. No wonder it wasn't updating correctly!

I'm going to continue to test more, but this looks to be working now. One potential problem is that if gecko is really slow to respond, it will take a while for the content to update, so we still want to look into ways to make gecko respond faster.
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-23 11:36:03 PST
Can we do some timing/profiling of the message sending code path? Put some logging, with timestamps, in the code and see how long it takes for the ping-pong process to happen.
Comment 8 :Margaret Leibovic 2012-01-23 13:11:06 PST
(In reply to Mark Finkle (:mfinkle) from comment #7)
> Can we do some timing/profiling of the message sending code path? Put some
> logging, with timestamps, in the code and see how long it takes for the
> ping-pong process to happen.

I started to do this, but I realized that with these changes, we don't wait on getting a message back from Gecko anymore, so there isn't a roundtrip to measure for Tab:Close(d)/Tab:Select(ed). I could add logging to other message round-trips, like the Tab:Add(ed) code, though.
Comment 9 Matt Brubeck (:mbrubeck) 2012-01-23 15:52:58 PST
Comment on attachment 590762 [details] [diff] [review]
patch

Nice cleanup!

>     public Tab selectTab(int id) {
>         if (!tabs.containsKey(id))
>             return null;
>+
>+        final Tab tab = tabs.get(id);
>+
>+        if (tab.getURL().equals("about:home"))
>+            GeckoApp.mAppContext.showAboutHome();

Just to be defensive, I'd like to add a null check in here, like "if (tab == null) return;"

Since "id" might come from a JavaScript message, this will avoid crashing on race conditions where the tab was removed after the message was sent (or if buggy JavaScript just sends an invalid id).

>     public boolean isSelectedTab(Tab tab) {
>+        if (selectedTab == null)
>+            return false;
>+
>+        return tab.getId() == selectedTab.getId();
>     }

Is there a reason to do this instead of just "return tab == selectedTab;"?

r=mbrubeck with the above questions/comments addressed.  As a later follow-up, it might also be nice to factor out the mBrowserToolbar calls to somewhere nicer...
Comment 10 :Margaret Leibovic 2012-01-23 16:06:39 PST
(In reply to Matt Brubeck (:mbrubeck) from comment #9)
> >     public Tab selectTab(int id) {
> >         if (!tabs.containsKey(id))
> >             return null;
> >+
> >+        final Tab tab = tabs.get(id);
> >+
> >+        if (tab.getURL().equals("about:home"))
> >+            GeckoApp.mAppContext.showAboutHome();
> 
> Just to be defensive, I'd like to add a null check in here, like "if (tab ==
> null) return;"
> 
> Since "id" might come from a JavaScript message, this will avoid crashing on
> race conditions where the tab was removed after the message was sent (or if
> buggy JavaScript just sends an invalid id).

I'll have to do "return null;", since this method expects a Tab to be returned. However, I looked at all the places we call selectTab, and we never use the return value, so we should be fine (just need to be careful if we add more consumers later).

> >     public boolean isSelectedTab(Tab tab) {
> >+        if (selectedTab == null)
> >+            return false;
> >+
> >+        return tab.getId() == selectedTab.getId();
> >     }
> 
> Is there a reason to do this instead of just "return tab == selectedTab;"?

Hm, I guess I was worried about object comparison in Java not doing what I want and assumed I would need to add an equals method to Tab, but we are actually comparing the object instances here, so this does work (I just tested to confirm).
 
> r=mbrubeck with the above questions/comments addressed.  As a later
> follow-up, it might also be nice to factor out the mBrowserToolbar calls to
> somewhere nicer...

Yes, plenty of room for more clean-up :)
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-01-25 07:30:06 PST
https://hg.mozilla.org/mozilla-central/rev/fcec3385530b
Comment 13 :Margaret Leibovic 2012-01-25 10:31:56 PST
Comment on attachment 590762 [details] [diff] [review]
patch

[Approval Request Comment]
Improves tab responsiveness. Landed on m-c this morning, so it will be in Nightly tomorrow.
Comment 14 Alex Keybl [:akeybl] 2012-01-25 17:07:07 PST
Comment on attachment 590762 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Comment 16 Cristian Nicolae (:xti) 2012-03-01 07:51:38 PST
Verified fixed on:

Firefox 13.0a1 (2012-03-01)
20120301031135
http://hg.mozilla.org/mozilla-central/rev/1c3b291d0830

--
Device: Samsung Galaxy S2
OS: Android 2.3.4

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