cc: Allow sending BeginMainFrame before draw or activation

cc: Allow sending BeginMainFrame before draw or activation

BeginMainFrame before draw:
Enabled by default because of concurrency benefits.
When main thread painting, this will block the main thread from
finishing the commit until the active tree has been drawn.
When impl side painting, this does not block the main thread.

The following flag will disable sending BeginMainFrame to the
main thread before we draw the previous commit:
--disable-main-frame-before-draw

BeginMainFrame before activation:
Disabled by default because of latency concerns.
This applies only when impl-side-painting is enabled and will
cause the main thread to block finishing the commit until the
pending tree is activated.

The following flags control whether BeginMainFrame is sent to
the main thread before or after the previous commit has
activated:
--enable-main-frame-before-activation
--disable-main-frame-before-activation

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

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

[brianderson, 2013-09-07 01:18:04]

This is a much simpler and generalized version of
https://codereview.chromium.org/22320019

[brianderson, 2013-09-07 02:05:08]

Blocking from the scheduler state machine seems to work pretty well and is a
simpler solution. Thanks for the suggestion, Sami.

[brianderson, 2013-09-16 23:27:06]

The difference between patch set 1, which blocks the main thread towards the
beginning of the commit, and patch set 2, which blocks towards the end of the
commit, is that patch set 2 allows the synchronous resource updates (texture
uploads) to start and complete earlier.

At first, I thought that this might increase memory usage when we are not
impl-side painting, since we could be uploading textures before the previous
frame has drawn.  However, we've always had to support having two frames worth
of textures because ToT can't discard the previous frame's textures until the
commit has completed either.

+reveman and +vmpstr for verification of my claim that memory usage should not
increase.  If true, I don't see a reason to go back to the implementation in
patch set 1.

[reveman, 2013-09-17 14:16:26]

On 2013/09/16 23:27:06, Brian Anderson wrote:
> The difference between patch set 1, which blocks the main thread towards the
> beginning of the commit, and patch set 2, which blocks towards the end of the
> commit, is that patch set 2 allows the synchronous resource updates (texture
> uploads) to start and complete earlier.
> 
> At first, I thought that this might increase memory usage when we are not
> impl-side painting, since we could be uploading textures before the previous
> frame has drawn.  However, we've always had to support having two frames worth
> of textures because ToT can't discard the previous frame's textures until the
> commit has completed either.

Correct, this should not affect texture memory usage as each new commit will use
a new set of textures except for partial updates but those happen during commit.

> 
> +reveman and +vmpstr for verification of my claim that memory usage should not
> increase.  If true, I don't see a reason to go back to the implementation in
> patch set 1.

[danakj, 2013-09-17 17:05:07]

Can you add tests that show the different behaviour with the 2 settings on vs
off?

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
File cc/scheduler/scheduler_state_machine.cc (left):

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine.cc:355: DCHECK_EQ(commit_state_,
COMMIT_STATE_WAITING_FOR_FIRST_DRAW);
Should we DCHECK(active_tree_waiting_for_first_draw_)? The unit tests are
setting this true when they set this state.

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine.cc:377: DCHECK_EQ(commit_state_,
COMMIT_STATE_WAITING_FOR_FIRST_DRAW);
Should we DCHECK(active_tree_waiting_for_first_draw_)? The unit tests are
setting this true when they set this state.

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine_unittest.cc:71:
active_tree_needs_first_draw_ = true;
Can you explain these? I don't see that the state machine is necessarily in this
state when forced redraw or readbacking?

[nduca, 2013-10-02 19:03:46]

Dumb question: does this overlapping policy trigger only when we know our
critical path is exceeding 16ms?

[brianderson, 2013-10-02 20:38:13]

On 2013/10/02 19:03:46, nduca wrote:
> Dumb question: does this overlapping policy trigger only when we know our
> critical path is exceeding 16ms?

With deadline scheduling enabled, that will be the case, since the deadline
scheduler ties the start of the commit to the BeginFrame on the impl thread. 
This patch needs a bit more work to make the same guarantee when deadline
scheduling is disabled.

[Sami, 2013-10-03 13:31:11]

> With deadline scheduling enabled, that will be the case, since the deadline
> scheduler ties the start of the commit to the BeginFrame on the impl thread. 
> This patch needs a bit more work to make the same guarantee when deadline
> scheduling is disabled.

