Make disable vsync run the renderer independently

Make disable vsync run the renderer independently

https://codereview.chromium.org/2083423006 removed the creation of begin
frame sources inside of cc proxies.  When vsync was disabled, the
browser would run unthrottled but the renderer would still tick based
on the browser.  This regressed some perf tests.  This patch reverts
that situation and reimplements unthrottled renderer begin frame sources
in a different way.

When the command line parameter is present, the renderer now runs
unthrottled by creating a separate unthrottled begin frame source inside
of RenderThreadImpl instead of an external one.

As some clean up, the property of being throttled is baked into the
BeginFrameSource interface itself.  This allows for the removal of the
throttling scheduler setting (and the already unused use external bfs
setting).  This should allow for future patches to allow toggling this
property.

The browser compositor and display compositor continue to be tied
together and tick at the same rate.  Therefore, with this patch,
--disable-gpu-vsync=gpu only applies to the display/browser.
--disable-gpu-vsync=beginframe only applies to the renderer.
--disable-gpu-vsync on its own will unthrottle both.

R=brianderson@chromium.org,sunnyps@chromium.org
BUG=643722, 644926
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[enne (OOO), 2016-09-08 22:14:24]

Description was changed from

==========
Make disable vsync run the renderer independently

https://codereview.chromium.org/2083423006 removed the creation of begin
frame sources inside of cc proxies.  When vsync was disabled, the
browser would run unthrottled but the renderer would still tick based
on the browser.  This regressed some perf tests.  This patch reverts
that situation and reimplements unthrottled renderer begin frame sources
in a different way.

When the command line parameter is present, the renderer now runs
unthrottled by creating a separate unthrottled begin frame source inside
of RenderThreadImpl instead of an external one.

As some clean up, the property of being throttled is baked into the
BeginFrameSource interface itself.  This allows for the removal of the
throttling scheduler setting (and the already unused use external bfs
setting).  This should allow for future patches to allow toggling this
property.

The browser compositor and display compositor continue to be tied
together and tick at the same rate.  Therefore, with this patch,
--disable-gpu-vsync=gpu only applies to the display/browser.
--disable-gpu-vsync=beginframe only applies to the renderer.
--disable-gpu-vsync on its own will unthrottle both.

R=brianderson@chromium.org,sunnyps@chromium.org
BUG=643722,644926
==========

to

==========
Make disable vsync run the renderer independently

https://codereview.chromium.org/2083423006 removed the creation of begin
frame sources inside of cc proxies.  When vsync was disabled, the
browser would run unthrottled but the renderer would still tick based
on the browser.  This regressed some perf tests.  This patch reverts
that situation and reimplements unthrottled renderer begin frame sources
in a different way.

When the command line parameter is present, the renderer now runs
unthrottled by creating a separate unthrottled begin frame source inside
of RenderThreadImpl instead of an external one.

As some clean up, the property of being throttled is baked into the
BeginFrameSource interface itself.  This allows for the removal of the
throttling scheduler setting (and the already unused use external bfs
setting).  This should allow for future patches to allow toggling this
property.

The browser compositor and display compositor continue to be tied
together and tick at the same rate.  Therefore, with this patch,
--disable-gpu-vsync=gpu only applies to the display/browser.
--disable-gpu-vsync=beginframe only applies to the renderer.
--disable-gpu-vsync on its own will unthrottle both.

R=brianderson@chromium.org,sunnyps@chromium.org
BUG=643722,644926
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[enne (OOO), 2016-09-08 22:19:52]

https://codereview.chromium.org/2323123002/diff/1/cc/scheduler/scheduler_sett...
File cc/scheduler/scheduler_settings.h (left):

https://codereview.chromium.org/2323123002/diff/1/cc/scheduler/scheduler_sett...
cc/scheduler/scheduler_settings.h:29: bool use_external_begin_frame_source;
Unused, so removed this too as I had to modify scheduler_unittest to handle
removing throttle_frame_production.

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

