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

RESOLVED FIXED in mozilla1.9

Status

()

defect
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

({perf, polish})

Trunk
mozilla1.9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment)

Assignee

Description

11 years ago
Posted patch patchSplinter Review
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.
Attachment #317856 - Flags: review?(enndeakin)
Flags: wanted1.9.0.x?

Updated

11 years ago
Attachment #317856 - Flags: review?(enndeakin) → review+
Assignee

Comment 1

11 years ago
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.
Assignee

Updated

11 years ago
Attachment #317856 - Flags: approval1.9?
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.
Attachment #317856 - Flags: approval1.9? → approval1.9-
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.
Attachment #317856 - Flags: approval1.9- → approval1.9?
Is there a way to test this?  
Like, how do we know how big of a perf gain this is?  
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 on attachment 317856 [details] [diff] [review]
patch

a=beltzner1.9
Attachment #317856 - Flags: approval1.9? → approval1.9+
Assignee

Updated

11 years ago
Keywords: checkin-needed
mozilla/toolkit/content/widgets/scrollbox.xml 	1.34 
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Assignee

Updated

11 years ago
Flags: wanted1.9.0.x?
You need to log in before you can comment on or make changes to this bug.