606148 - When Panorama starts to stack tabs, it becomes extremely slow

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

As a Panoraman bug this is really a blocker. We can't ship with this kind
of major performance problems.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

[Kevin Hanes]

This sounds pretty similar to 604213; I'll leave it up to Sean whether it's a dupe.

Comment 4

[Sean Dunn]

Attachment: 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.

Comment 5

[Ian Gilman [:iangilman]]

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.

Comment 6

[Sean Dunn]

Attachment: v2

Fixed per Ian's comments.

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 488471 [details] [diff] [review]
v2

Dietrich doesn't review bugs that aren't blockers; reassigning review to dolske.

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 488471 [details] [diff] [review]
v2

Switching review to myself.

Comment 9

[Ian Gilman [:iangilman]]

Comment on attachment 488471 [details] [diff] [review]
v2

Thanks!

Comment 10

[Ian Gilman [:iangilman]]

Comment on attachment 488471 [details] [diff] [review]
v2

Canceling approval now that we have blocking. Sean, please package/try.

Comment 11

[Sean Dunn]

Attachment: v3

Sent to try: http://hg.mozilla.org/try/rev/da671db91b7b

Comment 12

[Sean Dunn]

Try passed.

Comment 13

[Ian Gilman [:iangilman]]

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.

Comment 14

[Ian Gilman [:iangilman]]

Attachment: Ian's attempt at unrotting

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

Comment 15

[Sean Dunn]

Attachment: v4

I had to update a member var used in a test. Works now.

Comment 16

[Sean Dunn]

Actually hold off on this -- I need to unrot again.

Comment 17

[Sean Dunn]

Attachment: v5

Unrotted, pushed to try: http://hg.mozilla.org/try/rev/de40b6394e81

Comment 18

[(no longer active)]

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

Comment 19

[Sean Dunn]

Attachment: v6

Unrotted, and dependent upon changes in 567029.

Comment 20

[Ian Gilman [:iangilman]]

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?

Comment 21

[Sean Dunn]

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.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

I'm surprised to see this as a [softblocker].
Because of this I can't use Panorama.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

...because it takes ages to load it.

Comment 24

[Sean Dunn]

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.

Comment 25

[Michael Yoshitaka Erlewine [:mitcho]]

(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.

Comment 26

[Sean Dunn]

I'll take care of that today. And again, thanks for figuring out what was causing those leaks!

Comment 27

[Michael Yoshitaka Erlewine [:mitcho]]

softblocker = critical

Comment 28

[Tomas]

Any progress on This?

Comment 29

[Sean Dunn]

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.

Comment 30

[Michael Yoshitaka Erlewine [:mitcho]]

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?

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

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 :)

Comment 32

[Ian Gilman [:iangilman]]

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.

Comment 33

[Sean Dunn]

Attachment: v7

Unrotted, pushed to try: http://hg.mozilla.org/try/rev/6c2e8fe77c32

Comment 34

[Sean Dunn]

Resubmitted to try after errors found and fixed: http://hg.mozilla.org/try/rev/850984939555

Comment 35

[Sean Dunn]

Attachment: v8

Comment 36

[Sean Dunn]

Try passed except for errors that appear unrelated.

Comment 37

[Ian Gilman [:iangilman]]

So this is just waiting for packaging?

Comment 38

[Sean Dunn]

No, it currently depends on 567029, which has the creeping children problem. I hope to get that fixed as soon as I can.

Comment 39

[Sean Dunn]

Attachment: v9 for checkin

Unrotted

Comment 40

[Sean Dunn]

Attachment: v10

unrotted, ready for checkin.

Comment 41

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/89b83442d8c9

Comment 42

[Matt Wilkinson]

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.