https://codereview.chromium.org/2323123002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:218: enum BeginFrameSourceType {
Most of these tests are using external.  I'll probably clean this up to only be
external or unthrottled, but tried to keep these tests as unchanged as possible
here.

https://codereview.chromium.org/2323123002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:2296:
BeginFramesNotFromClient(THROTTLED_BFS);
BeginFramesNotFromClient are really old tests from
https://codereview.chromium.org/221833009.  I think that they don't apply any
more now that everything is using external begin frame sources, but I just left
them for now.

Additionally, for a long time, external=true and throttle=false has not been a
valid state for some time (as throttle=false overrides it and so it's redundant)
and so I removed that case.

https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_pe...
File cc/trees/layer_tree_host_perftest.cc (left):

https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_pe...
cc/trees/layer_tree_host_perftest.cc:49: settings->wait_for_beginframe_interval
= false;
disable_display_vsync creates an unthrottled begin frame source in
TestDelegatingOutputSurface, and the unthrottled begin frame source causes the
scheduler to do the right thing, so this can just be removed.

https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest.cc:2807:
settings->wait_for_beginframe_interval = false;
This is not relevant here, so just removed.

[sunnyps, 2016-09-08 22:31:05]

I only looked at this very briefly but I think I understand the main idea here.
I'm curious if you considered just adding a flag (disable_vsync) to
SchedulerSettings (that's set based on the command line flag) and have the
scheduler see that instead of querying the BFS?

[enne (OOO), 2016-09-08 22:53:42]

On 2016/09/08 at 22:31:05, sunnyps wrote:
> I only looked at this very briefly but I think I understand the main idea
here. I'm curious if you considered just adding a flag (disable_vsync) to
SchedulerSettings (that's set based on the command line flag) and have the
scheduler see that instead of querying the BFS?

I think having the BeginFrameSource know whether its throttled or not removes a
lot of redundant plumbing and prevents having to keep redundant state around. 
That was why I implemented it this way.  I thought that's why you filed issue
444926, as you said "we can't just set an unthrottled BFS on the scheduler and
expect things to work, the scheduler itself has to be aware that vsync is turned
off", in that you wanted to unify the creation of the unthrottled BFS and the
scheduler setting.  My personal feeling is that it's cleaner to just inform the
scheduler of its BFS (which means the BFS needs to know throttling) rather than
to inform the scheduler of not throttling (and have it create some internal BFS,
again).

[sunnyps, 2016-09-08 22:57:10]

On 2016/09/08 22:53:42, enne wrote:
> On 2016/09/08 at 22:31:05, sunnyps wrote:
> > I only looked at this very briefly but I think I understand the main idea
> here. I'm curious if you considered just adding a flag (disable_vsync) to
> SchedulerSettings (that's set based on the command line flag) and have the
> scheduler see that instead of querying the BFS?
> 
> I think having the BeginFrameSource know whether its throttled or not removes
a
> lot of redundant plumbing and prevents having to keep redundant state around. 
> That was why I implemented it this way.  I thought that's why you filed issue
> 444926, as you said "we can't just set an unthrottled BFS on the scheduler and
> expect things to work, the scheduler itself has to be aware that vsync is
turned
> off", in that you wanted to unify the creation of the unthrottled BFS and the
> scheduler setting.  My personal feeling is that it's cleaner to just inform
the
> scheduler of its BFS (which means the BFS needs to know throttling) rather
than
> to inform the scheduler of not throttling (and have it create some internal
BFS,
> again).

I'm not suggesting having the scheduler create an internal BFS here (although I
did in the bug). I'm just suggesting passing the "!IsThrottled" state in
SchedulerSettings instead of via the BFS.

[sunnyps, 2016-09-08 22:59:13]

On 2016/09/08 22:57:10, sunnyps wrote:
> On 2016/09/08 22:53:42, enne wrote:
> > On 2016/09/08 at 22:31:05, sunnyps wrote:
> > > I only looked at this very briefly but I think I understand the main idea
> > here. I'm curious if you considered just adding a flag (disable_vsync) to
> > SchedulerSettings (that's set based on the command line flag) and have the
> > scheduler see that instead of querying the BFS?
> > 
> > I think having the BeginFrameSource know whether its throttled or not
removes
> a
> > lot of redundant plumbing and prevents having to keep redundant state
around. 
> > That was why I implemented it this way.  I thought that's why you filed
issue
> > 444926, as you said "we can't just set an unthrottled BFS on the scheduler
and
> > expect things to work, the scheduler itself has to be aware that vsync is
> turned
> > off", in that you wanted to unify the creation of the unthrottled BFS and
the
> > scheduler setting.  My personal feeling is that it's cleaner to just inform
> the
> > scheduler of its BFS (which means the BFS needs to know throttling) rather
> than
> > to inform the scheduler of not throttling (and have it create some internal
> BFS,
> > again).
> 
> I'm not suggesting having the scheduler create an internal BFS here (although
I
> did in the bug). I'm just suggesting passing the "!IsThrottled" state in
> SchedulerSettings instead of via the BFS.

Also I see that we already pass the throttled state via SchedulerSettings. I was
misremembering how it all worked (there used to be a flag at one point).

[enne (OOO), 2016-09-08 23:04:44]

The other advantage of doing it this way is that the scheduler settings are
const.  Because the begin frame source can be changed out, it means that if we
ever wanted to toggle throttling, we'd be able to more easily.

[enne (OOO), 2016-09-09 17:42:07]

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

[commit-bot: I haz the power, 2016-09-09 17:42:32]

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-09-09 18:03:57]

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

[commit-bot: I haz the power, 2016-09-09 18:03:58]

Dry run: 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_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[enne (OOO), 2016-09-09 18:10:08]

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

[commit-bot: I haz the power, 2016-09-09 18:10:38]

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-09-09 18:29:59]

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

[commit-bot: I haz the power, 2016-09-09 18:30:00]

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...)

