Fix scheduler bug in skipping main frames

Fix scheduler bug in skipping main frames

Previously the scheduler would think that it could skip a main frame if
the begin frame's start time + estimated time < deadline.  However,
if this begin frame gets delivered very late, then this is incorrect,
and it's even possible that start time + estimated time < Now() and is
in the past, which doesn't make sense.

Instead, check if Now() + estimated time < deadline.  This is the right
logic to see if it's possible to finish activation from the current
moment before the deadline.

This was causing problems where the following events would occur:
  - browser has a 40ms draw
  - browser delivers a begin frame
  - renderer mistakenly thinks the activation fits below the deadline
    even though estimated time > one frame
  - renderer skips this begin main frame
  - browser has another 40ms draw
  - renderer now does a begin main frame, >40ms late

This was causing tab capture performance tests where the browser was
pegged with long draw operations to cause the renderer frame times to go
from 60 to 100ms.

R=sunnyps@chromium.org
BUG=632292
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/ff408189176e09e6c8160cc3ac6d44bc9846ebef
Cr-Commit-Position: refs/heads/master@{#425449}

[enne (OOO), 2016-10-12 20:00:06]

Description was changed from

==========
Fix scheduler bug in skipping main frames

Previously the scheduler would think that it could skip a main frame if
the begin frame's start time + estimated time < deadline.  However,
if this begin frame gets delivered very late, then this is incorrect,
and it's even possible that start time + estimated time < Now() and is
in the past, which doesn't make sense.

Instead, check if Now() + estimated time < deadline.  This is the right
logic to see if it's possible to finish activation from the current
moment before the deadline.

This was causing problems where the following events would occur:
  - browser has a 40ms draw
  - browser delivers a begin frame
  - renderer mistakenly thinks the activation fits below the deadline
    even though estimated time > one frame
  - renderer skips this begin main frame
  - browser has another 40ms draw
  - renderer now does a begin main frame, >40ms late

This was causing tab capture performance tests where the browser was
pegged with long draw operations to cause the renderer frame times to go
from 60 to 100ms.

R=sunnyps@chromium.org
BUG=632292
==========

to

==========
Fix scheduler bug in skipping main frames

Previously the scheduler would think that it could skip a main frame if
the begin frame's start time + estimated time < deadline.  However,
if this begin frame gets delivered very late, then this is incorrect,
and it's even possible that start time + estimated time < Now() and is
in the past, which doesn't make sense.

Instead, check if Now() + estimated time < deadline.  This is the right
logic to see if it's possible to finish activation from the current
moment before the deadline.

This was causing problems where the following events would occur:
  - browser has a 40ms draw
  - browser delivers a begin frame
  - renderer mistakenly thinks the activation fits below the deadline
    even though estimated time > one frame
  - renderer skips this begin main frame
  - browser has another 40ms draw
  - renderer now does a begin main frame, >40ms late

This was causing tab capture performance tests where the browser was
pegged with long draw operations to cause the renderer frame times to go
from 60 to 100ms.

R=sunnyps@chromium.org
BUG=632292
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[enne (OOO), 2016-10-12 20:00:18]

Description was changed from

==========
Fix scheduler bug in skipping main frames

Previously the scheduler would think that it could skip a main frame if
the begin frame's start time + estimated time < deadline.  However,
if this begin frame gets delivered very late, then this is incorrect,
and it's even possible that start time + estimated time < Now() and is
in the past, which doesn't make sense.

Instead, check if Now() + estimated time < deadline.  This is the right
logic to see if it's possible to finish activation from the current
moment before the deadline.

This was causing problems where the following events would occur:
  - browser has a 40ms draw
  - browser delivers a begin frame
  - renderer mistakenly thinks the activation fits below the deadline
    even though estimated time > one frame
  - renderer skips this begin main frame
  - browser has another 40ms draw
  - renderer now does a begin main frame, >40ms late

This was causing tab capture performance tests where the browser was
pegged with long draw operations to cause the renderer frame times to go
from 60 to 100ms.

R=sunnyps@chromium.org
BUG=632292
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Fix scheduler bug in skipping main frames

Previously the scheduler would think that it could skip a main frame if
the begin frame's start time + estimated time < deadline.  However,
if this begin frame gets delivered very late, then this is incorrect,
and it's even possible that start time + estimated time < Now() and is
in the past, which doesn't make sense.

Instead, check if Now() + estimated time < deadline.  This is the right
logic to see if it's possible to finish activation from the current
moment before the deadline.

This was causing problems where the following events would occur:
  - browser has a 40ms draw
  - browser delivers a begin frame
  - renderer mistakenly thinks the activation fits below the deadline
    even though estimated time > one frame
  - renderer skips this begin main frame
  - browser has another 40ms draw
  - renderer now does a begin main frame, >40ms late

This was causing tab capture performance tests where the browser was
pegged with long draw operations to cause the renderer frame times to go
from 60 to 100ms.

R=sunnyps@chromium.org
BUG=632292
==========

[enne (OOO), 2016-10-12 20:21:56]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 20:22:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brianderson, 2016-10-12 20:38:16]

