be smarter about dispatching starved paints

RESOLVED FIXED in mozilla12

Status

()

Core
Widget: Win32
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: tnikkel, Assigned: kael)

Tracking

Trunk
mozilla12
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Kevin diagnosed and found this issue. I'm just filing for him.

In bug 579488 (typing in twitter) nsWindow::DispatchStarvedPaints forces us to paint after every character typed. The paints take a long time. This forces us to paint more often, instead of coalescing paints, making things worse then they could be. We should be smarter about dispatching starved paints.

There are some thoughts about this (for Linux) in bug 602303.
(Reporter)

Comment 1

6 years ago
Bugs that we don't want to regress: bug 601547, bug 592093, bug 592954.
(Assignee)

Comment 2

6 years ago
Created attachment 505712 [details] [diff] [review]
Throttle dispatching of starved paints so that time is allowed for processing input events

I tried throttling the starved paint logic so that it will only dispatch a starved paint synchronously if it's been at least 50 milliseconds since the last paint completed. This drastically improves the responsiveness of the twitter input field for me without compromising the responsiveness of the sinatra network graph on GitHub (from one of the previously mentioned bug). This seems like a decent way to solve the problem.
I like this!

+  // The point in time at which the last paint completed. We use this to avoid
+  //  painting too rapidly in response to frequent input events.
+  mozilla::TimeStamp mLastPaintEndTime;

add "typedef mozilla::TimeStamp TimeStamp;" to the top of nsWindow to avoid mozilla:: prefixes.
(Reporter)

Comment 4

6 years ago
I'm not sure if this is too risky or not for FF4, but if we want it for FF4 we should get it landed soon so it can get testing.
I'd approve a final patch.
(Assignee)

Comment 6

6 years ago
Created attachment 506605 [details] [diff] [review]
Throttle dispatching of starved paints so that time is allowed for processing input events (2)

Patch revised based on feedback and tested locally.

Spent a bit trying to come up with a good automated test but could not come up with one that would perform well without a lot of effort. Andreas suggested that a manual test might be sufficient here.

My manual test case for this is, roughly:

1. Load up new twitter and expand the tweet input field so it is around 5 lines tall and horizontally covers the right pane of their UI. This makes paints expensive enough to reproduce the issue.
2. Rapidly spam keystrokes into the input field (holding down a key is not sufficient) for around 15 seconds, then stop. 

Without the patch, you should observe Firefox slowly processing each keystroke for multiple seconds after you stop typing, until it eventually becomes responsive again. With the patch, it should become responsive within a second of your last keystroke.
Attachment #505712 - Attachment is obsolete: true
Attachment #506605 - Flags: review?(roc)
--- a/widget/src/windows/nsWindow.h	Fri Jan 21 04:25:18 2011 +0100
+++ b/widget/src/windows/nsWindow.h	Mon Jan 24 17:54:29 2011 -0800
@@ -60,7 +60,12 @@
 #include "gfxWindowsSurface.h"
 #include "nsWindowDbg.h"
 #include "cairo.h"
-#include "nsITimer.h"
+#include "nsITimer.h"
+#include "mozilla/TimeStamp.h"
+
+typedef mozilla::TimeStamp TimeStamp;
+typedef mozilla::TimeDuration TimeDuration;

Sorry, we shouldn't pollute the global namespace like this. Put these typedefs inside the nsWindow class.
(Assignee)

Comment 8

6 years ago
Created attachment 506626 [details] [diff] [review]
Throttle dispatching of starved paints so that time is allowed for processing input events (3)

Ah, my mistake. Moved the typedefs into the correct place.
Attachment #506605 - Attachment is obsolete: true
Attachment #506626 - Flags: review?(roc)
Attachment #506605 - Flags: review?(roc)
Attachment #506626 - Flags: review?(roc) → review+
(Reporter)

Updated

6 years ago
Attachment #506626 - Flags: approval2.0?
Attachment #506626 - Flags: approval2.0? → approval2.0+
(Reporter)

Comment 9

6 years ago
I had to make one small change because we can call DispatchPendingEvents before we've had a paint, but otherwise green on try server. I'll push this later today.
(Reporter)

Comment 10

6 years ago
http://hg.mozilla.org/mozilla-central/rev/273cb783edac
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 635465
(Reporter)

Comment 11

6 years ago
Backed this out to fix bug 635465
http://hg.mozilla.org/mozilla-central/rev/88afdccd3ba6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 12

6 years ago
So in order to fix this again we need to fix bug 635465 in a better way, perhaps bug 635465, comment 12.
Comment on attachment 506626 [details] [diff] [review]
Throttle dispatching of starved paints so that time is allowed for processing input events (3)

(removing the patch approval for bookeeping)
Attachment #506626 - Flags: approval2.0+
I'm not comfortable landing this on cedar...
Whiteboard: not-ready
(Reporter)

Comment 15

6 years ago
This shouldn't land. It was backed out for causing a regression and we haven't figured out how to fix the regression yet.
(Reporter)

Updated

6 years ago
Blocks: 712883
(Reporter)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d1511c16ba
Depends on: 716549
Whiteboard: not-ready
https://hg.mozilla.org/mozilla-central/rev/f4d1511c16ba
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.