[enne (OOO), 2016-09-09 18:42:56]

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

[commit-bot: I haz the power, 2016-09-09 18:43:28]

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-09-09 19:23:38]

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

[commit-bot: I haz the power, 2016-09-09 19:23:39]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[enne (OOO), 2016-09-09 19:51:08]

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

[commit-bot: I haz the power, 2016-09-09 19:51:40]

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-09-09 21:44:15]

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

[commit-bot: I haz the power, 2016-09-09 21:44:16]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2016-09-12 17:35:02]

brianderson: have you had a chance to look at this?

[brianderson, 2016-09-12 19:07:38]

I may have missed something but it looks like LayerTreeHostPerfTest,
LayerTreeHostTestGpuRasterizationReenabled,
LayerTreeHostTestAbortedCommitDoesntStallDisabledVsync used unthrottled sources
before this patch but not after. Not sure if the first two really need it but
the DisabledVSync test probably does.

Otherwise, the patch looks good.

[enne (OOO), 2016-09-12 20:04:38]

On 2016/09/12 at 19:07:38, brianderson wrote:
> I may have missed something but it looks like LayerTreeHostPerfTest,
LayerTreeHostTestGpuRasterizationReenabled,
LayerTreeHostTestAbortedCommitDoesntStallDisabledVsync used unthrottled sources
before this patch but not after. Not sure if the first two really need it but
the DisabledVSync test probably does.

LayerTreeHostPerfTest was still uses unthrottled. 
LayerTreeHostTestGpuRasterizationReenabled doesn't look like it needs this flag
and so was intentionally changed to be throttled.

LayerTreeHostTestAbortedCommitDoesntStallDisabledVsync was to test the scheduler
state machine for logic that doesn't exist anymore, so I've just removed that
test.

