Aura: Allow browser UI thread waiting for compositor commits.

Aura: Allow browser UI thread waiting for compositor commits.

This is necessary for --ui-enable-threaded-compositing. In principle, we will not be blocking the browser UI any more than we would without --ui-enable-threaded-compositing.

BUG=155048
TEST=by hand


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161962

[jonathan.backer, 2012-10-10 21:19:08]

There's one FIXME in thread_restrictions.h. ScopedAllowWait is meant to be a
temporary measure. Obviously that's not true with
--ui-enable-threaded-compositing.

[piman, 2012-10-10 21:28:03]

On 2012/10/10 21:19:08, jonathan.backer wrote:
> There's one FIXME in thread_restrictions.h. ScopedAllowWait is meant to be a
> temporary measure. Obviously that's not true with
> --ui-enable-threaded-compositing.

LGTM, though I'm not an owner in either directory.

[jamesr, 2012-10-11 00:06:50]

lgtm for cc

[tfarina, 2012-10-11 00:13:22]

http://codereview.chromium.org/11085054/diff/1/base/threading/thread_restrict...
File base/threading/thread_restrictions.h (right):

http://codereview.chromium.org/11085054/diff/1/base/threading/thread_restrict...
base/threading/thread_restrictions.h:184: friend class cc::CCCompletionEvent;   
         // http://crbug.com/FIXME
did you really mean crbug.com/FIXME?

[jonathan.backer, 2012-10-11 15:12:20]

+jam for base/threading/thread_restrictions.h

@jam: Just a little background. This is so that we can put the aura browser
compositor on it's own thread (behind --ui-enable-threaded-compositing). With
this flag, the UI is manipulated on the browser UI thread, information is
periodically proxied to the compositor thread, and drawing occurs on the
compositor thread. It's during the proxy of information that we block the UI
thread (there are other instances, but this is the common case). This is short
in duration and guaranteed completion.

There's very little risk here: we use --enable-threaded-compositing in the
renderer (we use the same CC code in the browser and renderer), so we're very
careful to avoid deadlocks.

[jam, 2012-10-11 15:33:51]

On 2012/10/11 15:12:20, jonathan.backer wrote:
> +jam for base/threading/thread_restrictions.h
> 
> @jam: Just a little background. This is so that we can put the aura browser
> compositor on it's own thread (behind --ui-enable-threaded-compositing).

I think the fact that it's behind a flag is a bit of a red herring since I
assume this will be the default at a later time?

> With
> this flag, the UI is manipulated on the browser UI thread, information is
> periodically proxied to the compositor thread, and drawing occurs on the
> compositor thread. It's during the proxy of information that we block the UI
> thread (there are other instances, but this is the common case). This is short
> in duration and guaranteed completion.
> 
> There's very little risk here: we use --enable-threaded-compositing in the
> renderer (we use the same CC code in the browser and renderer), so we're very
> careful to avoid deadlocks.

The point of this assert is to catch jank, so I'm not worried about deadlocks.

I'm not familiar with the compositor code. Can you explain to me why waiting on
another thread is the only way to do this? i.e. why can't this data be posted in
a task?

[jonathan.backer, 2012-10-11 16:21:36]

> I think the fact that it's behind a flag is a bit of a red herring since I
> assume this will be the default at a later time?

Absolutely. I'm aiming for M24 but that may be ambitious. I was more showing how
the code is made active --- on CrOS we typically enable new features behind a
flag by passing them into Chrome on startup via a script
/sbin/session_manager_setup.sh (similar to default on via chrome:flags but less
user visible)

> The point of this assert is to catch jank, so I'm not worried about deadlocks.

This should not increase UI jank. When we are waiting, the tasks that we're
waiting for the compositor thread to execute (mostly draw calls and texture
uploads) were typically executed on the UI thread prior to
--ui-enable-threaded-compositing. In the common case, it will typically reduce
jank by offloading work from the UI to a separate thread.
 
> I'm not familiar with the compositor code. Can you explain to me why waiting
on
> another thread is the only way to do this? i.e. why can't this data be posted
in
> a task?

