Remove burst mode from VSyncMonitor.

Remove burst mode from VSyncMonitor.

Extra vsyncs after each rendering cause 1% more power consumption.

NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279997

[egor.starkov, 2014-06-19 13:20:21]

Currently VSyncMonitor requests for 5 more vsync calls from system after every
rendering, that look like dry shots, because rendering thread doesn't want to
render any more. This increases power consumption by ~1% in scenario when cursor
is just blinking on google.com. Measured on android 4.4 phone with test shell
installed.

[Ted C, 2014-06-19 17:55:25]

Adding @aruslan as he added the code initially and I'm not familiar with it's
purpose.

[aruslan, 2014-06-19 18:23:23]

On 2014/06/19 17:55:25, Ted C wrote:
> Adding @aruslan as he added the code initially and I'm not familiar with it's
> purpose.

I don't know if we still need this or not, but the original intent was to keep
scrolling smoother.
Daniel should know more.

[no sievers, 2014-06-19 18:52:50]

Yes I think it *might* cause jank if we are less aggressive in sending
BeginFrame messages.

Can we maybe reduce MAX_AUTO_ONVSYNC_COUNT  instead? What are the common,
representative use cases for this? How many vsync intervals pass without a frame
update in such cases?

(1% power consumption sounds a bit like it would be hard to measure against
noise. If this specific case further only occurs a very small fraction of the
time, then there are no practical power savings gained from this.)

[Sami, 2014-06-20 15:48:01]

I think postSyntheticVSync() should make keeping the vsync  going a little
longer largely unnecessary, so I'm not opposed to removing this.

Is there a bug for the original reason we added this?

[no sievers, 2014-06-20 18:31:54]

I see, so maybe this is actually better. And if |needs_begin_frame_| stays on,
then we keep requesting ticks anyway. I'm not aware of a specific bug, things
have changed a lot, so we could try to make the vsync accounting strict this way
(1 request == 1 tick).

If we find a latency regression where we explicitly need to send a proactive
update (and for example ignore needs_begin_frame being off), then maybe it's
better to do this in the right place rather than let VSyncMonitor handle this by
ticking more than once.

There's a slight chance that this uncovers a bug somewhere, where we don't
request an update (have to check browser compositor things like missing a frame
etc.), so just FYI.

LGTM.

[egor.starkov, 2014-06-23 08:35:01]

The CQ bit was checked by egor.starkov@samsung.com