https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_pe...
File cc/trees/layer_tree_host_perftest.cc (left):

https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_pe...
cc/trees/layer_tree_host_perftest.cc:49: settings->wait_for_beginframe_interval
= false;
On 2016/09/08 at 22:19:52, enne wrote:
> disable_display_vsync creates an unthrottled begin frame source in
TestDelegatingOutputSurface, and the unthrottled begin frame source causes the
scheduler to do the right thing, so this can just be removed.

This used unthrottled before but with the other changes in the patch to
BeginFrameSource, the scheduler still knows it's unthrottled.

[enne (OOO), 2016-09-12 20:04:46]

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

[commit-bot: I haz the power, 2016-09-12 20:05:15]

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

[brianderson, 2016-09-12 21:01:07]

https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_pe...
File cc/trees/layer_tree_host_perftest.cc (left):

https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_pe...
cc/trees/layer_tree_host_perftest.cc:49: settings->wait_for_beginframe_interval
= false;
On 2016/09/12 20:04:38, enne wrote:
> On 2016/09/08 at 22:19:52, enne wrote:
> > disable_display_vsync creates an unthrottled begin frame source in
> TestDelegatingOutputSurface, and the unthrottled begin frame source causes the
> scheduler to do the right thing, so this can just be removed.
> 
> This used unthrottled before but with the other changes in the patch to
> BeginFrameSource, the scheduler still knows it's unthrottled.

Ah, sorry I missed this comment. lgtm, but maybe add the comment to the code
before landing.

[commit-bot: I haz the power, 2016-09-12 21:01:30]

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

[commit-bot: I haz the power, 2016-09-12 21:01:31]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2016-09-12 21:04:07]

On 2016/09/12 at 21:01:07, brianderson wrote:
>
https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_pe...
> File cc/trees/layer_tree_host_perftest.cc (left):
> 
>
https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_pe...
> cc/trees/layer_tree_host_perftest.cc:49:
settings->wait_for_beginframe_interval = false;
> On 2016/09/12 20:04:38, enne wrote:
> > On 2016/09/08 at 22:19:52, enne wrote:
> > > disable_display_vsync creates an unthrottled begin frame source in
> > TestDelegatingOutputSurface, and the unthrottled begin frame source causes
the
> > scheduler to do the right thing, so this can just be removed.
> > 
> > This used unthrottled before but with the other changes in the patch to
> > BeginFrameSource, the scheduler still knows it's unthrottled.
> 
> Ah, sorry I missed this comment. lgtm, but maybe add the comment to the code
before landing.

I'm a little confused what comment you want.  wait_for_beginframe_interval
doesn't exist anymore, and the test itself still has the "disable_display_vsync
= true" line, which seems enough on its own? What else would you like clarified?

[brianderson, 2016-09-12 21:44:37]

On 2016/09/12 21:04:07, enne wrote:
> On 2016/09/12 at 21:01:07, brianderson wrote:
> >
>
https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_pe...
> > File cc/trees/layer_tree_host_perftest.cc (left):
> > 
> >
>
https://codereview.chromium.org/2323123002/diff/1/cc/trees/layer_tree_host_pe...
> > cc/trees/layer_tree_host_perftest.cc:49:
> settings->wait_for_beginframe_interval = false;
> > On 2016/09/12 20:04:38, enne wrote:
> > > On 2016/09/08 at 22:19:52, enne wrote:
> > > > disable_display_vsync creates an unthrottled begin frame source in
> > > TestDelegatingOutputSurface, and the unthrottled begin frame source causes
> the
> > > scheduler to do the right thing, so this can just be removed.
> > > 
> > > This used unthrottled before but with the other changes in the patch to
> > > BeginFrameSource, the scheduler still knows it's unthrottled.
> > 
> > Ah, sorry I missed this comment. lgtm, but maybe add the comment to the code
> before landing.
> 
> I'm a little confused what comment you want.  wait_for_beginframe_interval
> doesn't exist anymore, and the test itself still has the
"disable_display_vsync
> = true" line, which seems enough on its own? What else would you like
clarified?

