Free worker context resources on idle.

Free worker context resources on idle.

Allows an idle callback to be set on the worker context, and gives
clients of the context a way to notify the context that it is busy or
idle.

RenderThreadImpl now sets a callback which will clean up Skia and
command buffer resources when the context becomes idle for more than
one second.

BUG=624630
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

[ericrk, 2016-08-19 00:21:31]

Description was changed from

==========
Free worker context resources on idle.

Allows an idle callback to be set on the worker context, and gives
clients of the context a way to notify the context that it is busy or
idle.

RenderThreadImpl now sets a callback which will clean up Skia and
command buffer resources when the context becomes idle for more than
one second.

BUG=624630
==========

to

==========
Free worker context resources on idle.

Allows an idle callback to be set on the worker context, and gives
clients of the context a way to notify the context that it is busy or
idle.

RenderThreadImpl now sets a callback which will clean up Skia and
command buffer resources when the context becomes idle for more than
one second.

BUG=624630
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ericrk, 2016-08-19 00:21:31]

Description was changed from

==========
Free worker context resources on idle.

Allows an idle callback to be set on the worker context, and gives
clients of the context a way to notify the context that it is busy or
idle.

RenderThreadImpl now sets a callback which will clean up Skia and
command buffer resources when the context becomes idle for more than
one second.

BUG=624630
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Free worker context resources on idle.

Allows an idle callback to be set on the worker context, and gives
clients of the context a way to notify the context that it is busy or
idle.

RenderThreadImpl now sets a callback which will clean up Skia and
command buffer resources when the context becomes idle for more than
one second.

BUG=624630
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[ericrk, 2016-08-19 20:46:25]

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

[ericrk, 2016-08-19 20:46:28]

ericrk@chromium.org changed reviewers:
+ danakj@chromium.org

[ericrk, 2016-08-19 20:46:28]

Here's the "idle" counterpart to the previous "visble" cl. A bit more complex as
we need to delay the callback until we've been idle a certain amount of time.

[commit-bot: I haz the power, 2016-08-19 20:46:51]

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-08-19 22:24:45]

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

[commit-bot: I haz the power, 2016-08-19 22:24:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2016-08-23 01:26:25]

https://codereview.chromium.org/2257533006/diff/90001/cc/output/context_provi...
File cc/output/context_provider.cc (right):

https://codereview.chromium.org/2257533006/diff/90001/cc/output/context_provi...
cc/output/context_provider.cc:15: context_provider_->DetachFromThread();
You don't need to do this before ClientBecameBusy?

https://codereview.chromium.org/2257533006/diff/90001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2257533006/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:456:
context_provider->ContextSupport()->TrimResources();
Why do we need a separate TrimResources from DeleteCachedResources?

https://codereview.chromium.org/2257533006/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:2151: // is alive, so we know that the
unreatained worker context provider
unretained

https://codereview.chromium.org/2257533006/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:2156:
base::ThreadTaskRunnerHandle::Get());
what about just making it use the task runner the context is bound to? thats how
the callbacks on ContextProvider work

https://codereview.chromium.org/2257533006/diff/90001/gpu/command_buffer/clie...
File gpu/command_buffer/client/context_support.h (right):

https://codereview.chromium.org/2257533006/diff/90001/gpu/command_buffer/clie...
gpu/command_buffer/client/context_support.h:42: // Similar to temporarily
calling SetAggressivelyFreeResources(true), this
We now have 3 functions we can call to free things, one with a toggle. How can
we consolidate? I can't actually explain this anymore :/

https://codereview.chromium.org/2257533006/diff/90001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/2257533006/diff/90001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5926: // We can only modify
|pending_idle_callback_| from the provided callback
Its kinda awk if you end up running the cancel right after the callback because
of posting order.

What if you used a generation id like "scheduled idle 5" and "cancelled idle 5"
now you don't run 5. but if scheduled idle != cancelled idle then you let the
posted cleanup run? or something like that?

https://codereview.chromium.org/2257533006/diff/90001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/2257533006/diff/90001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.h:837:
std::unique_ptr<base::CancelableClosure> pending_idle_callback_;
why unique_ptr it?

