When Panorama starts to stack tabs, it becomes extremely slow

VERIFIED FIXED in Firefox 4.0b11

Status

Firefox Graveyard
Panorama
P2
critical
VERIFIED FIXED
7 years ago
2 years ago

People

(Reporter: smaug, Assigned: Sean Dunn)

Tracking

({perf})

unspecified
Firefox 4.0b11
x86
Linux
Dependency tree / graph

Details

(Whiteboard: [softblocker][fx4-fixed-bugday])

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

7 years ago
If I have lots of tabs and Panorama starts to stack them, everything
becomes very very slow.
I mean I have to wait 15+ seconds on my i7 machine to do anything when
stacking tabs are being painted or something.
It feels like there is a real problem in the algorithm being used.
O(n^x) or even O(x^n) algorithms?

(So far I've spent over an hour to recover from the mess
Panorama caused by hiding or re-arranging tabs randomly.)
(Reporter)

Comment 1

7 years ago
As a Panoraman bug this is really a blocker. We can't ship with this kind
of major performance problems.
Severity: normal → blocker
blocking2.0: --- → ?
(Reporter)

Comment 2

7 years ago
Based on sysprof Panorama does something which X Server really can't handle
well.
But anyhow, this bug prevents me to use Panorama on Linux.

Comment 3

7 years ago
This sounds pretty similar to 604213; I'll leave it up to Sean whether it's a dupe.
Assignee: nobody → seanedunn
Blocks: 604213
Priority: -- → P2
(Assignee)

Comment 4

7 years ago
Created attachment 487253 [details] [diff] [review]
v1

Replaced most if not all iQ queries for tabitem sub-elements with stored object members queried in the constructor.
Stacking code now just shows the top 6 tabitems. All tabitems which aren't shown are hidden, and their positions are not updated until they're shown again.
Attachment #487253 - Flags: feedback?(ian)
Comment on attachment 487253 [details] [diff] [review]
v1

Lovely. Comments:

* I know it's only being used in this one context at the moment, but as long as we're adding .hidden to TabItem, we might as well think about it in terms of a general API. I'd recommend hide and show methods on TabItem that take care of the class adding/removing, so if other parts of the system want to use this functionality they don't have to think about what's involved. Also, this leaves the door open for more intelligence in the future; say we want to skip all DOM manipulation in setBounds if the tab is hidden and then update the DOM just before reshowing. 

