627628 - be smarter about dispatching starved paints

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Timothy Nikkel (:tnikkel)]

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.

Comment 1

[Timothy Nikkel (:tnikkel)]

Bugs that we don't want to regress: bug 601547, bug 592093, bug 592954.

Comment 2

[Katelyn Gadd (:kael)]

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

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[Timothy Nikkel (:tnikkel)]

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd approve a final patch.

Comment 6

[Katelyn Gadd (:kael)]

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

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 8

[Katelyn Gadd (:kael)]

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

Comment 9

[Timothy Nikkel (:tnikkel)]

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.

Comment 10

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/273cb783edac

Comment 11

[Timothy Nikkel (:tnikkel)]

Backed this out to fix bug 635465
http://hg.mozilla.org/mozilla-central/rev/88afdccd3ba6

Comment 12

[Timothy Nikkel (:tnikkel)]

So in order to fix this again we need to fix bug 635465 in a better way, perhaps bug 635465, comment 12.

Comment 13

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

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)

Comment 14

[(no longer active)]

I'm not comfortable landing this on cedar...

Comment 15

[Timothy Nikkel (:tnikkel)]

This shouldn't land. It was backed out for causing a regression and we haven't figured out how to fix the regression yet.

Comment 16

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d1511c16ba

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/f4d1511c16ba