Certainly this isn't the only way to do this. I'll let jamesr@ (or piman@) chime
in as to why it was done this way. I think that there are definite advantages to
the current model (simplicity of snapshotting --- incremental aggregation for
free --- and flexibility of compositor scheduling). I would point out, that the
compositor thread is handling the display --- which is typically what we want to
be least janky and that we typically block the UI thread once per frame for <
1ms (even on low power devices like daisy).

[jam, 2012-10-11 16:45:48]

On 2012/10/11 16:21:36, jonathan.backer wrote:
> > The point of this assert is to catch jank, so I'm not worried about
deadlocks.
> 
> This should not increase UI jank. When we are waiting, the tasks that we're
> waiting for the compositor thread to execute (mostly draw calls and texture
> uploads) were typically executed on the UI thread prior to
> --ui-enable-threaded-compositing. In the common case, it will typically reduce
> jank by offloading work from the UI to a separate thread.

But do we know that the compositor thread is free right when we try to
synchronize them? i.e. if it's off doing something else, this will just block
the UI thread.

Another thing is that if the threads are not blocked on each other, then the UI
thread can continue to do other work while the compositor thread frees up to
handle the task. the current situation is that these tasks just pile up and not
run, even if they have nothing to do with UI. there will then be a backlog once
the UI thread stops waiting, and that'll cause jank.

>
>  
> > I'm not familiar with the compositor code. Can you explain to me why waiting
> on
> > another thread is the only way to do this? i.e. why can't this data be
posted
> in
> > a task?
> 
> Certainly this isn't the only way to do this. I'll let jamesr@ (or piman@)
chime
> in as to why it was done this way. I think that there are definite advantages
to
> the current model (simplicity of snapshotting --- incremental aggregation for
> free --- and flexibility of compositor scheduling). I would point out, that
the
> compositor thread is handling the display --- which is typically what we want
to
> be least janky and that we typically block the UI thread once per frame for <
> 1ms (even on low power devices like daisy).

see my 2 points above: namely that reducing parallelism will make things janky,
and also the uncertainty of what the compositor thread is doing which may lead
to delays

[jamesr, 2012-10-11 18:31:14]

On 2012/10/11 16:45:48, John Abd-El-Malek wrote:
> On 2012/10/11 16:21:36, jonathan.backer wrote:
> > > The point of this assert is to catch jank, so I'm not worried about
> deadlocks.
> > 
> > This should not increase UI jank. When we are waiting, the tasks that we're
> > waiting for the compositor thread to execute (mostly draw calls and texture
> > uploads) were typically executed on the UI thread prior to
> > --ui-enable-threaded-compositing. In the common case, it will typically
reduce
> > jank by offloading work from the UI to a separate thread.
> 
> But do we know that the compositor thread is free right when we try to
> synchronize them? i.e. if it's off doing something else, this will just block
> the UI thread.

One thing to keep in mind is that in this mode it's the compositor thread that
actually renders frames that the user can see, not the UI thread.  When thinking
about UI jank due to being able to draw it's the compositor thread that is
critical, not the UI thread.

> 
> Another thing is that if the threads are not blocked on each other, then the
UI
> thread can continue to do other work while the compositor thread frees up to
> handle the task. the current situation is that these tasks just pile up and
not
> run, even if they have nothing to do with UI. there will then be a backlog
once
> the UI thread stops waiting, and that'll cause jank.

This is a valid concern.  The compositor thread is designed to not block unless
the graphics pipeline is deeply backlogged, which means the user can't see any
updates no matter what threads are doing what.

> 
> >
> >  
> > > I'm not familiar with the compositor code. Can you explain to me why
waiting
> > on
> > > another thread is the only way to do this? i.e. why can't this data be
> posted
> > in
> > > a task?
> > 
> > Certainly this isn't the only way to do this. I'll let jamesr@ (or piman@)
> chime
> > in as to why it was done this way. I think that there are definite
advantages
> to
> > the current model (simplicity of snapshotting --- incremental aggregation
for
> > free --- and flexibility of compositor scheduling). I would point out, that
> the
> > compositor thread is handling the display --- which is typically what we
want
> to
> > be least janky and that we typically block the UI thread once per frame for
<
> > 1ms (even on low power devices like daisy).
> 
> see my 2 points above: namely that reducing parallelism will make things
janky,
> and also the uncertainty of what the compositor thread is doing which may lead
> to delays