brianderson@chromium.org changed reviewers:
+ brianderson@chromium.org

[brianderson, 2016-10-12 20:38:17]

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:322: Now() > adjusted_args.deadline) {
Would it work to store this Now() and use it in both places?

[sunnyps, 2016-10-12 20:51:46]

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler_unit...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1331: // Advance frame and create a begin
frame.
Why do we need this extra frame in between?

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1340: scheduler_->SetNeedsBeginMainFrame();
nit: Can you call SetNeedsBeginMainFrame before sending the begin frame?

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1344:
EXPECT_TRUE(client_->HasAction("WillBeginImplFrame"));
nit: Can you use EXPECT_ACTION here?

[commit-bot: I haz the power, 2016-10-12 21:49:31]

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

[commit-bot: I haz the power, 2016-10-12 21:49:32]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2016-10-13 17:07:58]

Description was changed from

==========
Fix scheduler bug in skipping main frames

Previously the scheduler would think that it could skip a main frame if
the begin frame's start time + estimated time < deadline.  However,
if this begin frame gets delivered very late, then this is incorrect,
and it's even possible that start time + estimated time < Now() and is
in the past, which doesn't make sense.

Instead, check if Now() + estimated time < deadline.  This is the right
logic to see if it's possible to finish activation from the current
moment before the deadline.

This was causing problems where the following events would occur:
  - browser has a 40ms draw
  - browser delivers a begin frame
  - renderer mistakenly thinks the activation fits below the deadline
    even though estimated time > one frame
  - renderer skips this begin main frame
  - browser has another 40ms draw
  - renderer now does a begin main frame, >40ms late

This was causing tab capture performance tests where the browser was
pegged with long draw operations to cause the renderer frame times to go
from 60 to 100ms.

R=sunnyps@chromium.org
BUG=632292
==========

to

==========
Fix scheduler bug in skipping main frames

Previously the scheduler would think that it could skip a main frame if
the begin frame's start time + estimated time < deadline.  However,
if this begin frame gets delivered very late, then this is incorrect,
and it's even possible that start time + estimated time < Now() and is
in the past, which doesn't make sense.

Instead, check if Now() + estimated time < deadline.  This is the right
logic to see if it's possible to finish activation from the current
moment before the deadline.

This was causing problems where the following events would occur:
  - browser has a 40ms draw
  - browser delivers a begin frame
  - renderer mistakenly thinks the activation fits below the deadline
    even though estimated time > one frame
  - renderer skips this begin main frame
  - browser has another 40ms draw
  - renderer now does a begin main frame, >40ms late

This was causing tab capture performance tests where the browser was
pegged with long draw operations to cause the renderer frame times to go
from 60 to 100ms.

R=sunnyps@chromium.org
BUG=632292
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[enne (OOO), 2016-10-13 17:09:44]

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:322: Now() > adjusted_args.deadline) {
On 2016/10/12 at 20:38:17, brianderson wrote:
> Would it work to store this Now() and use it in both places?

Done.

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:322: Now() > adjusted_args.deadline) {
On 2016/10/12 at 20:38:17, brianderson wrote:
> Would it work to store this Now() and use it in both places?

Done.

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler_unit...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1331: // Advance frame and create a begin
frame.
On 2016/10/12 at 20:51:46, sunnyps wrote:
> Why do we need this extra frame in between?

Which extra frame? There's one frame where the main thread misses the deadline
to get into the MissedLastDeadline state.  Then there's this second frame which
is delivered very late.

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1340: scheduler_->SetNeedsBeginMainFrame();
On 2016/10/12 at 20:51:46, sunnyps wrote:
> nit: Can you call SetNeedsBeginMainFrame before sending the begin frame?

Done.

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1344:
EXPECT_TRUE(client_->HasAction("WillBeginImplFrame"));
On 2016/10/12 at 20:51:46, sunnyps wrote:
> nit: Can you use EXPECT_ACTION here?

Done.

[enne (OOO), 2016-10-13 17:10:02]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 17:10:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 17:58:44]

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

[commit-bot: I haz the power, 2016-10-13 17:58:45]

Dry run: This issue passed the CQ dry run.

[sunnyps, 2016-10-13 20:25:57]

