Send the pending input event ack before the UpdateRect message

Send the pending input event ack before the UpdateRect message

This can potentially cut the mouse move, mouse wheel, and touch move
latency for platforms that send the UpdateRect_ACK message within the
UpdateRect IPC handler. Today, it seems that windows and mac will
sometimes send the UpdateRect_ACK synchronously, while linux never will.

Patch also includes some extra instrumentation to make this easier to
track that we may or may not want to land.

BUG=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114141

[jamesr, 2011-12-06 02:02:49]

This probably isn't quite good to land - I've only tested on linux where there
is no benefit since the UpdateRect_ACK is deferred anyway.

I have a test page for this internally, will put this up on my public hosting
and post a link once I do.

[jamesr, 2011-12-06 02:23:47]

On 2011/12/06 02:02:49, jamesr wrote:
> I have a test page for this internally, will put this up on my public hosting
> and post a link once I do.

http://webstuff.nfshost.com/examples/canvas-mouse-latency.html

That page does a draw and a busywait for 100ms in the mousemove handler.

[darin (slow to review), 2011-12-06 23:19:48]

LGTM

http://codereview.chromium.org/8820001/diff/1/content/renderer/render_widget.cc
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/8820001/diff/1/content/renderer/render_widget....
content/renderer/render_widget.cc:450: TRACE_EVENT2("renderer",
"HandleMouseMove",
nit: maybe this code would be cleaner with a local variable:

  const WebMouseEvent* mouse_event =
      static_cast<...>(...);
  TRACE_EVENT2(...
               "x", mouse_event->x,
               "y", mouse_event->y);
  prevent_default = WillHandleMouseEvent(*mouse_event);

http://codereview.chromium.org/8820001/diff/1/content/renderer/render_widget....
content/renderer/render_widget.cc:845: if (pending_input_event_ack_.get())
This probably deserves a comment.

[jamesr, 2011-12-09 01:53:45]

I've found one case where this is a big win.  On platforms that send the
UpdateRect_ACK synchronously (windows+mac) if the page has a slow mouse move
listener that does paint damage and a slow RAF callback that also does paint
damage, then without this patch the renderer side pattern is:

1.) handle mousemove, receive paint damage, hold ACK since paint damage is done
2.) DoDeferredUpdate due to self timer and:
  a.) send UpdateRect IPC to browser
  b.) send pending input ACK to browser
3.) Receive UpdateRect_ACK and from that IPC handlers run RAF callback and paint
4.) Receive next mouse move from browser in response to pending input ACK and go
back to (1)

so we end up RAF+painting twice for every mouse move we process. Not so good,
the latency is bad and it's uneven.

with this patch the pattern is:

1.) handle mousemove, receive paint damage, hold ACK since paint damage is done
2.) DoDeferredUpdate due to self timer and:
  a.) send pending input ACK to browser
  b.) send UpdateRect IPC to browser
3.) receive next mouse move in response to pending input ACK, process it, take
paint damage, set pending input ACK
4.) RAF+paint due to UpdateRect_ACK from browser, and go back to (2)

so we do one mouse move, then one paint. much better!

so I'll clean this up and submit.  It's a slightly contrived test case, but
definitely in the realm of plausibility. I wouldn't be surprised if this is what
maps did, for example.

[darin (slow to review), 2011-12-09 17:41:50]

Very cool!

On Thu, Dec 8, 2011 at 5:53 PM, <jamesr@chromium.org> wrote:

> I've found one case where this is a big win.  On platforms that send the
> UpdateRect_ACK synchronously (windows+mac) if the page has a slow mouse
> move
> listener that does paint damage and a slow RAF callback that also does
> paint
> damage, then without this patch the renderer side pattern is:
>
> 1.) handle mousemove, receive paint damage, hold ACK since paint damage is
> done
> 2.) DoDeferredUpdate due to self timer and:
>  a.) send UpdateRect IPC to browser
>  b.) send pending input ACK to browser
> 3.) Receive UpdateRect_ACK and from that IPC handlers run RAF callback and
> paint
> 4.) Receive next mouse move from browser in response to pending input ACK
> and go
> back to (1)
>
> so we end up RAF+painting twice for every mouse move we process. Not so
> good,
> the latency is bad and it's uneven.
>
> with this patch the pattern is:
>
> 1.) handle mousemove, receive paint damage, hold ACK since paint damage is
> done
> 2.) DoDeferredUpdate due to self timer and:
>  a.) send pending input ACK to browser
>  b.) send UpdateRect IPC to browser
> 3.) receive next mouse move in response to pending input ACK, process it,
> take
> paint damage, set pending input ACK
> 4.) RAF+paint due to UpdateRect_ACK from browser, and go back to (2)
>
> so we do one mouse move, then one paint. much better!
>
> so I'll clean this up and submit.  It's a slightly contrived test case, but
> definitely in the realm of plausibility. I wouldn't be surprised if this
> is what
> maps did, for example.
>
>
http://codereview.chromium.**org/8820001/<http://codereview.chromium.org/8820...
>

[commit-bot: I haz the power, 2011-12-12 20:59:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/8820001/9001

[commit-bot: I haz the power, 2011-12-12 22:43:30]

Try job failure for 8820001-9001 (retry) on win_rel for step "base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[jamesr, 2011-12-12 23:25:02]

I dunno what commit-bot is talking about, the failure looks unrelated and my
tryjobs passed.

Gonna click the checkbox again and see what happens.

[commit-bot: I haz the power, 2011-12-12 23:26:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/8820001/9001

[commit-bot: I haz the power, 2011-12-13 01:35:28]

Change committed as 114141