My understanding is that the cc::Scheduler will 1) use the
BackToBackBeginFrameSource of the Display if disable_display_vsync=true in the
Browser and 2) a new BackToBackBeginFrameSource in the Renderer based directly
on the command line switch. I incorrectly assumed that this test set things up
like the Renderer, which is why I thought extra logic would be needed to emulate
(2).

So maybe something like:
// LayerTreeTests give the Display's BeginFrameSource directly to the
LayerTreeHost like we do in the Browser process, so setting
disable_display_vsync here unthrottles both the cc::DisplayScheduler and
cc::Scheduler.

[enne (OOO), 2016-09-12 21:50:20]

On 2016/09/12 at 21:44:37, brianderson wrote:

> So maybe something like:
> // LayerTreeTests give the Display's BeginFrameSource directly to the
LayerTreeHost like we do in the Browser process, so setting
disable_display_vsync here unthrottles both the cc::DisplayScheduler and
cc::Scheduler.

Thanks.  Just wanted to make sure I captured what you wanted clarified.

PTAL

[enne (OOO), 2016-09-12 21:50:45]

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

[commit-bot: I haz the power, 2016-09-12 21:51:27]

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

[brianderson, 2016-09-12 21:55:58]

lgtm

[enne (OOO), 2016-09-12 22:00:53]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2016-09-12 22:00:55]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-09-12 22:01:39]

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-09-12 22:10:29]

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

[commit-bot: I haz the power, 2016-09-12 22:10:31]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[enne (OOO), 2016-09-12 22:12:15]

enne@chromium.org changed reviewers:
+ danakj@chromium.org, sievers@chromium.org

[enne (OOO), 2016-09-12 22:13:03]

danakj: ui/compositor OWNERS
sievers: content/ OWNERS

[danakj, 2016-09-12 22:18:00]

https://codereview.chromium.org/2323123002/diff/140001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2323123002/diff/140001/ui/compositor/composit...
ui/compositor/compositor.cc:113: if (display_vsync_string != "beginframe")
Don't you mean != "gpu"?

If "beginframe" then we want to disable vsync here, no?

[danakj, 2016-09-12 22:24:26]

Can you make this comment better?
https://cs.chromium.org/chromium/src/ui/gl/gl_switches.cc?rcl=0&l=34

https://codereview.chromium.org/2323123002/diff/140001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2323123002/diff/140001/ui/compositor/composit...
ui/compositor/compositor.cc:113: if (display_vsync_string != "beginframe")
On 2016/09/12 22:18:00, danakj wrote:
> Don't you mean != "gpu"?
> 
> If "beginframe" then we want to disable vsync here, no?

Can you put a comment here explaining that this is taking the same behaviour as
the Display etc? So it only unthrottles if "gpu" should be unthrottled which
actually means Display. And we do that because Display is in process and if it
was not then we should change this to be be unthrottled when "beginframe" is
unthrottled instead?

[enne (OOO), 2016-09-12 22:40:23]

enne@chromium.org changed reviewers:
+ sadrul@chromium.org

[enne (OOO), 2016-09-12 22:40:24]

sadrul: ui/ OWNERS

On 2016/09/12 at 22:24:26, danakj wrote:
> Can you make this comment better?
https://cs.chromium.org/chromium/src/ui/gl/gl_switches.cc?rcl=0&l=34
> 
>
https://codereview.chromium.org/2323123002/diff/140001/ui/compositor/composit...
> File ui/compositor/compositor.cc (right):
> 
>
https://codereview.chromium.org/2323123002/diff/140001/ui/compositor/composit...
> ui/compositor/compositor.cc:113: if (display_vsync_string != "beginframe")
> On 2016/09/12 22:18:00, danakj wrote:
> > Don't you mean != "gpu"?
> > 
> > If "beginframe" then we want to disable vsync here, no?
> 
> Can you put a comment here explaining that this is taking the same behaviour
as the Display etc? So it only unthrottles if "gpu" should be unthrottled which
actually means Display. And we do that because Display is in process and if it
was not then we should change this to be be unthrottled when "beginframe" is
unthrottled instead?

