Optimize the rendering when there are pending key events....

Optimize the rendering when there are pending key events.

BUG=27932
: Regression: Forced rendering for every keystroke
TEST=See the bug report.


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

[Evan Martin, 2009-11-23 18:05:37]

Seems clever.  Darin may have better thoughts on it.

[James Su, 2009-11-24 07:12:33]

I just updated the CL to have a maximum postponed duration for the paint ack
message, so that painting can still be done regularly if the user holds a key
for very long time.

James Su

[darin (slow to review), 2009-11-24 07:46:06]

so this is to compensate for the fact that we now queue key events in the
browser process and only send them one-at-a-time to the renderer?  perhaps that
was not a good idea.

previously, we benefited from sending a nice stream of key events to the
renderer, which helped ensure fast (low latency) keyboard input.

i'm concerned that we are now just hacking around the problem instead of fixing
the root problem in a better way.

suppressing painting until all key events have been forwarded seems like it
could result in janky bursts of no screen updating.

it seems like there should be a better solution to:
http://code.google.com/p/chromium/issues/detail?id=21624

http://codereview.chromium.org/435002/diff/2006/3003
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/435002/diff/2006/3003#newcode820
chrome/browser/renderer_host/render_widget_host.cc:820: void
RenderWidgetHost::OnMsgInputEventAck(const IPC::Message& message) {
this function has grown to be way too long to be readable.  i think it needs to
be broken down into smaller units.

[James Su, 2009-11-24 10:10:01]

Thanks a lot for your review.

On 2009/11/24 07:46:06, darin wrote:
> so this is to compensate for the fact that we now queue key events in the
> browser process and only send them one-at-a-time to the renderer?  perhaps
that
> was not a good idea.
> 
> previously, we benefited from sending a nice stream of key events to the
> renderer, which helped ensure fast (low latency) keyboard input.
IMHO, the situation is not that bad. In the previous solution, if the renderer
is too slow to process the key events, they will be queued in the renderer's
message queue. So the renderer may need to process multiple key events in batch
before having a chance to process the deferred update.
In the current solution, the keys are queued in the browser, so the renderer
always has a chance to process the deferred update after processing each key
event. This actually helps get rid off the "janky bursts" effect.

> 
> i'm concerned that we are now just hacking around the problem instead of
fixing
> the root problem in a better way.
I also hope we could find out a better way :-)

> 
> suppressing painting until all key events have been forwarded seems like it
> could result in janky bursts of no screen updating.
This effect is just what the bug reporter want. In the previous solution, it's
realized in a nature way. But now we need extra code (this CL) to simulate it.

> 
> it seems like there should be a better solution to:
> http://code.google.com/p/chromium/issues/detail?id=21624
> 
> http://codereview.chromium.org/435002/diff/2006/3003
> File chrome/browser/renderer_host/render_widget_host.cc (right):
> 
> http://codereview.chromium.org/435002/diff/2006/3003#newcode820
> chrome/browser/renderer_host/render_widget_host.cc:820: void
> RenderWidgetHost::OnMsgInputEventAck(const IPC::Message& message) {
> this function has grown to be way too long to be readable.  i think it needs
to
> be broken down into smaller units.
I'll update the CL to break function.

[James Su, 2009-11-25 03:21:45]

Hi,
  I just updated the CL to split OnMsgInputEventAck().

- James Su

[darin (slow to review), 2009-11-25 21:26:59]

I'm OK with landing this patch now as it fixes a pretty severe problem.  I'm
still not happy with the overall solution.

I'd really like to see it improved along the lines of what I suggested
previously.  We should try to make sure that the renderer can consume keyboard
events as fast as it can without having to wait for acknowledgments from the
browser.

http://codereview.chromium.org/435002/diff/4/5
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/435002/diff/4/5#newcode55
chrome/browser/renderer_host/render_widget_host.cc:55: static const int
kPaintACKMsgMaxPostponedDurationMS = 1000;
1000 seems kind of high to me.  how did you select this value?

http://codereview.chromium.org/435002/diff/4/6
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/435002/diff/4/6#newcode584
chrome/browser/renderer_host/render_widget_host.h:584: // not sent yet.
nit: no new line here.  one comment paragraph is good :)

