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)
Tracking
(firefox11 fixed, firefox12 fixed, firefox13 verified, blocking-fennec1.0 beta+, fennec11+)
VERIFIED
FIXED
Firefox 12
People
(Reporter: smaug, Assigned: Margaret)
References
Details
(Keywords: perf)
Attachments
(1 file, 1 obsolete file)
18.38 KB,
patch
|
mbrubeck
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•14 years ago
|
OS: Linux → Android
Hardware: x86_64 → ARM
Assignee | ||
Comment 1•14 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•14 years ago
|
Assignee: nobody → margaret.leibovic
Comment 3•14 years ago
|
||
Reproduced this on my Galaxy SII which is fairly new (~1.2Ghz) phone, switching tabs takes about ~5 seconds.
Updated•14 years ago
|
Severity: normal → major
tracking-fennec: --- → ?
status-firefox11:
--- → affected
status-firefox12:
--- → affected
Keywords: perf
Assignee | ||
Comment 4•14 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•14 years ago
|
||
(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•14 years ago
|
||
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)
Comment 7•14 years ago
|
||
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•14 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•14 years ago
|
tracking-fennec: ? → 11+
Priority: -- → P1
Comment 9•14 years ago
|
||
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•14 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•14 years ago
|
||
Comment 12•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Assignee | ||
Comment 13•14 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?
Comment 14•14 years ago
|
||
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•14 years ago
|
||
Updated•14 years ago
|
Keywords: fennecnative-betablocker
Updated•13 years ago
|
blocking-fennec1.0: --- → beta+
Comment 16•13 years ago
|
||
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
Updated•5 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•