Sure, done.  I also updated the gl_switches file to reflect what it's doing.

[sadrul, 2016-09-13 20:45:35]

stamp lgtm

[enne (OOO), 2016-09-13 20:47:43]

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

[commit-bot: I haz the power, 2016-09-13 20:48:07]

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

[enne (OOO), 2016-09-13 20:57:22]

danakj: did you have more thoughts on this?

[danakj, 2016-09-13 21:19:02]

ui LGTM

[enne (OOO), 2016-09-13 21:21:27]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2016-09-13 21:21:34]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-09-13 21:21:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2323123002/#ps160001
(title: "Comments")

[commit-bot: I haz the power, 2016-09-13 21:21:54]

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-09-13 21:31:01]

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

[commit-bot: I haz the power, 2016-09-13 21:31:03]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[enne (OOO), 2016-09-14 00:28:20]

sievers: ping?

[enne (OOO), 2016-09-14 20:34:53]

enne@chromium.org changed reviewers:
+ piman@chromium.org

[enne (OOO), 2016-09-14 20:34:54]

piman: content OWNERS?

[piman, 2016-09-14 23:49:25]

quick questions:
1- when the renderer is not throttled, does it still respect the browser acks
before submitting more frames? Otherwise I would be concerned about the lack of
back pressure causing some nasty effects such as having the GPU be arbitrary far
behind (I.e. unbounded latency) and corresponding unbounded memory use.
2- if perf tests are sensitive to the behaviour of disabled vsync, I worry that
they measure the wrong thing. Should we file a bug to investigate?

[sunnyps, 2016-09-14 23:54:57]

On 2016/09/14 23:49:25, piman OOO back 2016-09-12 wrote:
> quick questions:
> 1- when the renderer is not throttled, does it still respect the browser acks
> before submitting more frames? Otherwise I would be concerned about the lack
of
> back pressure causing some nasty effects such as having the GPU be arbitrary
far
> behind (I.e. unbounded latency) and corresponding unbounded memory use.

Yes, cc scheduler (in both browser and renderer) waits for swap ack before
drawing more frames.

> 2- if perf tests are sensitive to the behaviour of disabled vsync, I worry
that
> they measure the wrong thing. Should we file a bug to investigate?

[enne (OOO), 2016-09-15 00:28:28]

On 2016/09/14 at 23:49:25, piman wrote:

> 2- if perf tests are sensitive to the behaviour of disabled vsync, I worry
that they measure the wrong thing. Should we file a bug to investigate?

Well, this is a disabled vsync test, so it makes sense that it was affected by
how disabled vsync begin frames are presented, I think.  The normally throttled
cast tests did not seem to be affected.  I tried reproing this on Linux, but
wasn't able to.

[piman, 2016-09-15 00:50:42]

lgtm

[piman, 2016-09-15 00:51:58]

On 2016/09/15 00:28:28, enne wrote:
> On 2016/09/14 at 23:49:25, piman wrote:
> 
> > 2- if perf tests are sensitive to the behaviour of disabled vsync, I worry
> that they measure the wrong thing. Should we file a bug to investigate?
> 
> Well, this is a disabled vsync test, so it makes sense that it was affected by
> how disabled vsync begin frames are presented, I think.  The normally
throttled
> cast tests did not seem to be affected.  I tried reproing this on Linux, but
> wasn't able to.