lgtm

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler_unit...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2418593002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1331: // Advance frame and create a begin
frame.
On 2016/10/13 17:09:44, enne wrote:
> On 2016/10/12 at 20:51:46, sunnyps wrote:
> > Why do we need this extra frame in between?
> 
> Which extra frame? There's one frame where the main thread misses the deadline
> to get into the MissedLastDeadline state.  Then there's this second frame
which
> is delivered very late.

Ooh, sorry. Misread that.

[brianderson, 2016-10-13 23:04:58]

lgtm too.

[enne (OOO), 2016-10-14 17:51:14]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-10-14 17:51:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 18:37:53]

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

[commit-bot: I haz the power, 2016-10-14 18:37:54]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[enne (OOO), 2016-10-14 18:54:30]

Hmm, looks like failures are of the form:
[ RUN      ] SchedulerTest.MainFrameNotSkippedAfterLateBeginFrame
../../cc/scheduler/scheduler_unittest.cc:1346: Failure
Value of: client_->Action(2)
  Actual: "ScheduledActionDrawIfPossible"
Expected: "ScheduledActionDrawAndSwapIfPossible"

Which is probably why the code that I cribbed from didn't use EXPECT_ACTION
before.  I will change it back to that so it can be more robust.  This detail is
not what the test is testing.

[enne (OOO), 2016-10-14 19:08:49]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-10-14 19:08:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
sunnyps@chromium.org, brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2418593002/#ps40001
(title: "Rename expected draw function after danakj changes")

[commit-bot: I haz the power, 2016-10-14 19:09:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2016-10-14 19:10:15]

On 2016/10/14 at 18:54:30, enne wrote:
> Hmm, looks like failures are of the form:
> [ RUN      ] SchedulerTest.MainFrameNotSkippedAfterLateBeginFrame
> ../../cc/scheduler/scheduler_unittest.cc:1346: Failure
> Value of: client_->Action(2)
>   Actual: "ScheduledActionDrawIfPossible"
> Expected: "ScheduledActionDrawAndSwapIfPossible"
> 
> Which is probably why the code that I cribbed from didn't use EXPECT_ACTION
before.  I will change it back to that so it can be more robust.  This detail is
not what the test is testing.

Oh nevermind, this is just because of danakj's changes.  *renaming*

[commit-bot: I haz the power, 2016-10-14 20:15:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-14 20:18:23]

Description was changed from

==========
Fix scheduler bug in skipping main frames

Previously the scheduler would think that it could skip a main frame if
the begin frame's start time + estimated time < deadline.  However,
if this begin frame gets delivered very late, then this is incorrect,
and it's even possible that start time + estimated time < Now() and is
in the past, which doesn't make sense.

Instead, check if Now() + estimated time < deadline.  This is the right
logic to see if it's possible to finish activation from the current
moment before the deadline.

This was causing problems where the following events would occur:
  - browser has a 40ms draw
  - browser delivers a begin frame
  - renderer mistakenly thinks the activation fits below the deadline
    even though estimated time > one frame
  - renderer skips this begin main frame
  - browser has another 40ms draw
  - renderer now does a begin main frame, >40ms late

This was causing tab capture performance tests where the browser was
pegged with long draw operations to cause the renderer frame times to go
from 60 to 100ms.

R=sunnyps@chromium.org
BUG=632292
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Fix scheduler bug in skipping main frames

Previously the scheduler would think that it could skip a main frame if
the begin frame's start time + estimated time < deadline.  However,
if this begin frame gets delivered very late, then this is incorrect,
and it's even possible that start time + estimated time < Now() and is
in the past, which doesn't make sense.

Instead, check if Now() + estimated time < deadline.  This is the right
logic to see if it's possible to finish activation from the current
moment before the deadline.

This was causing problems where the following events would occur:
  - browser has a 40ms draw
  - browser delivers a begin frame
  - renderer mistakenly thinks the activation fits below the deadline
    even though estimated time > one frame
  - renderer skips this begin main frame
  - browser has another 40ms draw
  - renderer now does a begin main frame, >40ms late

This was causing tab capture performance tests where the browser was
pegged with long draw operations to cause the renderer frame times to go
from 60 to 100ms.

R=sunnyps@chromium.org
BUG=632292
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/ff408189176e09e6c8160cc3ac6d44bc9846ebef
Cr-Commit-Position: refs/heads/master@{#425449}
==========

[commit-bot: I haz the power, 2016-10-14 20:18:24]

Patchset 3 (id:??) landed as
https://crrev.com/ff408189176e09e6c8160cc3ac6d44bc9846ebef
Cr-Commit-Position: refs/heads/master@{#425449}