[commit-bot: I haz the power, 2014-06-23 08:36:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/egor.starkov@samsung.com/346813002/1

[egor.starkov, 2014-06-23 08:39:17]

On 2014/06/20 18:31:54, sievers wrote:
> I see, so maybe this is actually better. And if |needs_begin_frame_| stays on,
> then we keep requesting ticks anyway. I'm not aware of a specific bug, things
> have changed a lot, so we could try to make the vsync accounting strict this
way
> (1 request == 1 tick).
> 
> If we find a latency regression where we explicitly need to send a proactive
> update (and for example ignore needs_begin_frame being off), then maybe it's
> better to do this in the right place rather than let VSyncMonitor handle this
by
> ticking more than once.
> 
> There's a slight chance that this uncovers a bug somewhere, where we don't
> request an update (have to check browser compositor things like missing a
frame
> etc.), so just FYI.
> 
> LGTM.

1% could be of course a measurement error, but it seems to me that these vsyncs
are
not needed there anyway. So the patch will just remove unnecessary calls and
simplify code a bit.
Thanks for review.

[commit-bot: I haz the power, 2014-06-23 12:17:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-23 12:20:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-23 12:20:42]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[aruslan, 2014-06-24 02:36:14]

On 2014/06/23 08:39:17, egor.starkov wrote:
> On 2014/06/20 18:31:54, sievers wrote:
> > I see, so maybe this is actually better. And if |needs_begin_frame_| stays
on,
> > then we keep requesting ticks anyway. I'm not aware of a specific bug,
things
> > have changed a lot, so we could try to make the vsync accounting strict this
> way
> > (1 request == 1 tick).
> > 
> > If we find a latency regression where we explicitly need to send a proactive
> > update (and for example ignore needs_begin_frame being off), then maybe it's
> > better to do this in the right place rather than let VSyncMonitor handle
this
> by
> > ticking more than once.
> > 
> > There's a slight chance that this uncovers a bug somewhere, where we don't
> > request an update (have to check browser compositor things like missing a
> frame
> > etc.), so just FYI.
> > 
> > LGTM.
> 
> 1% could be of course a measurement error, but it seems to me that these
vsyncs
> are
> not needed there anyway. So the patch will just remove unnecessary calls and
> simplify code a bit.
> Thanks for review.

Did you try running content_shell VSyncMonitorTest tests?
I'd be suspicious that out of 287 content_shell tests only 4 has failed, and all
of them a VSyncMonitor-related:

C 1743.891s Main  [==========] 287 tests ran.
C 1743.891s Main  [  PASSED  ] 283 tests.
C 1743.891s Main  [  FAILED  ] 4 tests, listed below:
C 1743.891s Main  [  FAILED  ]
org.chromium.content.browser.VSyncMonitorTest#testVSyncPeriodDisallowJBVSync
(CRASHED)
C 1743.892s Main  [  FAILED  ]
org.chromium.content.browser.VSyncMonitorTest#testVSyncPeriodAllowJBVSync
(CRASHED)
C 1743.892s Main  [  FAILED  ]
org.chromium.content.browser.VSyncMonitorTest#testVSyncActivationFromIdleDisallowJBVSync
(CRASHED)
C 1743.892s Main  [  FAILED  ]
org.chromium.content.browser.VSyncMonitorTest#testVSyncActivationFromIdleAllowJBVSync
(CRASHED)
C 1743.892s Main  
C 1743.892s Main  4 FAILED TESTS

http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

Yours,
-- R.

[egor.starkov, 2014-06-24 14:16:03]

On 2014/06/24 02:36:14, aruslan wrote:
> On 2014/06/23 08:39:17, egor.starkov wrote:
> > On 2014/06/20 18:31:54, sievers wrote:
> > > I see, so maybe this is actually better. And if |needs_begin_frame_| stays
> on,
> > > then we keep requesting ticks anyway. I'm not aware of a specific bug,
> things
> > > have changed a lot, so we could try to make the vsync accounting strict
this
> > way
> > > (1 request == 1 tick).
> > > 
> > > If we find a latency regression where we explicitly need to send a
proactive
> > > update (and for example ignore needs_begin_frame being off), then maybe
it's
> > > better to do this in the right place rather than let VSyncMonitor handle
> this
> > by
> > > ticking more than once.
> > > 
> > > There's a slight chance that this uncovers a bug somewhere, where we don't
> > > request an update (have to check browser compositor things like missing a
> > frame
> > > etc.), so just FYI.
> > > 
> > > LGTM.
> > 
> > 1% could be of course a measurement error, but it seems to me that these
> vsyncs
> > are
> > not needed there anyway. So the patch will just remove unnecessary calls and
> > simplify code a bit.
> > Thanks for review.
> 
> Did you try running content_shell VSyncMonitorTest tests?
> I'd be suspicious that out of 287 content_shell tests only 4 has failed, and
all
> of them a VSyncMonitor-related:
> 
> C 1743.891s Main  [==========] 287 tests ran.
> C 1743.891s Main  [  PASSED  ] 283 tests.
> C 1743.891s Main  [  FAILED  ] 4 tests, listed below:
> C 1743.891s Main  [  FAILED  ]
> org.chromium.content.browser.VSyncMonitorTest#testVSyncPeriodDisallowJBVSync
> (CRASHED)
> C 1743.892s Main  [  FAILED  ]
> org.chromium.content.browser.VSyncMonitorTest#testVSyncPeriodAllowJBVSync
> (CRASHED)
> C 1743.892s Main  [  FAILED  ]
>
org.chromium.content.browser.VSyncMonitorTest#testVSyncActivationFromIdleDisallowJBVSync
> (CRASHED)
> C 1743.892s Main  [  FAILED  ]
>
org.chromium.content.browser.VSyncMonitorTest#testVSyncActivationFromIdleAllowJBVSync
> (CRASHED)
> C 1743.892s Main  
> C 1743.892s Main  4 FAILED TESTS
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
> 
> Yours,
> -- R.

Thanks for pointing. I've checked those tests locally and found out that
VSyncMonitorTest
has 4 tests and they all are failing with my patch and without my patch. So I
just updated
my patchset to fix compilation error.

[Yaron, 2014-06-24 18:16:00]

rubberstamp owners lgtm
sievers approval is better than mine for this

[egor.starkov, 2014-06-25 07:02:51]

The CQ bit was checked by egor.starkov@samsung.com

[commit-bot: I haz the power, 2014-06-25 07:05:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/egor.starkov@samsung.com/346813002/20001

[commit-bot: I haz the power, 2014-06-25 10:43:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-25 11:02:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-25 11:02:09]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[egor.starkov, 2014-06-25 11:07:42]

The CQ bit was checked by egor.starkov@samsung.com

[commit-bot: I haz the power, 2014-06-25 11:09:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/egor.starkov@samsung.com/346813002/20001

[commit-bot: I haz the power, 2014-06-25 13:59:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-25 15:37:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-25 15:37:54]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[egor.starkov, 2014-06-26 13:35:51]

The CQ bit was checked by egor.starkov@samsung.com

[commit-bot: I haz the power, 2014-06-26 13:37:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/egor.starkov@samsung.com/346813002/20001

[commit-bot: I haz the power, 2014-06-26 13:38:30]

Change committed as 279997

[Peter Beverloo, 2014-06-26 15:22:07]

A revert of this CL has been created in
https://codereview.chromium.org/358473006/ by peter@chromium.org.

The reason for reverting is: Four of the introduced tests time out on Android.

VSyncMonitorTest#testVSyncActivationFromIdleAllowJBVSync
VSyncMonitorTest#testVSyncActivationFromIdleDisallowJBVSync
VSyncMonitorTest#testVSyncPeriodAllowJBVSync
VSyncMonitorTest#testVSyncPeriodDisallowJBVSync

The log output can be seen here:

http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...

This failure could be observed in the try-bot results as part of the patch.
Please do not use NOTRY unless you're absolutely certain that the failures are
unrelated..

[egor.starkov, 2014-06-27 14:00:53]

This commit was reverted. So I've fixed the tests to follow the rule 1 request
== 1 tick. VSyncMonitorTest should not fail now. Could you please review the new
patchset?

[aruslan, 2014-06-27 15:47:21]

lgtm - thanks for fixing the test.

[egor.starkov, 2014-06-27 17:57:55]

On 2014/06/27 15:47:21, aruslan wrote:
> lgtm - thanks for fixing the test.

Hi Ruslan, do I need to create another review to commit this patch
or I can just reopen this one and commit again through CQ?

Br,
Egor.

[aruslan, 2014-06-27 18:02:06]

On 2014/06/27 17:57:55, egor.starkov wrote:
> On 2014/06/27 15:47:21, aruslan wrote:
> > lgtm - thanks for fixing the test.
> 
> Hi Ruslan, do I need to create another review to commit this patch
> or I can just reopen this one and commit again through CQ?
> 
> Br,
> Egor.

I always uploaded a new patch; may be there are simpler ways, but I'm not aware
of them.
Once uploaded - please add a reference to this CL and add OWNERS to lgtm the
changes.

Thanks,
-- Ruslan.

[aruslan, 2014-06-27 18:02:28]

On 2014/06/27 18:02:06, aruslan wrote:
> On 2014/06/27 17:57:55, egor.starkov wrote:
> > On 2014/06/27 15:47:21, aruslan wrote:
> > > lgtm - thanks for fixing the test.
> > 
> > Hi Ruslan, do I need to create another review to commit this patch
> > or I can just reopen this one and commit again through CQ?
> > 
> > Br,
> > Egor.
> 
> I always uploaded a new patch; may be there are simpler ways, but I'm not
aware
> of them.
> Once uploaded - please add a reference to this CL and add OWNERS to lgtm the
> changes.
> 
> Thanks,
> -- Ruslan.

(new patch == new issue in codereview.)

[egor.starkov, 2014-06-27 18:37:04]

On 2014/06/27 18:02:28, aruslan wrote:
> On 2014/06/27 18:02:06, aruslan wrote:
> > On 2014/06/27 17:57:55, egor.starkov wrote:
> > > On 2014/06/27 15:47:21, aruslan wrote:
> > > > lgtm - thanks for fixing the test.
> > > 
> > > Hi Ruslan, do I need to create another review to commit this patch
> > > or I can just reopen this one and commit again through CQ?
> > > 
> > > Br,
> > > Egor.
> > 
> > I always uploaded a new patch; may be there are simpler ways, but I'm not
> aware
> > of them.
> > Once uploaded - please add a reference to this CL and add OWNERS to lgtm the
> > changes.
> > 
> > Thanks,
> > -- Ruslan.
> 
> (new patch == new issue in codereview.)

Ok, thanks.

[Sami, 2014-07-07 14:34:31]

On 2014/06/27 at 18:37:04, egor.starkov wrote:
> On 2014/06/27 18:02:28, aruslan wrote:
> > On 2014/06/27 18:02:06, aruslan wrote:
> > > On 2014/06/27 17:57:55, egor.starkov wrote:
> > > > On 2014/06/27 15:47:21, aruslan wrote:
> > > > > lgtm - thanks for fixing the test.
> > > > 
> > > > Hi Ruslan, do I need to create another review to commit this patch
> > > > or I can just reopen this one and commit again through CQ?
> > > > 
> > > > Br,
> > > > Egor.
> > > 
> > > I always uploaded a new patch; may be there are simpler ways, but I'm not
> > aware
> > > of them.
> > > Once uploaded - please add a reference to this CL and add OWNERS to lgtm
the
> > > changes.
> > > 
> > > Thanks,
> > > -- Ruslan.
> > 
> > (new patch == new issue in codereview.)
> 
> Ok, thanks.

FYI, you can also re-open the old review and land the fixed patch that way
through the commit queue. Having all the revision in the same place makes it a
little easier for reviewers to see how the patch was changed.