Ok, thanks, that makes sense, I think.
I'm not sure we really want to perf test that configuration since we don't ship
it, but that's another question.

[enne (OOO), 2016-09-15 17:31:39]

On 2016/09/15 at 00:51:58, piman wrote:
> On 2016/09/15 00:28:28, enne wrote:
> > On 2016/09/14 at 23:49:25, piman wrote:
> > 
> > > 2- if perf tests are sensitive to the behaviour of disabled vsync, I worry
> > that they measure the wrong thing. Should we file a bug to investigate?
> > 
> > Well, this is a disabled vsync test, so it makes sense that it was affected
by
> > how disabled vsync begin frames are presented, I think.  The normally
throttled
> > cast tests did not seem to be affected.  I tried reproing this on Linux, but
> > wasn't able to.
> 
> Ok, thanks, that makes sense, I think.

Thanks.  If it had been other tests affected by me removing disabled vsync code,
that would have been much more worrisome.  This patch is somewhat speculative
re: that perf test in that I couldn't repro this issue locally, but the change
from the previous patch is a bit of a smoking gun and so I'm hopeful. 
brianderson also said he'd prefer if the renderer ran independently in this
case, so I think this is where we want to be anyway.

[enne (OOO), 2016-09-15 17:31:43]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-09-15 17:32:22]

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-09-15 17:35:18]

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

[commit-bot: I haz the power, 2016-09-15 17:35:20]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[enne (OOO), 2016-09-15 18:56:37]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-09-15 18:56:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
brianderson@chromium.org, piman@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2323123002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-15 18:57:09]

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-09-15 19:57:40]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-15 19:59:23]

Description was changed from

==========
Make disable vsync run the renderer independently

https://codereview.chromium.org/2083423006 removed the creation of begin
frame sources inside of cc proxies.  When vsync was disabled, the
browser would run unthrottled but the renderer would still tick based
on the browser.  This regressed some perf tests.  This patch reverts
that situation and reimplements unthrottled renderer begin frame sources
in a different way.

When the command line parameter is present, the renderer now runs
unthrottled by creating a separate unthrottled begin frame source inside
of RenderThreadImpl instead of an external one.

As some clean up, the property of being throttled is baked into the
BeginFrameSource interface itself.  This allows for the removal of the
throttling scheduler setting (and the already unused use external bfs
setting).  This should allow for future patches to allow toggling this
property.

The browser compositor and display compositor continue to be tied
together and tick at the same rate.  Therefore, with this patch,
--disable-gpu-vsync=gpu only applies to the display/browser.
--disable-gpu-vsync=beginframe only applies to the renderer.
--disable-gpu-vsync on its own will unthrottle both.

R=brianderson@chromium.org,sunnyps@chromium.org
BUG=643722,644926
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make disable vsync run the renderer independently

https://codereview.chromium.org/2083423006 removed the creation of begin
frame sources inside of cc proxies.  When vsync was disabled, the
browser would run unthrottled but the renderer would still tick based
on the browser.  This regressed some perf tests.  This patch reverts
that situation and reimplements unthrottled renderer begin frame sources
in a different way.

When the command line parameter is present, the renderer now runs
unthrottled by creating a separate unthrottled begin frame source inside
of RenderThreadImpl instead of an external one.

As some clean up, the property of being throttled is baked into the
BeginFrameSource interface itself.  This allows for the removal of the
throttling scheduler setting (and the already unused use external bfs
setting).  This should allow for future patches to allow toggling this
property.

The browser compositor and display compositor continue to be tied
together and tick at the same rate.  Therefore, with this patch,
--disable-gpu-vsync=gpu only applies to the display/browser.
--disable-gpu-vsync=beginframe only applies to the renderer.
--disable-gpu-vsync on its own will unthrottle both.

R=brianderson@chromium.org,sunnyps@chromium.org
BUG=643722,644926
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[commit-bot: I haz the power, 2016-09-15 19:59:25]

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