[Use pkasting(at)chromium.org, 2009-11-25 21:30:32]

On Wed, Nov 25, 2009 at 1:26 PM, <darin@chromium.org> wrote:

> I'd really like to see it improved along the lines of what I suggested
> previously.  We should try to make sure that the renderer can consume
> keyboard
> events as fast as it can without having to wait for acknowledgments from
> the
> browser.


Can one of you file an appropriate bug about this stuff?  I fear this wish
will just get lost.

PK

[James Su, 2009-11-26 01:21:48]

A bug for further optimization has been filed: http://crbug.com/28839

- James Su

http://codereview.chromium.org/435002/diff/4/5
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/435002/diff/4/5#newcode55
chrome/browser/renderer_host/render_widget_host.cc:55: static const int
kPaintACKMsgMaxPostponedDurationMS = 1000;
On 2009/11/25 21:27:00, darin wrote:
> 1000 seems kind of high to me.  how did you select this value?

I chose this value randomly. How about 500?

http://codereview.chromium.org/435002/diff/4/6
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/435002/diff/4/6#newcode584
chrome/browser/renderer_host/render_widget_host.h:584: // not sent yet.
On 2009/11/25 21:27:00, darin wrote:
> nit: no new line here.  one comment paragraph is good :)

Done.

[darin (slow to review), 2009-11-26 05:54:48]

On Wed, Nov 25, 2009 at 5:21 PM, <suzhe@chromium.org> wrote:

> A bug for further optimization has been filed: http://crbug.com/28839
>
> - James Su
>
>
>
> http://codereview.chromium.org/435002/diff/4/5
> File chrome/browser/renderer_host/render_widget_host.cc (right):
>
> http://codereview.chromium.org/435002/diff/4/5#newcode55
> chrome/browser/renderer_host/render_widget_host.cc:55: static const int
> kPaintACKMsgMaxPostponedDurationMS = 1000;
> On 2009/11/25 21:27:00, darin wrote:
>
>> 1000 seems kind of high to me.  how did you select this value?
>>
>
> I chose this value randomly. How about 500?


Why not 100 or 50?  What basis do we have to make a decision?

-Darin


>
>
> http://codereview.chromium.org/435002/diff/4/6
> File chrome/browser/renderer_host/render_widget_host.h (right):
>
> http://codereview.chromium.org/435002/diff/4/6#newcode584
> chrome/browser/renderer_host/render_widget_host.h:584: // not sent yet.
> On 2009/11/25 21:27:00, darin wrote:
>
>> nit: no new line here.  one comment paragraph is good :)
>>
>
> Done.
>
>
> http://codereview.chromium.org/435002
>

[James Su, 2009-11-26 06:38:11]

100 may only allow to process one or two key strokes in batch, which would be no
effect at all.
1000 allows to process at most 20 key strokes in batch on my machine, 500 allows
about 10.

On 2009/11/26 05:54:48, darin wrote:
> On Wed, Nov 25, 2009 at 5:21 PM, <mailto:suzhe@chromium.org> wrote:
> 
> > A bug for further optimization has been filed: http://crbug.com/28839
> >
> > - James Su
> >
> >
> >
> > http://codereview.chromium.org/435002/diff/4/5
> > File chrome/browser/renderer_host/render_widget_host.cc (right):
> >
> > http://codereview.chromium.org/435002/diff/4/5#newcode55
> > chrome/browser/renderer_host/render_widget_host.cc:55: static const int
> > kPaintACKMsgMaxPostponedDurationMS = 1000;
> > On 2009/11/25 21:27:00, darin wrote:
> >
> >> 1000 seems kind of high to me.  how did you select this value?
> >>
> >
> > I chose this value randomly. How about 500?
> 
> 
> Why not 100 or 50?  What basis do we have to make a decision?
> 
> -Darin
> 
> 
> >
> >
> > http://codereview.chromium.org/435002/diff/4/6
> > File chrome/browser/renderer_host/render_widget_host.h (right):
> >
> > http://codereview.chromium.org/435002/diff/4/6#newcode584
> > chrome/browser/renderer_host/render_widget_host.h:584: // not sent yet.
> > On 2009/11/25 21:27:00, darin wrote:
> >
> >> nit: no new line here.  one comment paragraph is good :)
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/435002
> >
>