>-    iQ(tab.canvasEl)
>+    tab.$canvas
>     .unbind("mousedown", TabHandlers._hideHandler)
>     .unbind("mouseup", TabHandlers._showHandler);
> 
>-    iQ(tab.canvasEl)
>+    tab.$canvas
>     .mousedown(TabHandlers._hideHandler)
>     .mouseup(TabHandlers._showHandler);
>   },
>   
>   onUnmatch: function(tab, index){
>     iQ(tab.container).removeClass("onTop");
>     tab.removeClass("notMainMatch");
> 
>-    iQ(tab.canvasEl)
>+    tab.$canvas
>      .unbind("mousedown", TabHandlers._hideHandler)
>      .unbind("mouseup", TabHandlers._showHandler);

Nit: I know this isn't your fault, but as long as you're in there, could you fix the indentation of those .unbind and .mouse calls? They should be one indent in from the tab.$canvas above them.
Attachment #487253 - Flags: feedback?(ian) → feedback+
(Assignee)

Comment 6

7 years ago
Created attachment 488471 [details] [diff] [review]
v2

Fixed per Ian's comments.
Attachment #487253 - Attachment is obsolete: true
Attachment #488471 - Flags: review?(dietrich)
Comment on attachment 488471 [details] [diff] [review]
v2

Dietrich doesn't review bugs that aren't blockers; reassigning review to dolske.
Attachment #488471 - Flags: review?(dietrich) → review?(dolske)
Comment on attachment 488471 [details] [diff] [review]
v2

Switching review to myself.
Attachment #488471 - Flags: review?(dolske) → review?(ian)
Comment on attachment 488471 [details] [diff] [review]
v2

Thanks!
Attachment #488471 - Flags: review?(ian)
Attachment #488471 - Flags: review+
Attachment #488471 - Flags: approval2.0?
blocking2.0: ? → final+
Keywords: perf
Comment on attachment 488471 [details] [diff] [review]
v2

Canceling approval now that we have blocking. Sean, please package/try.
Attachment #488471 - Flags: approval2.0?
(Assignee)

Comment 11

7 years ago
Created attachment 492889 [details] [diff] [review]
v3

Sent to try: http://hg.mozilla.org/try/rev/da671db91b7b
Attachment #488471 - Attachment is obsolete: true
(Assignee)

Comment 12

7 years ago
Try passed.
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
I've tried landing this today (sorry for the delay... been out for Thanksgiving), and it had a bit of bit-rot, which I patched up. Just to be sure, I ran a local test pass, and got a whole pile of failures (see below). So, maybe I didn't un-rot the patch as well as I thought, or maybe something else changed that isn't mixing well with this patch. At any rate, Sean, can you look into it?

The test failures:

EST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug595560.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug595560.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug595560.js | Found a tab after previous test timed out: http://mochi.test:8888/
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug595804.js | Tab View is not visible
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug595804.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug595930.js | Tab View is hidden
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug595943.js | Tab View is hidden because we clicked on the app tab
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug595943.js | Tab View remains hidden
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_bug595943.js | we finish with Tab View hidden
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_dragdrop.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_dragdrop.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_dragdrop.js | Found a tab after previous test timed out: about:blank
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_exit_button.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_group.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_group.js | Tab item exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_group.js | The number of children in new tab group is increased by 1 - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_launch.js | Tab View is hidden. Count: 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_snapping.js | There is only one group - Got 2, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_startup_transitions.js | By default, we animate on zoom.
Created attachment 493784 [details] [diff] [review]
Ian's attempt at unrotting

Here's my attempt at unrotting, in case it's of use.
(Assignee)

Comment 15

7 years ago
Created attachment 494027 [details] [diff] [review]
v4

I had to update a member var used in a test. Works now.
Attachment #492889 - Attachment is obsolete: true
(Assignee)

Comment 16

7 years ago
Actually hold off on this -- I need to unrot again.
(Assignee)

Comment 17

7 years ago
Created attachment 494051 [details] [diff] [review]
v5

Unrotted, pushed to try: http://hg.mozilla.org/try/rev/de40b6394e81
Attachment #494027 - Attachment is obsolete: true

Comment 18

7 years ago
This patch doesn't apply cleanly on trunk:

ehsanakhgari:~/moz/mozilla-central [11:40:52]$ hg qpush && hg qfi . && hg push
applying attachment.cgi?id=494051
patching file browser/base/content/tabview/groupitems.js
Hunk #6 FAILED at 1120
Hunk #7 FAILED at 1158
Hunk #8 FAILED at 1227
3 out of 9 hunks FAILED -- saving rejects to file browser/base/content/tabview/groupitems.js.rej
patching file browser/base/content/tabview/tabitems.js
Hunk #1 FAILED at 64
Hunk #2 succeeded at 109 with fuzz 2 (offset 1 lines).
Hunk #5 succeeded at 216 with fuzz 2 (offset -38 lines).
Hunk #6 FAILED at 272
Hunk #7 FAILED at 359
Hunk #8 FAILED at 388
Hunk #9 FAILED at 423
Hunk #10 succeeded at 472 with fuzz 2 (offset -31 lines).
Hunk #15 FAILED at 897
6 out of 16 hunks FAILED -- saving rejects to file browser/base/content/tabview/tabitems.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh attachment.cgi?id=494051
Keywords: checkin-needed
(Assignee)

Updated

7 years ago
Depends on: 567029
(Assignee)

Comment 19

7 years ago
Created attachment 495032 [details] [diff] [review]
v6

Unrotted, and dependent upon changes in 567029.
Attachment #493784 - Attachment is obsolete: true
Attachment #494051 - Attachment is obsolete: true
Sean, Ehsan removed the checkin-needed flag when he asked you to unrot; if it's ready to go now, please re-add the flag.

Also, you mention above that you sent it to try, but not whether the try was successful... was it?
(Assignee)

Comment 21

7 years ago
It was *mostly* successful. Linux 64 opt crashed and burned, and Win debug had leaks. I don't recommend checking this in until I've resolved all the problems I've been having with leaks and crashes in my other bugs. I'll update this bug when things are looking good, hopefully soon after this weekend.

Updated

7 years ago
Severity: blocker → normal
Status: NEW → ASSIGNED
Whiteboard: [softblocker]
(Reporter)

Comment 22

7 years ago
I'm surprised to see this as a [softblocker].
Because of this I can't use Panorama.
(Reporter)

Comment 23

7 years ago
...because it takes ages to load it.
(Assignee)

Comment 24

7 years ago
There was a recent leak that mitcho found (and verified resolved!) that was affecting this landing. I'll try to get this in, since I agree, it makes things very slow.
(In reply to comment #24)
> There was a recent leak that mitcho found (and verified resolved!) that was
> affecting this landing. I'll try to get this in, since I agree, it makes things
> very slow.

Oh, huh. I didn't realize this bug was blocked on a leak too. Sean, have you tried applying this to current m-c and pushing to try? I'd be happy to try if you are busy.
(Assignee)

Comment 26

7 years ago
I'll take care of that today. And again, thanks for figuring out what was causing those leaks!
(Assignee)

Updated

7 years ago
Blocks: 617454
softblocker = critical
Severity: normal → critical

Comment 28

7 years ago
Any progress on This?
(Assignee)

Comment 29

7 years ago
Yes, it's been unrotted, but there's a glitch in the way at the moment, in addition to some other bugs I'm juggling right now. I hope to get a new version attached and sent to try tonight.
I think a large part of the benefits of this patch are also in bug 622835, which just landed today. Tomas, please try a nightly after tonight (or maybe in two days) and see if this performance improves. You'll notice a difference because the stacking of tabs will no longer have a "random" appearance and instead look like an orderly fan.

Sean, bug 622835 seems to take care of much of this already, including getting rid of randRotate and using something like angleAccum for positioning (though not called that there). The only substantial change to carry over from your work here now would be the ._hidden support which, I can see, would further help performance, though I don't know that it would be a huge impact.

Finally, Sean, a huge part of your patch seems to be the replacement of things like canvasEl with $canvas properties. This seems like a good idea in the long run, but I think touches too many moving parts at this point in the game, and isn't really related to the task at hand. I opened a new bug for this, for post-fx4: bug 626881.

Given this post-622835 situation, I nominate un-(blocking on this bug). Ian, thoughts?
Depends on: 622835
Just to jump in:

Bug 622835 is more a display thing. I think it does not improve the performance that much because it still sets the rotation for 50 tabs if there are 50 (just like the version before). The real gain (I think) would be to have the first 6 items rotated and the tabs behind that hidden so that Firefox does not apply css transforms on or even display them.

To conclude: IMHO the main part of this patch here is still valid, just needs some unrotting :)
I agree with Tim. Also, as discussed in IRC, the iQ change is not insignificant in terms of performance, and since its already in this patch, let's keep it there.
(Assignee)

Comment 33

7 years ago
Created attachment 505275 [details] [diff] [review]
v7

Unrotted, pushed to try: http://hg.mozilla.org/try/rev/6c2e8fe77c32
Attachment #495032 - Attachment is obsolete: true
(Assignee)

Comment 34

7 years ago
Resubmitted to try after errors found and fixed: http://hg.mozilla.org/try/rev/850984939555
(Assignee)

Comment 35

7 years ago
Created attachment 505288 [details] [diff] [review]
v8
Attachment #505275 - Attachment is obsolete: true
(Assignee)

Comment 36

7 years ago
Try passed except for errors that appear unrelated.
So this is just waiting for packaging?
(Assignee)

Comment 38

7 years ago
No, it currently depends on 567029, which has the creeping children problem. I hope to get that fixed as soon as I can.
Blocks: 627096
(Assignee)

Comment 39

7 years ago
Created attachment 506473 [details] [diff] [review]
v9 for checkin

Unrotted
Attachment #505288 - Attachment is obsolete: true
(Assignee)

Comment 40

7 years ago
Created attachment 506860 [details] [diff] [review]
v10

unrotted, ready for checkin.
Attachment #506473 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [softblocker] → [softblocker][land 567029 before this one]

Comment 41

7 years ago
http://hg.mozilla.org/mozilla-central/rev/89b83442d8c9
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [softblocker][land 567029 before this one] → [softblocker]
Target Milestone: --- → Firefox 4.0b11
Status: RESOLVED → VERIFIED
Whiteboard: [softblocker] → [softblocker][fx4-fixed-bugday]

Comment 42

7 years ago
I have verified that this bug is fixed on Firefox 4.0 Beta 11. I'm running Fedora 14 x86_64. I've opened a dozen or more tabs inside of a tab group and shrunk the tab group down to a stack. I haven't noticed any significant slowness in the stack or when fanning out the tabs.
Depends on: 634558
Depends on: 676803
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.