[jonathan.backer, 2012-10-11 18:52:53]

On 2012/10/11 16:45:48, John Abd-El-Malek wrote:
> On 2012/10/11 16:21:36, jonathan.backer wrote:
> > > The point of this assert is to catch jank, so I'm not worried about
> deadlocks.
> > 
> > This should not increase UI jank. When we are waiting, the tasks that we're
> > waiting for the compositor thread to execute (mostly draw calls and texture
> > uploads) were typically executed on the UI thread prior to
> > --ui-enable-threaded-compositing. In the common case, it will typically
reduce
> > jank by offloading work from the UI to a separate thread.
> 
> But do we know that the compositor thread is free right when we try to
> synchronize them? i.e. if it's off doing something else, this will just block
> the UI thread.

Poking through some code to answer your questions. AFAIK, there isn't a hard
guarantee, but
(a) the synchronization phase (where UI waits) is initiated by the compositor
thread (the compositor thread tries to schedule when it is idle --- typically
right after a draw, via a post task, to give it as much time as possible to
upload textures for the next frame)
(b) the compositor thread is lightweight --- the heaviest task I've seen is a
series of texture uploads which is capped at 4 ms

> Another thing is that if the threads are not blocked on each other, then the
UI
> thread can continue to do other work while the compositor thread frees up to
> handle the task. the current situation is that these tasks just pile up and
not
> run, even if they have nothing to do with UI. there will then be a backlog
once
> the UI thread stops waiting, and that'll cause jank.