[James Su, 2009-11-26 06:40:30]

Anyway, if you think my proposal in http://crbug/28839 is reasonable, this CL
would just be a temporary solution.

- James Su

On 2009/11/26 06:38:11, James Su wrote:
> 100 may only allow to process one or two key strokes in batch, which would be
no
> effect at all.
> 1000 allows to process at most 20 key strokes in batch on my machine, 500
allows
> about 10.
> 
> On 2009/11/26 05:54:48, darin wrote:
> > On Wed, Nov 25, 2009 at 5:21 PM, <mailto:suzhe@chromium.org> wrote:
> > 
> > > A bug for further optimization has been filed: http://crbug.com/28839
> > >
> > > - James Su
> > >
> > >
> > >
> > > http://codereview.chromium.org/435002/diff/4/5
> > > File chrome/browser/renderer_host/render_widget_host.cc (right):
> > >
> > > http://codereview.chromium.org/435002/diff/4/5#newcode55
> > > chrome/browser/renderer_host/render_widget_host.cc:55: static const int
> > > kPaintACKMsgMaxPostponedDurationMS = 1000;
> > > On 2009/11/25 21:27:00, darin wrote:
> > >
> > >> 1000 seems kind of high to me.  how did you select this value?
> > >>
> > >
> > > I chose this value randomly. How about 500?
> > 
> > 
> > Why not 100 or 50?  What basis do we have to make a decision?
> > 
> > -Darin
> > 
> > 
> > >
> > >
> > > http://codereview.chromium.org/435002/diff/4/6
> > > File chrome/browser/renderer_host/render_widget_host.h (right):
> > >
> > > http://codereview.chromium.org/435002/diff/4/6#newcode584
> > > chrome/browser/renderer_host/render_widget_host.h:584: // not sent yet.
> > > On 2009/11/25 21:27:00, darin wrote:
> > >
> > >> nit: no new line here.  one comment paragraph is good :)
> > >>
> > >
> > > Done.
> > >
> > >
> > > http://codereview.chromium.org/435002
> > >
> >

[darin (slow to review), 2009-11-26 08:00:50]

Fine to proceed with 1000 if you think that's best.  I don't mean to hold up
this CL on that issue.  I was just curious how to tune it, but I very much
like your proposed solution in
http://code.google.com/p/chromium/issues/detail?id=28839.

Thanks!
-Darin

On Wed, Nov 25, 2009 at 10:40 PM, <suzhe@chromium.org> wrote:

