Switching tabs is too slow (can take 5-10 seconds)

VERIFIED FIXED in Firefox 11

Status

()

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

People

(Reporter: smaug, Assigned: Margaret)

Tracking

({perf})

unspecified
Firefox 12
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, firefox13 verified, blocking-fennec1.0 beta+, fennec11+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

5 years ago
OS: Linux → Android
Hardware: x86_64 → ARM
(Assignee)

Comment 1

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

Updated

5 years ago
Assignee: nobody → margaret.leibovic
Duplicate of this bug: 720202
Reproduced this on my Galaxy SII which is fairly new (~1.2Ghz) phone, switching tabs takes about ~5 seconds.

Updated

5 years ago
Severity: normal → major
tracking-fennec: --- → ?
status-firefox11: --- → affected
status-firefox12: --- → affected
Keywords: perf
(Assignee)

Comment 4

5 years ago
Updating the summary, since this affects multiple devices.
Summary: Switching tabs takes 5-10 seconds on tablet (Asus Transformer) → Switching tabs is too slow (can take 5-10 seconds)
(Assignee)

Comment 5

5 years ago
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!
Attachment #590632 - Flags: feedback?(mbrubeck)
(Assignee)

Comment 6

5 years ago
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.
Attachment #590632 - Attachment is obsolete: true
Attachment #590632 - Flags: feedback?(mbrubeck)
Attachment #590762 - Flags: review?(mbrubeck)
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.
(Assignee)

Comment 8

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

Updated

5 years ago
tracking-fennec: ? → 11+
Priority: -- → P1
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...
Attachment #590762 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 10

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

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcec3385530b
https://hg.mozilla.org/mozilla-central/rev/fcec3385530b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
(Assignee)

Comment 13

5 years ago
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.
Attachment #590762 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Blocks: 721214
(Assignee)

Updated

5 years ago
Blocks: 721220
Comment on attachment 590762 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #590762 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0b942692f5f
status-firefox11: affected → fixed
status-firefox12: affected → fixed

Updated

5 years ago
Keywords: fennecnative-betablocker
blocking-fennec1.0: --- → beta+
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
Status: RESOLVED → VERIFIED
status-firefox13: --- → verified
You need to log in before you can comment on or make changes to this bug.