I understand your concern. Assuming that the compositor thread is idle (and it
almost always is --- again, no hard guarantees, just a best effort to schedule
tasks and make sure no single task takes more than 4ms), the UI thread is
waiting for the compositor thread to complete work that it needs done (texture
updates). If we can't upload fast enough (this rarely happens --- but I have
seen it occur), we want to throttle the UI back (slow the rate of UI update).
Waiting achieves this (this currently happens without
--ui-enable-threaded-compositing). The advantage of having the compositor thread
schedule the uploads is that it can trigger a draw (with checkerboards for yet
to be uploaded textures). This is particularly useful when animations are done
on the compositor thread (happens in the renderer --- TODO for the browser).
Then transform and opacity animations (which don't require new textures) can
proceed.

Again, no hard guarantee, but we prioritize the browser UI in the GPU process
(it pre-empts other clients) in an effort to keep the UI/compositor thread jank
free.

> 
> see my 2 points above: namely that reducing parallelism will make things
janky,
> and also the uncertainty of what the compositor thread is doing which may lead
> to delays

Your concerns are well founded on general principle. I can't speak for the
technical decision to block instead of post task on commit (the most common
block). But in practice, --ui-enable-threaded-compositing reduces UI jank over
the status quo by enabling some parallelism that is currently missing (drawing
on a separate thread). It also reduces user visible jank (to be improved once
animations move to compositor thread) by guaranteeing that we can draw a steady
60 fps.

[jam, 2012-10-12 06:16:36]

(missed the replies, sorry)

lgtm, thanks for explaining this to me. as you can tell, adding permanent
exclusions to that list is something that should be very rare.

[piman, 2012-10-12 17:46:56]

On Thu, Oct 11, 2012 at 11:52 AM, <backer@chromium.org> wrote:

> On 2012/10/11 16:45:48, John Abd-El-Malek wrote:
>
>> On 2012/10/11 16:21:36, jonathan.backer wrote:
>> > > The point of this assert is to catch jank, so I'm not worried about
>> deadlocks.
>> >
>> > This should not increase UI jank. When we are waiting, the tasks that
>> we're
>> > waiting for the compositor thread to execute (mostly draw calls and
>> texture
>> > uploads) were typically executed on the UI thread prior to
>> > --ui-enable-threaded-**compositing. In the common case, it will
>> typically
>>
> reduce
>
>> > jank by offloading work from the UI to a separate thread.
>>
>
>  But do we know that the compositor thread is free right when we try to
>> synchronize them? i.e. if it's off doing something else, this will just
>> block
>> the UI thread.
>>
>
> Poking through some code to answer your questions. AFAIK, there isn't a
> hard
> guarantee, but
> (a) the synchronization phase (where UI waits) is initiated by the
> compositor
> thread (the compositor thread tries to schedule when it is idle ---
> typically
> right after a draw, via a post task, to give it as much time as possible to
> upload textures for the next frame)
> (b) the compositor thread is lightweight --- the heaviest task I've seen
> is a
> series of texture uploads which is capped at 4 ms
>
>
>  Another thing is that if the threads are not blocked on each other, then
>> the
>>
> UI
>
>> thread can continue to do other work while the compositor thread frees up
>> to
>> handle the task. the current situation is that these tasks just pile up
>> and
>>
> not
>
>> run, even if they have nothing to do with UI. there will then be a backlog
>>
> once
>
>> the UI thread stops waiting, and that'll cause jank.
>>
>
> I understand your concern. Assuming that the compositor thread is idle
> (and it
> almost always is --- again, no hard guarantees, just a best effort to
> schedule
> tasks and make sure no single task takes more than 4ms), the UI thread is
> waiting for the compositor thread to complete work that it needs done
> (texture
> updates). If we can't upload fast enough (this rarely happens --- but I
> have
> seen it occur), we want to throttle the UI back (slow the rate of UI
> update).
> Waiting achieves this (this currently happens without
> --ui-enable-threaded-**compositing). The advantage of having the
> compositor thread
> schedule the uploads is that it can trigger a draw (with checkerboards for
> yet
> to be uploaded textures). This is particularly useful when animations are
> done
> on the compositor thread (happens in the renderer --- TODO for the
> browser).
> Then transform and opacity animations (which don't require new textures)
> can
> proceed.
>
> Again, no hard guarantee, but we prioritize the browser UI in the GPU
> process
> (it pre-empts other clients) in an effort to keep the UI/compositor thread
> jank
> free.
>
>
>
>  see my 2 points above: namely that reducing parallelism will make things
>>
> janky,
>
>> and also the uncertainty of what the compositor thread is doing which may
>> lead
>> to delays
>>
>
> Your concerns are well founded on general principle. I can't speak for the
> technical decision to block instead of post task on commit (the most common
> block).


James can talk about the details, but the idea is that the shared state
(the entire layer tree) is very large. To be able to "just post a task",
you essentially have to either:
- duplicate the entire state
- or serialize modifications to the state
The former is memory-heavy (state includes all pixels), the latter is less
heavy but still fairly intensive (modifications include all modified
pixels) and computationally expensive.
Both possibilities have been explored but the current solution has been
deemed better, with the idea that the compositor thread is more responsive
than the UI thread, i.e. it is no worse, the time where the UI thread is
blocked would be spent doing work instead anyway.

Antoine

> But in practice, --ui-enable-threaded-**compositing reduces UI jank over
> the status quo by enabling some parallelism that is currently missing
> (drawing
> on a separate thread). It also reduces user visible jank (to be improved
> once
> animations move to compositor thread) by guaranteeing that we can draw a
> steady
> 60 fps.
>
>
https://codereview.chromium.**org/11085054/<https://codereview.chromium.org/1...
>
> --
>
>
>

[commit-bot: I haz the power, 2012-10-12 18:12:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/11085054/13001

[commit-bot: I haz the power, 2012-10-13 04:55:55]

Retried try job too often for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, content_browsertests,
content_unittests, crypto_unittests, gpu_unittests, interactive_ui_tests,
ipc_tests, jingle_unittests, media_unittests, net_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests

[commit-bot: I haz the power, 2012-10-15 17:08:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/11085054/23001

[commit-bot: I haz the power, 2012-10-15 21:22:28]

Change committed as 161962