Last Comment Bug 627628 - be smarter about dispatching starved paints
: be smarter about dispatching starved paints
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: All Windows 7
: -- normal with 3 votes (vote)
: mozilla12
Assigned To: K. Gadd (:kael)
:
Mentors:
Depends on: 635465 716549
Blocks: 712883
  Show dependency treegraph
 
Reported: 2011-01-20 21:47 PST by Timothy Nikkel (:tnikkel)
Modified: 2012-01-14 03:37 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Throttle dispatching of starved paints so that time is allowed for processing input events (2.67 KB, patch)
2011-01-20 23:18 PST, K. Gadd (:kael)
no flags Details | Diff | Splinter Review
Throttle dispatching of starved paints so that time is allowed for processing input events (2) (2.73 KB, patch)
2011-01-24 18:00 PST, K. Gadd (:kael)
no flags Details | Diff | Splinter Review
Throttle dispatching of starved paints so that time is allowed for processing input events (3) (2.95 KB, patch)
2011-01-24 19:11 PST, K. Gadd (:kael)
roc: review+
Details | Diff | Splinter Review

Description Timothy Nikkel (:tnikkel) 2011-01-20 21:47:41 PST
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) 2011-01-20 21:56:41 PST
Bugs that we don't want to regress: bug 601547, bug 592093, bug 592954.
Comment 2 K. Gadd (:kael) 2011-01-20 23:18:52 PST
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.
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-23 15:11:31 PST
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) 2011-01-24 09:00:26 PST
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) (Exited; email my personal email if necessary) 2011-01-24 13:22:10 PST
I'd approve a final patch.
Comment 6 K. Gadd (:kael) 2011-01-24 18:00:26 PST
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.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-24 18:04:20 PST
--- 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 K. Gadd (:kael) 2011-01-24 19:11:36 PST
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.
Comment 9 Timothy Nikkel (:tnikkel) 2011-01-25 08:28:01 PST
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) 2011-01-25 15:36:11 PST
http://hg.mozilla.org/mozilla-central/rev/273cb783edac
Comment 11 Timothy Nikkel (:tnikkel) 2011-02-22 11:55:00 PST
Backed this out to fix bug 635465
http://hg.mozilla.org/mozilla-central/rev/88afdccd3ba6
Comment 12 Timothy Nikkel (:tnikkel) 2011-02-22 11:58:27 PST
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] 2011-02-23 18:28:39 PST
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 :Ehsan Akhgari 2011-03-25 21:31:04 PDT
I'm not comfortable landing this on cedar...
Comment 15 Timothy Nikkel (:tnikkel) 2011-03-25 23:31:19 PDT
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) 2012-01-13 17:31:43 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d1511c16ba
Comment 17 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-01-14 01:49:03 PST
https://hg.mozilla.org/mozilla-central/rev/f4d1511c16ba

Note You need to log in before you can comment on or make changes to this bug.