[cc] Don't send missed BeginFrames with past deadline.

[cc] Don't send missed BeginFrames with past deadline.

This avoids unnecessary IPCs since the BeginFrames would be dropped by
the cc::Scheduler in the renderer anyway, see linked bug.

BUG=704895
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[Eric Seckler, 2017-03-22 15:27:45]

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

[commit-bot: I haz the power, 2017-03-22 15:28:17]

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

[Eric Seckler, 2017-03-22 15:30:07]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-22 15:49:37]

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

[commit-bot: I haz the power, 2017-03-22 15:49:38]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Eric Seckler, 2017-03-22 16:07:06]

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

[commit-bot: I haz the power, 2017-03-22 16:07:33]

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

[Eric Seckler, 2017-03-22 16:20:27]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-22 16:44:48]

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

[commit-bot: I haz the power, 2017-03-22 16:44:49]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-03-24 10:13:40]

Description was changed from

==========
[content] Don't send missed BeginFrames with past deadline to renderer.

BUG=
==========

to

==========
[content] Don't send missed BeginFrames with past deadline to renderer.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-03-24 10:27:06]

Description was changed from

==========
[content] Don't send missed BeginFrames with past deadline to renderer.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Don't send missed BeginFrames with past deadline to renderer.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-03-24 10:27:25]

Description was changed from

==========
[cc] Don't send missed BeginFrames with past deadline to renderer.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Don't send missed BeginFrames with past deadline.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-03-24 10:27:37]

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

[commit-bot: I haz the power, 2017-03-24 10:28:00]

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, 2017-03-24 11:22:01]

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

[commit-bot: I haz the power, 2017-03-24 11:22:02]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-03-24 12:27:09]

Description was changed from

==========
[cc] Don't send missed BeginFrames with past deadline.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Don't send missed BeginFrames with past deadline.

BUG=704895
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-03-24 12:27:53]

Description was changed from

==========
[cc] Don't send missed BeginFrames with past deadline.

BUG=704895
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Don't send missed BeginFrames with past deadline.

This avoids unnecessary IPCs since the BeginFrames would be dropped by the
cc::Scheduler in the renderer anyway, see linked bug.

BUG=704895
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-03-24 12:28:04]

Description was changed from

==========
[cc] Don't send missed BeginFrames with past deadline.

This avoids unnecessary IPCs since the BeginFrames would be dropped by the
cc::Scheduler in the renderer anyway, see linked bug.

BUG=704895
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Don't send missed BeginFrames with past deadline.

This avoids unnecessary IPCs since the BeginFrames would be dropped by
the cc::Scheduler in the renderer anyway, see linked bug.

BUG=704895
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-03-24 12:36:41]

eseckler@chromium.org changed reviewers:
+ brianderson@chromium.org, piman@chromium.org

[Eric Seckler, 2017-03-24 12:36:42]

Antoine, Brian, PTAL :)

This seems to reduce the thread_IO_cpu_time_per_second and
tasks_per_second_total_all for animated-gif.html, which regressed by my prior
change (https://crbug.com/703098), see perf bot results:

https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h...

Thanks!

[Eric Seckler, 2017-03-24 13:48:07]

+cc Fady FYI

[brianderson, 2017-03-24 19:57:28]

https://codereview.chromium.org/2767933002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2767933002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:202: if (current_begin_frame_args_.deadline >
time_source_->Now()) {
Can you swap the nested if's, which would avoid making a syscall to get the
timestamp in some cases?

https://codereview.chromium.org/2767933002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:363: missed_begin_frame_args_.deadline >
Now()) {
The changes in this file duplicate logic in
Scheduler::BeginImplFrameWithDeadline. Can we remove the dupe in Scheduler?

https://codereview.chromium.org/2767933002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2767933002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:315: virtual base::TimeTicks Now() const;
Tests to come?

https://codereview.chromium.org/2767933002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2767933002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:2015:
base::TimeTicks::Now() > adjusted_args.deadline) {
This will call Now() once per RWHV. Not a big deal on Android, but if this is a
pattern we're going to repeat elsewhere, should we pass in now as an argument to
OnBeginFrame?

Looking at other BeginFrameObservers, they don't always use Now(), so probably
not.

[Eric Seckler, 2017-03-27 09:53:25]

https://codereview.chromium.org/2767933002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2767933002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:202: if (current_begin_frame_args_.deadline >
time_source_->Now()) {
On 2017/03/24 19:57:28, brianderson wrote:
> Can you swap the nested if's, which would avoid making a syscall to get the
> timestamp in some cases?

Actually, I think that DelayBasedBFS can't ever issue past deadlines anyway.
(The time source query above should avoid that.) So removed this if-statement
again.

I did swap the if-branches in ExternalBFS though. And I'm also special-casing
deadline == 0 there now, since that's what android's sync-compositor uses.

https://codereview.chromium.org/2767933002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:363: missed_begin_frame_args_.deadline >
Now()) {
On 2017/03/24 19:57:28, brianderson wrote:
> The changes in this file duplicate logic in
> Scheduler::BeginImplFrameWithDeadline. Can we remove the dupe in Scheduler?

I think it's fine to leave the logic in the cc::Scheduler, as the time may
change between when the browser issues a missed BF and when the renderer's
Scheduler receives it. (And since we need the call to Now() in Scheduler for
other reasons it doesn't add much overhead there.)

https://codereview.chromium.org/2767933002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2767933002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:315: virtual base::TimeTicks Now() const;
On 2017/03/24 19:57:28, brianderson wrote:
> Tests to come?

Yeah, added some :)

https://codereview.chromium.org/2767933002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2767933002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:2015:
base::TimeTicks::Now() > adjusted_args.deadline) {
On 2017/03/24 19:57:28, brianderson wrote:
> This will call Now() once per RWHV. Not a big deal on Android, but if this is
a
> pattern we're going to repeat elsewhere, should we pass in now as an argument
to
> OnBeginFrame?
> 
> Looking at other BeginFrameObservers, they don't always use Now(), so probably
> not.

Yeay, the only reason we need it on android is the deadline adjustment above. No
other RWHVs do that. I think that's also the reason why we see "past-deadline"
missed BeginFrames in the renderer primarily on android.

[Eric Seckler, 2017-03-27 12:40:04]

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

[commit-bot: I haz the power, 2017-03-27 12:40:13]

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, 2017-03-27 13:58:39]

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

[commit-bot: I haz the power, 2017-03-27 13:58:40]

Dry run: This issue passed the CQ dry run.