> Anyway, if you think my proposal in http://crbug/28839 is reasonable, this
> CL
> would just be a temporary solution.
>
> - James Su
>
>
> On 2009/11/26 06:38:11, James Su wrote:
>
>> 100 may only allow to process one or two key strokes in batch, which would
>> be
>>
> no
>
>> effect at all.
>> 1000 allows to process at most 20 key strokes in batch on my machine, 500
>>
> allows
>
>> about 10.
>>
>
>  On 2009/11/26 05:54:48, darin wrote:
>> > On Wed, Nov 25, 2009 at 5:21 PM, <mailto:suzhe@chromium.org> wrote:
>> >
>> > > A bug for further optimization has been filed: http://crbug.com/28839
>> > >
>> > > - James Su
>> > >
>> > >
>> > >
>> > > http://codereview.chromium.org/435002/diff/4/5
>> > > File chrome/browser/renderer_host/render_widget_host.cc (right):
>> > >
>> > > http://codereview.chromium.org/435002/diff/4/5#newcode55
>> > > chrome/browser/renderer_host/render_widget_host.cc:55: static const
>> int
>> > > kPaintACKMsgMaxPostponedDurationMS = 1000;
>> > > On 2009/11/25 21:27:00, darin wrote:
>> > >
>> > >> 1000 seems kind of high to me.  how did you select this value?
>> > >>
>> > >
>> > > I chose this value randomly. How about 500?
>> >
>> >
>> > Why not 100 or 50?  What basis do we have to make a decision?
>> >
>> > -Darin
>> >
>> >
>> > >
>> > >
>> > > http://codereview.chromium.org/435002/diff/4/6
>> > > File chrome/browser/renderer_host/render_widget_host.h (right):
>> > >
>> > > http://codereview.chromium.org/435002/diff/4/6#newcode584
>> > > chrome/browser/renderer_host/render_widget_host.h:584: // not sent
>> yet.
>> > > On 2009/11/25 21:27:00, darin wrote:
>> > >
>> > >> nit: no new line here.  one comment paragraph is good :)
>> > >>
>> > >
>> > > Done.
>> > >
>> > >
>> > > http://codereview.chromium.org/435002
>> > >
>> >
>>
>
>
>
> http://codereview.chromium.org/435002
>

[James Su, 2009-11-26 08:41:31]

So, may I submit this CL first, so that it can catch M4? I'll implement the new
proposal asap, but it may take some time.

- James Su

On 2009/11/26 08:00:50, darin wrote:
> Fine to proceed with 1000 if you think that's best.  I don't mean to hold up
> this CL on that issue.  I was just curious how to tune it, but I very much
> like your proposed solution in
> http://code.google.com/p/chromium/issues/detail?id=28839.
> 
> Thanks!
> -Darin
> 
> On Wed, Nov 25, 2009 at 10:40 PM, <mailto:suzhe@chromium.org> wrote:
> 
> > Anyway, if you think my proposal in http://crbug/28839 is reasonable, this
> > CL
> > would just be a temporary solution.
> >
> > - James Su
> >
> >
> > On 2009/11/26 06:38:11, James Su wrote:
> >
> >> 100 may only allow to process one or two key strokes in batch, which would
> >> be
> >>
> > no
> >
> >> effect at all.
> >> 1000 allows to process at most 20 key strokes in batch on my machine, 500
> >>
> > allows
> >
> >> about 10.
> >>
> >
> >  On 2009/11/26 05:54:48, darin wrote:
> >> > On Wed, Nov 25, 2009 at 5:21 PM, <mailto:suzhe@chromium.org> wrote:
> >> >
> >> > > A bug for further optimization has been filed: http://crbug.com/28839
> >> > >
> >> > > - James Su
> >> > >
> >> > >
> >> > >
> >> > > http://codereview.chromium.org/435002/diff/4/5
> >> > > File chrome/browser/renderer_host/render_widget_host.cc (right):
> >> > >
> >> > > http://codereview.chromium.org/435002/diff/4/5#newcode55
> >> > > chrome/browser/renderer_host/render_widget_host.cc:55: static const
> >> int
> >> > > kPaintACKMsgMaxPostponedDurationMS = 1000;
> >> > > On 2009/11/25 21:27:00, darin wrote:
> >> > >
> >> > >> 1000 seems kind of high to me.  how did you select this value?
> >> > >>
> >> > >
> >> > > I chose this value randomly. How about 500?
> >> >
> >> >
> >> > Why not 100 or 50?  What basis do we have to make a decision?
> >> >
> >> > -Darin
> >> >
> >> >
> >> > >
> >> > >
> >> > > http://codereview.chromium.org/435002/diff/4/6
> >> > > File chrome/browser/renderer_host/render_widget_host.h (right):
> >> > >
> >> > > http://codereview.chromium.org/435002/diff/4/6#newcode584
> >> > > chrome/browser/renderer_host/render_widget_host.h:584: // not sent
> >> yet.
> >> > > On 2009/11/25 21:27:00, darin wrote:
> >> > >
> >> > >> nit: no new line here.  one comment paragraph is good :)
> >> > >>
> >> > >
> >> > > Done.
> >> > >
> >> > >
> >> > > http://codereview.chromium.org/435002
> >> > >
> >> >
> >>
> >
> >
> >
> > http://codereview.chromium.org/435002
> >
>