So essentially this makes it a little easier for us to recover back to 60 fps if
we have occasional long draws? I like it.

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine.cc:451: if (active_tree_needs_first_draw_
&&
It's a little hard to see how this condition relates to
settings_.impl_side_painting as suggested by the comment. Maybe split this into
two if statements that explicitly check for settings_.impl_side_painting?

[brianderson, 2013-10-03 14:44:37]

>> With deadline scheduling enabled, that will be the case, since the deadline
>> scheduler ties the start of the commit to the BeginFrame on the impl thread. 
>> This patch needs a bit more work to make the same guarantee when deadline
>> scheduling is disabled.

> So essentially this makes it a little easier for us to recover back to 60 fps
if
> we have occasional long draws? I like it.

Even better: This will allow us to hit 60fps if the commit takes 16ms and
rasterization/activation takes 16ms consistently by going into a higher-latency
pipelined mode. This should also help with the occasional long draw (as long as
it isn't too long) while staying in a low-latency mode.

The retroactive begin frames are another feature that work with this to recover
from occasional long draws, for which we really need support for deadlines past
the next frame's frame_time to be able to recover from really long "draws"
(https://codereview.chromium.org/22354007), where "draws" can include garbage
collection and layout on the main thread.

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
File cc/scheduler/scheduler_state_machine.cc (left):

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine.cc:355: DCHECK_EQ(commit_state_,
COMMIT_STATE_WAITING_FOR_FIRST_DRAW);
On 2013/09/17 17:05:07, danakj wrote:
> Should we DCHECK(active_tree_waiting_for_first_draw_)? The unit tests are
> setting this true when they set this state.

Yes, that would be a good DCHECK to add.

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine.cc:451: if (active_tree_needs_first_draw_
&&
On 2013/10/03 13:31:11, Sami wrote:
> It's a little hard to see how this condition relates to
> settings_.impl_side_painting as suggested by the comment. Maybe split this
into
> two if statements that explicitly check for settings_.impl_side_painting?

The decision to be made here is the same regardless of whether
impl-side-painting is on or off, it's just what might happen afterwards that is
different. I will clarify the comment.

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/23907006/diff/3001/cc/scheduler/scheduler_sta...
cc/scheduler/scheduler_state_machine_unittest.cc:71:
active_tree_needs_first_draw_ = true;
On 2013/09/17 17:05:07, danakj wrote:
> Can you explain these? I don't see that the state machine is necessarily in
this
> state when forced redraw or readbacking?

You are right. I shouldn't be setting active_tree_needs_first_draw_ here. 

However, it is valid for SetNeedsForcedRedrawForReadback because, in order to
get into the READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK state, we must have
just come from a state that resulted in a new active tree (either
READBACK_STATE_WAITING_FOR_ACTIVATION when impl-side-painting, or
READBACK_STATE_WAITING_FOR_COMMIT when not impl-side-painting).

[brianderson, 2013-10-04 17:26:51]

A heads up that ToT, just a few months ago, used to start commits before
activation in some cases if the active tree was drawn twice. Without the proper
back pressure.

See https://code.google.com/p/chromium/issues/detail?id=287756 and
https://chromiumcodereview.appspot.com/23503003/#msg27 for background.

[brianderson, 2014-01-17 00:56:49]

I would like to move ahead with this patch again and enable starting commits
before draws by default.

Compared to starting before activation, starting before draw:
1) Doesn't break as many unit test assumptions.
2) Won't increase main-thread latency much.
3) Shouldn't have deadlock issues.

Starting before activation might not deadlock either, but it's riskier since we
have to block the main thread from finishing the commit until activation.
Activation could be blocked on draw, which could be blocked on swap ack, which
could be blocked on Browser, which could be blocked on Renderer Main in an NPAPI
plugin.

[brianderson, 2014-01-22 02:34:25]

PTAL.

I think my previous concern regarding deadlocks was overblown. With the deadline
scheduler, we no longer start a commit if we can't draw+swap the active tree, so
we should never be in the middle of a commit when the impl thread can't make
forward progress (to either draw the active tree or activate the pending tree).

[enne (OOO), 2014-01-23 21:51:54]

https://codereview.chromium.org/23907006/diff/461001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23907006/diff/461001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:376: if
(SchedulerStateMachine::COMMIT_STATE_FRAME_IN_PROGRESS ==
Why do you only update the anticipated draw time during commit?

https://codereview.chromium.org/23907006/diff/461001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.h (left):

https://codereview.chromium.org/23907006/diff/461001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.h:64: COMMIT_STATE_WAITING_FOR_FIRST_DRAW,
I'm not sure I follow the removal of this state.  Can that be done in a separate
patch?

[brianderson, 2014-01-23 21:52:02]

After talking with @enne a bit, I decided to make this patch a little more
conservative and only start the commit early if we aren't in prefer smoothness
mode. This way, we reduce CPU contention on low core devices when impl-thread
driven, but still get the concurrency benefit if we are main-thread driven.

[brianderson, 2014-01-23 23:31:34]

https://codereview.chromium.org/23907006/diff/461001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23907006/diff/461001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:376: if
(SchedulerStateMachine::COMMIT_STATE_FRAME_IN_PROGRESS ==
On 2014/01/23 21:51:55, enne wrote:
> Why do you only update the anticipated draw time during commit?

It avoided a PostTask storm of some sort. I'll see if there is a better
solution.

https://codereview.chromium.org/23907006/diff/461001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.h (left):

https://codereview.chromium.org/23907006/diff/461001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.h:64: COMMIT_STATE_WAITING_FOR_FIRST_DRAW,
On 2014/01/23 21:51:55, enne wrote:
> I'm not sure I follow the removal of this state.  Can that be done in a
separate
> patch?

At ToT, COMMIT_STATE_WAITING_FOR_FIRST_DRAW can be replaced with
(has_pending_tree_ || active_tree_needs_first_draw_), so removing it in a
separate patch would be pretty easy.

I included it in this patch because I needed to decouple draw scheduling from
commit scheduling. By default, this patch starts the next commit before drawing
the previous commit and we'd never enter this state. Commit throttling is done
implicitly now via has_pending_tree_, active_tree_needs_first_draw_, and the
current settings.

I suppose I could keep the state and add COMMIT_STATE_WAITING_FOR_ACTIVATION to
be explicit about what the commit is waiting for, but that would complicate the
state transition logic and encode redundant state with the has_pending_tree and
active_tree_needs_first_draw_ variables.

[brianderson, 2014-02-07 22:20:11]

I went ahead and added COMMIT_STATE_WAITING_FOR_ACTIVATION instead of getting
rid of COMMIT_STATE_WAITING_FOR_FIRST_DRAW. This way, what the commit is waiting
for is very obvious in traces.

PTAL.

[enne (OOO), 2014-02-14 22:53:34]

I really like these new states.  lgtm

[enne (OOO), 2014-02-14 22:53:58]

https://codereview.chromium.org/23907006/diff/791001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23907006/diff/791001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:458: // of smoothnes_takes_priority.
typo

[brianderson, 2014-02-15 00:26:29]

I am going to wait until after the M34 branch point to land this.

There was a flaky test in the previous run, but it wasn't caused by this patch,
so I've opened:
https://code.google.com/p/chromium/issues/detail?id=344078

[Sami, 2014-02-18 18:45:49]

I think this looks great, thanks Brian. lgtm modulo my question about the test.

https://codereview.chromium.org/23907006/diff/1021001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/23907006/diff/1021001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.cc:12:
start_commit_before_activate_enabled(false),
Should we dcheck in the scheduler constructor that
start_commit_before_activate_enabled implies start_commit_before_draw_enabled?
Just being paranoid :)

Also, maybe I missed it but should we have a test that explicitly turns on
start_commit_before_activate_enabled?

https://codereview.chromium.org/23907006/diff/1021001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23907006/diff/1021001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:457: // Don't start commits early if we
are prioritizing the active tree because
Should this comment talk about BeginMainFrame instead of commits?

[brianderson, 2014-02-20 00:25:35]

https://codereview.chromium.org/23907006/diff/1021001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_settings.cc (right):

https://codereview.chromium.org/23907006/diff/1021001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_settings.cc:12:
start_commit_before_activate_enabled(false),
On 2014/02/18 18:45:50, Sami wrote:
> Should we dcheck in the scheduler constructor that
> start_commit_before_activate_enabled implies start_commit_before_draw_enabled?
> Just being paranoid :)
> 

A DCHECK sounds like a good idea.

> Also, maybe I missed it but should we have a test that explicitly turns on
> start_commit_before_activate_enabled?

I'll work on a scheduler unit test before landing. I'll probably avoid enabling
it in any of the layer tree tests until we might enable it by default.

https://codereview.chromium.org/23907006/diff/1021001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23907006/diff/1021001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:457: // Don't start commits early if we
are prioritizing the active tree because
On 2014/02/18 18:45:50, Sami wrote:
> Should this comment talk about BeginMainFrame instead of commits?

Ok. That'll be good for consistency.

[brianderson, 2014-02-28 21:15:27]

Adding owners for OWNERS reviews.

danakj:
  ui/compositor/compositor.cc
kbr:
  content/browser/renderer_host/render_process_host_impl.cc
  content/renderer/gpu/render_widget_compositor.cc
jam:
  chrome/browser/chromeos/login/chrome_restart_request.cc
  content/browser/renderer_host/render_process_host_impl.cc
  content/renderer/gpu/render_widget_compositor.cc

PTAL, thanks!

[brianderson, 2014-02-28 21:18:20]

https://codereview.chromium.org/23907006/diff/1191001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23907006/diff/1191001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:519: if (active_tree_needs_first_draw_)
Thanks Sami for suggesting a test for when we enable commit before activate. I
realized this condition was causing us to prioritize committing the next commit
before drawing the previous commit after an activation.

[danakj, 2014-02-28 21:19:11]

https://codereview.chromium.org/23907006/diff/1211001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/23907006/diff/1211001/ui/compositor/composito...
ui/compositor/compositor.cc:213: settings.start_commit_before_draw_enabled =
Did you collect any data for the browser compositor that suggests they are a
good idea there too? The two environments are quite different (commit/paint is
much faster and controlled in the browser).

[Ken Russell (switch to Gerrit), 2014-02-28 21:22:02]

LGTM for the files I own.

[brianderson, 2014-02-28 21:36:27]

https://codereview.chromium.org/23907006/diff/1211001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/23907006/diff/1211001/ui/compositor/composito...
ui/compositor/compositor.cc:213: settings.start_commit_before_draw_enabled =
On 2014/02/28 21:19:12, danakj wrote:
> Did you collect any data for the browser compositor that suggests they are a
> good idea there too? The two environments are quite different (commit/paint is
> much faster and controlled in the browser).

I have not. Theoretically, this wont make a difference if the Browser compositor
is fast but will allow more concurrency if it is slow.

This also won't make as much sense once we're using SingleThreadProxy+Scheduler
in the Browser because this mainly gives us the benefit of drawing and
committing concurrently when there is an Impl thread and Main thread.

With SingleThreadProxy, prioritizing the commit will defer the draw and will
only give us a concurrency benefit if we can overlap the draw with
impl-side-painting.

Sadly, I was hoping we could get away with just testing the
start_commit_before_draw path. For the LayerTreeHostImpl unit tests, we can
probably get away with tying the value of start_commit_before_draw to
SingleThreadProxy once it has a Scheduler. We'll probably need to fork the
scheduler's unit test though.

[danakj, 2014-02-28 21:52:57]

https://codereview.chromium.org/23907006/diff/1211001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/23907006/diff/1211001/ui/compositor/composito...
ui/compositor/compositor.cc:213: settings.start_commit_before_draw_enabled =
On 2014/02/28 21:36:28, brianderson wrote:
> On 2014/02/28 21:19:12, danakj wrote:
> > Did you collect any data for the browser compositor that suggests they are a
> > good idea there too? The two environments are quite different (commit/paint
is
> > much faster and controlled in the browser).
> 
> I have not. Theoretically, this wont make a difference if the Browser
compositor
> is fast but will allow more concurrency if it is slow.

I'm worried that if we commit sooner we miss renderer frames or user input more
often and increase latency by a frame.

> This also won't make as much sense once we're using
SingleThreadProxy+Scheduler
> in the Browser because this mainly gives us the benefit of drawing and
> committing concurrently when there is an Impl thread and Main thread.

This also depends on a pending tree being present then?

> With SingleThreadProxy, prioritizing the commit will defer the draw and will
> only give us a concurrency benefit if we can overlap the draw with
> impl-side-painting.

With single-thread impl-side-painting we're expecting to commit directly to the
active tree, so committing before draw would clobber the active tree.

> Sadly, I was hoping we could get away with just testing the
> start_commit_before_draw path. For the LayerTreeHostImpl unit tests, we can
> probably get away with tying the value of start_commit_before_draw to
> SingleThreadProxy once it has a Scheduler. We'll probably need to fork the
> scheduler's unit test though.

[brianderson, 2014-02-28 22:30:51]

On 2014/02/28 21:52:57, danakj wrote:
>
https://codereview.chromium.org/23907006/diff/1211001/ui/compositor/composito...
> File ui/compositor/compositor.cc (right):
> 
>
https://codereview.chromium.org/23907006/diff/1211001/ui/compositor/composito...
> ui/compositor/compositor.cc:213: settings.start_commit_before_draw_enabled =
> On 2014/02/28 21:36:28, brianderson wrote:
> > On 2014/02/28 21:19:12, danakj wrote:
> > > Did you collect any data for the browser compositor that suggests they are
a
> > > good idea there too? The two environments are quite different
(commit/paint
> is
> > > much faster and controlled in the browser).
> > 
> > I have not. Theoretically, this wont make a difference if the Browser
> compositor
> > is fast but will allow more concurrency if it is slow.
> 
> I'm worried that if we commit sooner we miss renderer frames or user input
more
> often and increase latency by a frame.

We will not miss user input, especially assuming it is aligned with the
BeginImplFrame. Even if it's not aligned, drift between the input and draw will
not affect average latency over a long period of time.

It will affect how the Browser consumes Renderer frames, but it could actually
improve latency/throughput if it allows the swap ack to return to the Renderer
sooner. Also, we usually wont start a commit on the Browser unless a Renderer
has already submitted a frame, so we could only possibly miss a Renderer frame
when there are multiple Renderers submitting frames. Once the "losing" Renderer
misses a frame though, it enters a high latency mode and will be much less
likely to miss subsequent frames.

> 
> > This also won't make as much sense once we're using
> SingleThreadProxy+Scheduler
> > in the Browser because this mainly gives us the benefit of drawing and
> > committing concurrently when there is an Impl thread and Main thread.
> 
> This also depends on a pending tree being present then?

I keep using the terms BeginMainFrame and commit interchangeably when I
shouldn't. The commit is only the end of the BeginMainFrame. I should have said
we get concurrency between the BeginMainFrame and the draw, which doesn't
necessarily require a pending tree being present.

> 
> > With SingleThreadProxy, prioritizing the commit will defer the draw and will
> > only give us a concurrency benefit if we can overlap the draw with
> > impl-side-painting.
> 
> With single-thread impl-side-painting we're expecting to commit directly to
the
> active tree, so committing before draw would clobber the active tree.

See previous comment. We could get concurrency without clobbering the active
tree.

> 
> > Sadly, I was hoping we could get away with just testing the
> > start_commit_before_draw path. For the LayerTreeHostImpl unit tests, we can
> > probably get away with tying the value of start_commit_before_draw to
> > SingleThreadProxy once it has a Scheduler. We'll probably need to fork the
> > scheduler's unit test though.

[danakj, 2014-02-28 22:40:52]

On Fri, Feb 28, 2014 at 5:30 PM, <brianderson@chromium.org> wrote:

> On 2014/02/28 21:52:57, danakj wrote:
>
> https://codereview.chromium.org/23907006/diff/1211001/ui/
> compositor/compositor.cc
>
>> File ui/compositor/compositor.cc (right):
>>
>
>
> https://codereview.chromium.org/23907006/diff/1211001/ui/
> compositor/compositor.cc#newcode213
>
>> ui/compositor/compositor.cc:213: settings.start_commit_before_draw_enabled
>> =
>> On 2014/02/28 21:36:28, brianderson wrote:
>> > On 2014/02/28 21:19:12, danakj wrote:
>> > > Did you collect any data for the browser compositor that suggests
>> they are
>>
> a
>
>> > > good idea there too? The two environments are quite different
>>
> (commit/paint
>
>> is
>> > > much faster and controlled in the browser).
>> >
>> > I have not. Theoretically, this wont make a difference if the Browser
>> compositor
>> > is fast but will allow more concurrency if it is slow.
>>
>
>  I'm worried that if we commit sooner we miss renderer frames or user input
>>
> more
>
>> often and increase latency by a frame.
>>
>
> We will not miss user input, especially assuming it is aligned with the
> BeginImplFrame. Even if it's not aligned, drift between the input and draw
> will
> not affect average latency over a long period of time.
>
> It will affect how the Browser consumes Renderer frames, but it could
> actually
> improve latency/throughput if it allows the swap ack to return to the
> Renderer
> sooner. Also, we usually wont start a commit on the Browser unless a
> Renderer
> has already submitted a frame, so we could only possibly miss a Renderer
> frame
> when there are multiple Renderers submitting frames. Once the "losing"
> Renderer
> misses a frame though, it enters a high latency mode and will be much less
> likely to miss subsequent frames.


Is this true when there is a loading spinner animating in the browser
causing continuous commits, or when a user is moving a window around
causing commits? (That's the user input I meant not input on a webpage btw.)

Sorry if I'm being annoying here. We're doing a lot of work to reduce
single frames of latency, so I'd like to make sure we understand what we're
doing to the browser compositor here.

 > This also won't make as much sense once we're using
>> SingleThreadProxy+Scheduler
>> > in the Browser because this mainly gives us the benefit of drawing and
>> > committing concurrently when there is an Impl thread and Main thread.
>>
>
>  This also depends on a pending tree being present then?
>>
>
> I keep using the terms BeginMainFrame and commit interchangeably when I
> shouldn't. The commit is only the end of the BeginMainFrame. I should have
> said
> we get concurrency between the BeginMainFrame and the draw, which doesn't
> necessarily require a pending tree being present.
>
>
>
>  > With SingleThreadProxy, prioritizing the commit will defer the draw and
>> will
>> > only give us a concurrency benefit if we can overlap the draw with
>> > impl-side-painting.
>>
>
>  With single-thread impl-side-painting we're expecting to commit directly
>> to
>>
> the
>
>> active tree, so committing before draw would clobber the active tree.
>>
>
> See previous comment. We could get concurrency without clobbering the
> active
>
> tree.
>
>
>  > Sadly, I was hoping we could get away with just testing the
>> > start_commit_before_draw path. For the LayerTreeHostImpl unit tests, we
>> can
>> > probably get away with tying the value of start_commit_before_draw to
>> > SingleThreadProxy once it has a Scheduler. We'll probably need to fork
>> the
>> > scheduler's unit test though.
>>
>
>
> https://codereview.chromium.org/23907006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2014-03-01 01:58:42]

On 2014/02/28 22:40:52, danakj wrote:
> On Fri, Feb 28, 2014 at 5:30 PM, <mailto:brianderson@chromium.org> wrote:
> 
> > On 2014/02/28 21:52:57, danakj wrote:
> >
> > https://codereview.chromium.org/23907006/diff/1211001/ui/
> > compositor/compositor.cc
> >
> >> File ui/compositor/compositor.cc (right):
> >>
> >
> >
> > https://codereview.chromium.org/23907006/diff/1211001/ui/
> > compositor/compositor.cc#newcode213
> >
> >> ui/compositor/compositor.cc:213: settings.start_commit_before_draw_enabled
> >> =
> >> On 2014/02/28 21:36:28, brianderson wrote:
> >> > On 2014/02/28 21:19:12, danakj wrote:
> >> > > Did you collect any data for the browser compositor that suggests
> >> they are
> >>
> > a
> >
> >> > > good idea there too? The two environments are quite different
> >>
> > (commit/paint
> >
> >> is
> >> > > much faster and controlled in the browser).
> >> >
> >> > I have not. Theoretically, this wont make a difference if the Browser
> >> compositor
> >> > is fast but will allow more concurrency if it is slow.
> >>
> >
> >  I'm worried that if we commit sooner we miss renderer frames or user input
> >>
> > more
> >
> >> often and increase latency by a frame.
> >>
> >
> > We will not miss user input, especially assuming it is aligned with the
> > BeginImplFrame. Even if it's not aligned, drift between the input and draw
> > will
> > not affect average latency over a long period of time.
> >
> > It will affect how the Browser consumes Renderer frames, but it could
> > actually
> > improve latency/throughput if it allows the swap ack to return to the
> > Renderer
> > sooner. Also, we usually wont start a commit on the Browser unless a
> > Renderer
> > has already submitted a frame, so we could only possibly miss a Renderer
> > frame
> > when there are multiple Renderers submitting frames. Once the "losing"
> > Renderer
> > misses a frame though, it enters a high latency mode and will be much less
> > likely to miss subsequent frames.
> 
> 
> Is this true when there is a loading spinner animating in the browser
> causing continuous commits, or when a user is moving a window around
> causing commits? (That's the user input I meant not input on a webpage btw.)
> 
> Sorry if I'm being annoying here. We're doing a lot of work to reduce
> single frames of latency, so I'd like to make sure we understand what we're
> doing to the browser compositor here.

Not annoying. I'm glad you got me to think about how this would work with the
SingleThreadProxy as a SchedulerClient. We definitely don't want to start the
BeginMainFrame before the draw in that case, since we won't get the concurrency
benefits.

For ThreadProxy, the spinner / window move cases will put any active Renderers
into a high latency mode today already. Starting the commit earlier will send
the swap ack back to the Renderer earlier though. To really avoid Renderers
entering a high latency mode in these cases, we need commitless updates so the
browser can commit and start rasterization and then give the renderers up until
a deadline to submit their frames.

Since we are headed towards SingleThreadProxy as a SchedulerClient though, I
think we should keep the old behavior around. I'll just need to fork some of the
Scheduler tests. To improve the swap ack latency, maybe we can send the swap
acks before a draw if we know that we are going to commit immediately after the
draw, before any Renderers can submit another frame on the message loop.


> 
>  > This also won't make as much sense once we're using
> >> SingleThreadProxy+Scheduler
> >> > in the Browser because this mainly gives us the benefit of drawing and
> >> > committing concurrently when there is an Impl thread and Main thread.
> >>
> >
> >  This also depends on a pending tree being present then?
> >>
> >
> > I keep using the terms BeginMainFrame and commit interchangeably when I
> > shouldn't. The commit is only the end of the BeginMainFrame. I should have
> > said
> > we get concurrency between the BeginMainFrame and the draw, which doesn't
> > necessarily require a pending tree being present.
> >
> >
> >
> >  > With SingleThreadProxy, prioritizing the commit will defer the draw and
> >> will
> >> > only give us a concurrency benefit if we can overlap the draw with
> >> > impl-side-painting.
> >>
> >
> >  With single-thread impl-side-painting we're expecting to commit directly
> >> to
> >>
> > the
> >
> >> active tree, so committing before draw would clobber the active tree.
> >>
> >
> > See previous comment. We could get concurrency without clobbering the
> > active
> >
> > tree.

You are correct. I was wrong here regarding the SingleThreadProxy case. The
commit would get clobbered because the BeginMainFrame implies commit before the
draw.

> >
> >
> >  > Sadly, I was hoping we could get away with just testing the
> >> > start_commit_before_draw path. For the LayerTreeHostImpl unit tests, we
> >> can
> >> > probably get away with tying the value of start_commit_before_draw to
> >> > SingleThreadProxy once it has a Scheduler. We'll probably need to fork
> >> the
> >> > scheduler's unit test though.
> >>
> >
> >
> > https://codereview.chromium.org/23907006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-03-03 16:18:11]

lgtm

[danakj, 2014-03-03 16:39:28]

On Fri, Feb 28, 2014 at 8:58 PM, <brianderson@chromium.org> wrote:

> On 2014/02/28 22:40:52, danakj wrote:
>
>  On Fri, Feb 28, 2014 at 5:30 PM, <mailto:brianderson@chromium.org> wrote:
>>
>
>  > On 2014/02/28 21:52:57, danakj wrote:
>> >
>> > https://codereview.chromium.org/23907006/diff/1211001/ui/
>> > compositor/compositor.cc
>> >
>> >> File ui/compositor/compositor.cc (right):
>> >>
>> >
>> >
>> > https://codereview.chromium.org/23907006/diff/1211001/ui/
>> > compositor/compositor.cc#newcode213
>> >
>> >> ui/compositor/compositor.cc:213: settings.start_commit_before_
>> draw_enabled
>> >> =
>> >> On 2014/02/28 21:36:28, brianderson wrote:
>> >> > On 2014/02/28 21:19:12, danakj wrote:
>> >> > > Did you collect any data for the browser compositor that suggests
>> >> they are
>> >>
>> > a
>> >
>> >> > > good idea there too? The two environments are quite different
>> >>
>> > (commit/paint
>> >
>> >> is
>> >> > > much faster and controlled in the browser).
>> >> >
>> >> > I have not. Theoretically, this wont make a difference if the Browser
>> >> compositor
>> >> > is fast but will allow more concurrency if it is slow.
>> >>
>> >
>> >  I'm worried that if we commit sooner we miss renderer frames or user
>> input
>> >>
>> > more
>> >
>> >> often and increase latency by a frame.
>> >>
>> >
>> > We will not miss user input, especially assuming it is aligned with the
>> > BeginImplFrame. Even if it's not aligned, drift between the input and
>> draw
>> > will
>> > not affect average latency over a long period of time.
>> >
>> > It will affect how the Browser consumes Renderer frames, but it could
>> > actually
>> > improve latency/throughput if it allows the swap ack to return to the
>> > Renderer
>> > sooner. Also, we usually wont start a commit on the Browser unless a
>> > Renderer
>> > has already submitted a frame, so we could only possibly miss a Renderer
>> > frame
>> > when there are multiple Renderers submitting frames. Once the "losing"
>> > Renderer
>> > misses a frame though, it enters a high latency mode and will be much
>> less
>> > likely to miss subsequent frames.
>>
>
>
>  Is this true when there is a loading spinner animating in the browser
>> causing continuous commits, or when a user is moving a window around
>> causing commits? (That's the user input I meant not input on a webpage
>> btw.)
>>
>
>  Sorry if I'm being annoying here. We're doing a lot of work to reduce
>> single frames of latency, so I'd like to make sure we understand what
>> we're
>> doing to the browser compositor here.
>>
>
> Not annoying. I'm glad you got me to think about how this would work with
> the
> SingleThreadProxy as a SchedulerClient. We definitely don't want to start
> the
> BeginMainFrame before the draw in that case, since we won't get the
> concurrency
> benefits.
>
> For ThreadProxy, the spinner / window move cases will put any active
> Renderers
> into a high latency mode today already. Starting the commit earlier will
> send
> the swap ack back to the Renderer earlier though. To really avoid Renderers
> entering a high latency mode in these cases, we need commitless updates so
> the
> browser can commit and start rasterization and then give the renderers up
> until
> a deadline to submit their frames.
>

Oh, this is a cool point about commitless updates. I guess this is what you
said on the bug too, I don't think I quite put it together re: deadlines
like this in my head. Thanks.


>
> Since we are headed towards SingleThreadProxy as a SchedulerClient though,
> I
> think we should keep the old behavior around. I'll just need to fork some
> of the
> Scheduler tests. To improve the swap ack latency, maybe we can send the
> swap
> acks before a draw if we know that we are going to commit immediately
> after the
> draw, before any Renderers can submit another frame on the message loop.


I guess we just need to fork behaviour between ui/renderer until we get
commitless update then? We can mark said tests as such I guess to remind us
about this.


>
>
>
>
>    > This also won't make as much sense once we're using
>> >> SingleThreadProxy+Scheduler
>> >> > in the Browser because this mainly gives us the benefit of drawing
>> and
>> >> > committing concurrently when there is an Impl thread and Main thread.
>> >>
>> >
>> >  This also depends on a pending tree being present then?
>> >>
>> >
>> > I keep using the terms BeginMainFrame and commit interchangeably when I
>> > shouldn't. The commit is only the end of the BeginMainFrame. I should
>> have
>> > said
>> > we get concurrency between the BeginMainFrame and the draw, which
>> doesn't
>> > necessarily require a pending tree being present.
>> >
>> >
>> >
>> >  > With SingleThreadProxy, prioritizing the commit will defer the draw
>> and
>> >> will
>> >> > only give us a concurrency benefit if we can overlap the draw with
>> >> > impl-side-painting.
>> >>
>> >
>> >  With single-thread impl-side-painting we're expecting to commit
>> directly
>> >> to
>> >>
>> > the
>> >
>> >> active tree, so committing before draw would clobber the active tree.
>> >>
>> >
>> > See previous comment. We could get concurrency without clobbering the
>> > active
>> >
>> > tree.
>>
>
> You are correct. I was wrong here regarding the SingleThreadProxy case. The
> commit would get clobbered because the BeginMainFrame implies commit
> before the
> draw.
>
>
>  >
>> >
>> >  > Sadly, I was hoping we could get away with just testing the
>> >> > start_commit_before_draw path. For the LayerTreeHostImpl unit tests,
>> we
>> >> can
>> >> > probably get away with tying the value of start_commit_before_draw to
>> >> > SingleThreadProxy once it has a Scheduler. We'll probably need to
>> fork
>> >> the
>> >> > scheduler's unit test though.
>> >>
>> >
>> >
>> > https://codereview.chromium.org/23907006/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/23907006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2014-03-05 00:30:47]

Dana, ptal.

Luckily, I only had to fork two of the scheduler tests:
TestFailedDrawsEventuallyForceDrawAfterNextCommit
TestsetNeedsCommitIsNotLost

The rest of the tests are resilient not to break depending on the value of
start_commit_before_draw_enabled.

Since so many tests didn't depend on the value of
start_commit_before_draw_enabled, I also added a scheduler test called
StartCommitBeforeDrawDisabled.

https://codereview.chromium.org/23907006/diff/1211001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/23907006/diff/1211001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:773: if (did_swap && commit_state_ ==
COMMIT_STATE_WAITING_FOR_FIRST_DRAW)
This broke the old code path. I should not have added the did_swap restriction
here. Yay for unit tests!

https://codereview.chromium.org/23907006/diff/1231001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/23907006/diff/1231001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:183:
TEST(SchedulerStateMachineTest, StartCommitBeforeDrawDisabled) {
I went ahead and added this test to make sure we explicitly test *not* starting
the commit before the draw.

https://codereview.chromium.org/23907006/diff/1231001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/23907006/diff/1231001/ui/compositor/composito...
ui/compositor/compositor.cc:213: settings.start_commit_before_draw_enabled =
false;
Went ahead and disabled this since we'll be moving to SingleThreadProxy anyway.

[danakj, 2014-03-05 19:52:42]

LGTM

https://codereview.chromium.org/23907006/diff/1231001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/23907006/diff/1231001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:182: // This is enabled by
default.
Can you reword this, I'm not sure if it's saying
start_commit_before_draw_enabled is "enabled" by default or is equal to false by
default. This will get out of date anyways really when I think about it..

https://codereview.chromium.org/23907006/diff/1231001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:791: void
TestsetNeedsCommitIsNotLost(bool start_commit_before_draw_enabled) {
s/TestsetNeeds/TestSetNeeds/g

Looks like that s got missed in our chromifying.

[brianderson, 2014-03-07 21:30:11]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-03-07 21:31:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23907006/1311001

[commit-bot: I haz the power, 2014-03-08 10:59:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23907006/1311001

[commit-bot: I haz the power, 2014-03-08 12:40:59]

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

[commit-bot: I haz the power, 2014-03-08 12:41:00]

Try jobs failed on following builders: linux_chromium_rel

[brianderson, 2014-03-10 19:15:04]

Depends on https://codereview.chromium.org/190543012 to fix a race condition in
some of the unit tests.

[brianderson, 2014-03-11 15:37:21]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-03-11 15:37:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23907006/1351001

[commit-bot: I haz the power, 2014-03-11 15:38:38]

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

[commit-bot: I haz the power, 2014-03-11 15:38:39]

Failed to apply patch for cc/base/switches.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/base/switches.h
  Hunk #1 FAILED at 23.
  Hunk #2 FAILED at 71.
  2 out of 2 hunks FAILED -- saving rejects to file cc/base/switches.h.rej

Patch:       cc/base/switches.h
Index: cc/base/switches.h
diff --git a/cc/base/switches.h b/cc/base/switches.h
index
cedb0413e366971740e26ab698afaab8c97b400b..c95f2999f0af566d4fe5e9ad8cd5b8eec0af10c7
100644
--- a/cc/base/switches.h
+++ b/cc/base/switches.h
@@ -23,6 +23,10 @@ CC_EXPORT extern const char kDisableThreadedAnimation[];
 CC_EXPORT extern const char kDisableCompositedAntialiasing[];
 CC_EXPORT extern const char kEnableLCDText[];
 CC_EXPORT extern const char kEnableImplSidePainting[];
+CC_EXPORT extern const char kDisableStartCommitBeforeDraw[];
+CC_EXPORT extern const char kEnableStartCommitBeforeDraw[];
+CC_EXPORT extern const char kDisableStartCommitBeforeActivate[];
+CC_EXPORT extern const char kEnableStartCommitBeforeActivate[];
 CC_EXPORT extern const char kEnableTopControlsPositionCalculation[];
 CC_EXPORT extern const char kEnableGPURasterization[];
 CC_EXPORT extern const char kJankInsteadOfCheckerboard[];
@@ -71,6 +75,8 @@ CC_EXPORT extern const char kCCRebaselinePixeltests[];
 
 CC_EXPORT bool IsLCDTextEnabled();
 CC_EXPORT bool IsGpuRasterizationEnabled();
+CC_EXPORT bool IsStartCommitBeforeDrawEnabled();
+CC_EXPORT bool IsStartCommitBeforeActivateEnabled();
 CC_EXPORT bool IsImplSidePaintingEnabled();
 CC_EXPORT bool IsMapImageEnabled();

[brianderson, 2014-03-11 21:29:43]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2014-03-11 21:38:28]

The CQ bit was unchecked by brianderson@chromium.org

[brianderson, 2014-03-11 21:54:40]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-03-11 22:32:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23907006/1381001

[commit-bot: I haz the power, 2014-03-12 06:34:21]

Change committed as 256454

[brianderson, 2014-03-12 18:54:54]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-03-12 18:55:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23907006/1421001

[commit-bot: I haz the power, 2014-03-12 19:32:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/23907006/1421001

[commit-bot: I haz the power, 2014-03-12 21:58:29]

Change committed as 256669