[ericrk, 2016-08-24 18:32:21]

Thanks for the feedback! A few questions

https://codereview.chromium.org/2257533006/diff/90001/cc/output/context_provi...
File cc/output/context_provider.cc (right):

https://codereview.chromium.org/2257533006/diff/90001/cc/output/context_provi...
cc/output/context_provider.cc:15: context_provider_->DetachFromThread();
On 2016/08/23 01:26:24, danakj wrote:
> You don't need to do this before ClientBecameBusy?

The detach/attach is only necessary before access to the GL context itself. For
helper functions like the busy handler, this shouldn't matter (as long as we
have the lock acquired) -- but I'm happy to move it after if that would keep
things more understandable - WDYT?

https://codereview.chromium.org/2257533006/diff/90001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2257533006/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:2156:
base::ThreadTaskRunnerHandle::Get());
On 2016/08/23 01:26:24, danakj wrote:
> what about just making it use the task runner the context is bound to? thats
how
> the callbacks on ContextProvider work

The current callback systems always funnels through the Context/GpuControl.
Depending on the implementation, the Context/GpuControl may not have a task
runner (and may run callbacks directly), although typically it does have one.

It seems like we could:
- expose the task runner on GpuControl, and allow it to return null.
- Add a "ScheduleTask" and "ScheduleDelayedTask" to GpuControl, but at that
point we might as well just return the task runner?
- Move the idle callback to GpuControl/Context, although this would require some
new logic in each implementation of GpuControl/Context (similar to our other
callbacks I guess?)

https://codereview.chromium.org/2257533006/diff/90001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/2257533006/diff/90001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5926: // We can only modify
|pending_idle_callback_| from the provided callback
On 2016/08/23 01:26:25, danakj wrote:
> Its kinda awk if you end up running the cancel right after the callback
because
> of posting order.
> 
> What if you used a generation id like "scheduled idle 5" and "cancelled idle
5"
> now you don't run 5. but if scheduled idle != cancelled idle then you let the
> posted cleanup run? or something like that?

It's actually a little bit tricky to avoid races like this. The problem has to
do with safely accessing the state related to the callback. 

Assuming this is a locking context, at the point when we call
ClientBecameBusy/ClientBecameNotBusy, we are locked by the caller. However, at
the point when the delayed task is run, we are no longer locked. This means we
can't safely access any mutable state that might be in use by another thread.

There are two ways to work around this:
 1) Ensure that any mutable state which affects the callback is *only* modified
from a single thread. This is what we currently do - the callable callback is
the "mutable state", and it is only scheduled/canceled/run on the callback
thread.
 2) Take an additional lock before modifying any callback related mutable state.

As mentioned, the code I initially wrote uses option (1). We could move to a
callback generation id, but modification of this ID would have to happen with
the same restrictions, using either (1) or (2).

If we use a generation id with option (1), we still have the same race where a
cancel can happen right after the callback is run, as we have to post generation
updates to the callback thread.

If we use a generation id with option (2), we need a new lock which is held
around modifications of the generation id, and this is still a bit racy.

The only way I can think of to minimize this race without doing (1) or (2) is to
add an additional "ShouldRunIdleCallback(uint32_t idle_callback_id)" on
ContextSupport. The external callback code could then call this after acquiring
the context lock, and early out if it returns false. This would ensure that
generation id was only checked / modified while the context lock is held.
However, this is pretty roundabout and puts additional checking burden on the
callback.

WDYT?

https://codereview.chromium.org/2257533006/diff/90001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/2257533006/diff/90001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.h:837:
std::unique_ptr<base::CancelableClosure> pending_idle_callback_;
On 2016/08/23 01:26:25, danakj wrote:
> why unique_ptr it?

You must create a CancelableClosure on the same thread you call cancel on, so
this lets us delay creation until we are on the idle_callback_task_runner_.

This can be re-worked if we go with a generation_id_, see my comments below.

[ericrk, 2016-08-24 22:57:32]

Patchset #8 (id:130001) has been deleted

[ericrk, 2016-08-24 23:02:37]

Patchset #7 (id:110001) has been deleted