430925 - Scrollbox smooth scrolling should skip frames when missing the desired frame rate

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

STR 1:
1. Restore a browser session with a lot of tabs (~80 in my case).
2. While the tabs are loading, scroll from the first tab to the last tab or vice versa by pressing Ctrl+1 / Ctrl+9.

Expected results: It takes a reasonable time until the target tab gets visible, even though the animation might not be smooth.

Actual results: Every single frame is processed with a relatively huge delay. The whole process can take like 10 seconds, depending on your hardware.


STR 2:
1. Use a low-end computer or throttle your CPU (600 MHz in my case).
2. Open a browser window with a lot of tabs (~80 in my case).
3. When the CPU is idle, scroll from the first tab to the last tab or vice versa by pressing Ctrl+1 / Ctrl+9.

Expected results: It takes a guaranteed time until the target tab gets visible, even though the animation might not be smooth.

Actual results: Every single frame is processed with a noticeable delay. The whole process can take twice as long as desired, depending on your hardware.

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 317856 [details] [diff] [review]
patch

Requesting approval for a pretty isolated performance tweak.

It's so isolated that I think we could do it in a dot release, although I suppose that's not any better than doing it now.

Comment 2

[Damon Sicore (:damons)]

Comment on attachment 317856 [details] [diff] [review]
patch

Trying to wrap things up, and I'm going to take your comment to heart that we should do this in the dot release if at all possible.  So, a-'ing this for now.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 317856 [details] [diff] [review]
patch

If we're OK with taking it in a dot release, we should take it now. The regression risks are the same either way (and if it causes a regression that ends up delaying Firefox 3, then at least we'll have spared our users that pain - but I think that's extremely unlikely). This fixes a noticeable performance issue with a new feature in Firefox 3.

Comment 4

[Damon Sicore (:damons)]

Is there a way to test this?  

Comment 5

[Damon Sicore (:damons)]

Like, how do we know how big of a perf gain this is?  

Comment 6

[:Gavin Sharp [email: [e-mail]]]

This patch makes us progressively disable the tab bar smooth scroll effect to make scrolling more responsive if we detect that we aren't able to do the animation in a reasonable amount of time (e.g. when the CPU is pegged while restoring a session). With lots of tabs open you can get into this situation a lot, and this patch puts an upper bound on the amount of time tab switching can take. 

I don't think there's an easy way to measure the "perf gain", since it depends entirely on how overloaded the system is (and thus how long the animation is taking). I tested the patch with STR1 from comment 0, and it makes a noticeable difference (tab switching doesn't get lagged even when many pages are loading at once). 

I think we can probably test this functionality in general, by dispatching scroll events on the tabstrip and checking that the tab bar is correctly scrolled. That would be really testing anything specific to the changes in this bug, though.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 317856 [details] [diff] [review]
patch

a=beltzner1.9

Comment 8

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/content/widgets/scrollbox.xml 	1.34