[James Su, 2009-11-27 11:31:54]

Ping darin, may I submit this CL?

On 2009/11/26 08:41:31, James Su wrote:
> So, may I submit this CL first, so that it can catch M4? I'll implement the
new
> proposal asap, but it may take some time.
> 
> - James Su
> 
> On 2009/11/26 08:00:50, darin wrote:
> > Fine to proceed with 1000 if you think that's best.  I don't mean to hold up
> > this CL on that issue.  I was just curious how to tune it, but I very much
> > like your proposed solution in
> > http://code.google.com/p/chromium/issues/detail?id=28839.
> > 
> > Thanks!
> > -Darin
> > 
> > On Wed, Nov 25, 2009 at 10:40 PM, <mailto:suzhe@chromium.org> wrote:
> > 
> > > Anyway, if you think my proposal in http://crbug/28839 is reasonable, this
> > > CL
> > > would just be a temporary solution.
> > >
> > > - James Su
> > >
> > >
> > > On 2009/11/26 06:38:11, James Su wrote:
> > >
> > >> 100 may only allow to process one or two key strokes in batch, which
would
> > >> be
> > >>
> > > no
> > >
> > >> effect at all.
> > >> 1000 allows to process at most 20 key strokes in batch on my machine, 500
> > >>
> > > allows
> > >
> > >> about 10.
> > >>
> > >
> > >  On 2009/11/26 05:54:48, darin wrote:
> > >> > On Wed, Nov 25, 2009 at 5:21 PM, <mailto:suzhe@chromium.org> wrote:
> > >> >
> > >> > > A bug for further optimization has been filed: http://crbug.com/28839
> > >> > >
> > >> > > - James Su
> > >> > >
> > >> > >
> > >> > >
> > >> > > http://codereview.chromium.org/435002/diff/4/5
> > >> > > File chrome/browser/renderer_host/render_widget_host.cc (right):
> > >> > >
> > >> > > http://codereview.chromium.org/435002/diff/4/5#newcode55
> > >> > > chrome/browser/renderer_host/render_widget_host.cc:55: static const
> > >> int
> > >> > > kPaintACKMsgMaxPostponedDurationMS = 1000;
> > >> > > On 2009/11/25 21:27:00, darin wrote:
> > >> > >
> > >> > >> 1000 seems kind of high to me.  how did you select this value?
> > >> > >>
> > >> > >
> > >> > > I chose this value randomly. How about 500?
> > >> >
> > >> >
> > >> > Why not 100 or 50?  What basis do we have to make a decision?
> > >> >
> > >> > -Darin
> > >> >
> > >> >
> > >> > >
> > >> > >
> > >> > > http://codereview.chromium.org/435002/diff/4/6
> > >> > > File chrome/browser/renderer_host/render_widget_host.h (right):
> > >> > >
> > >> > > http://codereview.chromium.org/435002/diff/4/6#newcode584
> > >> > > chrome/browser/renderer_host/render_widget_host.h:584: // not sent
> > >> yet.
> > >> > > On 2009/11/25 21:27:00, darin wrote:
> > >> > >
> > >> > >> nit: no new line here.  one comment paragraph is good :)
> > >> > >>
> > >> > >
> > >> > > Done.
> > >> > >
> > >> > >
> > >> > > http://codereview.chromium.org/435002
> > >> > >
> > >> >
> > >>
> > >
> > >
> > >
> > > http://codereview.chromium.org/435002
> > >
> >

[darin (slow to review), 2009-11-30 21:05:28]

On 2009/11/27 11:31:54, James Su wrote:
> Ping darin, may I submit this CL?

Yes.  Sorry, that's what I meant earlier.  LGTM for trunk and M4.