Closed Bug 719493 Opened 14 years ago Closed 14 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect, P1)

ARM
Android

Tracking

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

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- fixed
firefox12 --- fixed
firefox13 --- verified
blocking-fennec1.0 --- beta+
fennec 11+ ---

People

(Reporter: smaug, Assigned: Margaret)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

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.
OS: Linux → Android
Hardware: x86_64 → ARM
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: nobody → margaret.leibovic
Reproduced this on my Galaxy SII which is fairly new (~1.2Ghz) phone, switching tabs takes about ~5 seconds.
Severity: normal → major
tracking-fennec: --- → ?
Keywords: perf
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)
Attached patch WIP (obsolete) — Splinter Review
(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)
Attached patch patchSplinter Review
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.
(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.
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+
(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 :)
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
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?
Blocks: 721214
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+
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: