Update Context Client Visibility to use Scoped Pattern

Update Context Client Visibility to use Scoped Pattern

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, SetClientVisible is now only accessible through the
ScopedContextClientVisibility object, which ensures that visible/
not-visible calls are paired.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

[ericrk, 2016-08-17 21:10:46]

Description was changed from

==========
Update Context Client Visibility to use Scoped Pattern

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, SetClientVisible is now only accessible through the
ScopedContextClientVisibility object, which ensures that visible/
not-visible calls are paired.
==========

to

==========
Update Context Client Visibility to use Scoped Pattern

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, SetClientVisible is now only accessible through the
ScopedContextClientVisibility object, which ensures that visible/
not-visible calls are paired.
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-17 21:10:47]

Description was changed from

==========
Update Context Client Visibility to use Scoped Pattern

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, SetClientVisible is now only accessible through the
ScopedContextClientVisibility object, which ensures that visible/
not-visible calls are paired.
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

==========
Update Context Client Visibility to use Scoped Pattern

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, SetClientVisible is now only accessible through the
ScopedContextClientVisibility object, which ensures that visible/
not-visible calls are paired.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[ericrk, 2016-08-17 21:26:03]

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

[ericrk, 2016-08-17 21:26:04]

Here's an idea for using a scoped pattern to ensure correctness. It seems to
work (caught a few things), but let me know if you can think of a less verbose
way to achieve this :P

[ericrk, 2016-08-18 18:15:33]

Ok, cleaned this up a bit more. Let me know how this looks. Thanks!

[danakj, 2016-08-18 21:39:44]

https://codereview.chromium.org/2252163003/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:151: if (is_visible && *scoped_visibility)
These 2 can be combined as is_visible == *scoped_visiblity?

https://codereview.chromium.org/2252163003/diff/80001/cc/output/gl_renderer_u...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/output/gl_renderer_u...
cc/output/gl_renderer_unittest.cc:2161: EXPECT_CALL(*context_support_ptr_,
ClientBecameNotVisible_(::testing::_))
just _, we using'd it at the top

https://codereview.chromium.org/2252163003/diff/80001/cc/test/test_context_su...
File cc/test/test_context_support.cc (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/test/test_context_su...
cc/test/test_context_support.cc:101: return
std::unique_ptr<gpu::ScopedVisibilityImpl>(
I think you can just write your own impl here, I don't like deping on impls from
other places. It can just be local to this .cc file. That or move the class out
of ContextSupport and just make it a direct thing no subclassing.

also return base::MakeUnique<T>(..);

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:159: // Calls SetClientVisible on the provided
|context_provider| and handles
SetClientVisible is a lie

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:167: if (*scoped_visibility && is_visible)
Combine?

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:194: if (*scoped_visibility && is_visible)
Combine?

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2359: if (main_context_client_visibility_) {
They already early out appropriately, can you mention why you're checking them
here too? (Cuz of Bind failing?)

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:844:
std::unique_ptr<gpu::ContextSupport::ScopedVisibility>
can you put these right below the output_surface_?

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:486: ClientBecameNotVisible_(::testing::_))
just _, add a call to using if needed

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

https://codereview.chromium.org/2252163003/diff/80001/gpu/command_buffer/clie...
gpu/command_buffer/client/context_support.h:20: class
ScopedContextClientVisibility;
remove?

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

https://codereview.chromium.org/2252163003/diff/80001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5877:
static_cast<ScopedVisibilityImpl*>(visibility.get())->Release(this);
I think maybe just define this class in this .cc file unless you're going to
make it a first class part of the API.

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

https://codereview.chromium.org/2252163003/diff/80001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.h:836: uint32_t visible_clients_;
num_visible_clients_?

[ericrk, 2016-08-19 00:12:52]

Patchset #6 (id:100001) has been deleted

[ericrk, 2016-08-19 00:23:50]

Patchset #6 (id:120001) has been deleted

[ericrk, 2016-08-19 17:23:17]

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

[ericrk, 2016-08-19 17:23:19]

Decided to keep clearing the compositor context in LTHI, 128k isn't too much,
but enough that I'd like to keep freeing it. If we reduce the minimum
allocations in a follow up patch, I'll remove the cleanup to simplify things.

https://codereview.chromium.org/2252163003/diff/80001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:151: if (is_visible && *scoped_visibility)
On 2016/08/18 21:39:44, danakj wrote:
> These 2 can be combined as is_visible == *scoped_visiblity?

good point

https://codereview.chromium.org/2252163003/diff/80001/cc/output/gl_renderer_u...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/output/gl_renderer_u...
cc/output/gl_renderer_unittest.cc:2161: EXPECT_CALL(*context_support_ptr_,
ClientBecameNotVisible_(::testing::_))
On 2016/08/18 21:39:44, danakj wrote:
> just _, we using'd it at the top

Done.

https://codereview.chromium.org/2252163003/diff/80001/cc/test/test_context_su...
File cc/test/test_context_support.cc (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/test/test_context_su...
cc/test/test_context_support.cc:101: return
std::unique_ptr<gpu::ScopedVisibilityImpl>(
On 2016/08/18 21:39:44, danakj wrote:
> I think you can just write your own impl here, I don't like deping on impls
from
> other places. It can just be local to this .cc file. That or move the class
out
> of ContextSupport and just make it a direct thing no subclassing.
> 
> also return base::MakeUnique<T>(..);

makes sense, moved to cc file

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:159: // Calls SetClientVisible on the provided
|context_provider| and handles
On 2016/08/18 21:39:44, danakj wrote:
> SetClientVisible is a lie

Comment wasn't really saying much... removed.

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:167: if (*scoped_visibility && is_visible)
On 2016/08/18 21:39:44, danakj wrote:
> Combine?

Done.

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:194: if (*scoped_visibility && is_visible)
On 2016/08/18 21:39:44, danakj wrote:
> Combine?

Done.

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2359: if (main_context_client_visibility_) {
On 2016/08/18 21:39:44, danakj wrote:
> They already early out appropriately, can you mention why you're checking them
> here too? (Cuz of Bind failing?)

Oh yeah... no reason I guess.

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:844:
std::unique_ptr<gpu::ContextSupport::ScopedVisibility>
On 2016/08/18 21:39:44, danakj wrote:
> can you put these right below the output_surface_?

Done.

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2252163003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:486: ClientBecameNotVisible_(::testing::_))
On 2016/08/18 21:39:44, danakj wrote:
> just _, add a call to using if needed

Done.

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

https://codereview.chromium.org/2252163003/diff/80001/gpu/command_buffer/clie...
gpu/command_buffer/client/context_support.h:20: class
ScopedContextClientVisibility;
On 2016/08/18 21:39:44, danakj wrote:
> remove?

Done.

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

https://codereview.chromium.org/2252163003/diff/80001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5877:
static_cast<ScopedVisibilityImpl*>(visibility.get())->Release(this);
On 2016/08/18 21:39:44, danakj wrote:
> I think maybe just define this class in this .cc file unless you're going to
> make it a first class part of the API.

Done.

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

https://codereview.chromium.org/2252163003/diff/80001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.h:836: uint32_t visible_clients_;
On 2016/08/18 21:39:44, danakj wrote:
> num_visible_clients_?

Done.

[danakj, 2016-08-19 18:29:55]

LGTM

https://codereview.chromium.org/2252163003/diff/160001/cc/test/test_context_s...
File cc/test/test_context_support.cc (right):

https://codereview.chromium.org/2252163003/diff/160001/cc/test/test_context_s...
cc/test/test_context_support.cc:31: // |initial_context_support_| is just
stored/used for sanity-checking Release.
Comment not quite true now

https://codereview.chromium.org/2252163003/diff/160001/cc/test/test_context_s...
cc/test/test_context_support.cc:39: visible_clients_(0),
can you do this in the header? (mayeb move out_of_order_callbacks there too?)

https://codereview.chromium.org/2252163003/diff/160001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/2252163003/diff/160001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.cc:67: //
|initial_context_support_| is just stored/used for sanity-checking Release.
Update comment

https://codereview.chromium.org/2252163003/diff/160001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.cc:5888:
DCHECK_GT(num_visible_clients_, 0u);
can you DCHECK(visibility) too? (same for the TestContextSupport I guess also)

[ericrk, 2016-08-19 18:49:58]

https://codereview.chromium.org/2252163003/diff/160001/cc/test/test_context_s...
File cc/test/test_context_support.cc (right):

https://codereview.chromium.org/2252163003/diff/160001/cc/test/test_context_s...
cc/test/test_context_support.cc:31: // |initial_context_support_| is just
stored/used for sanity-checking Release.
On 2016/08/19 18:29:55, danakj wrote:
> Comment not quite true now

Done.

https://codereview.chromium.org/2252163003/diff/160001/cc/test/test_context_s...
cc/test/test_context_support.cc:39: visible_clients_(0),
On 2016/08/19 18:29:55, danakj wrote:
> can you do this in the header? (mayeb move out_of_order_callbacks there too?)

Done.

https://codereview.chromium.org/2252163003/diff/160001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/2252163003/diff/160001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.cc:67: //
|initial_context_support_| is just stored/used for sanity-checking Release.
On 2016/08/19 18:29:55, danakj wrote:
> Update comment

Done.

https://codereview.chromium.org/2252163003/diff/160001/gpu/command_buffer/cli...
gpu/command_buffer/client/gles2_implementation.cc:5888:
DCHECK_GT(num_visible_clients_, 0u);
On 2016/08/19 18:29:55, danakj wrote:
> can you DCHECK(visibility) too? (same for the TestContextSupport I guess also)

Done.

[ericrk, 2016-08-19 18:50:02]

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

[commit-bot: I haz the power, 2016-08-19 18:50:40]

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

[ericrk, 2016-08-19 19:52:15]

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

[commit-bot: I haz the power, 2016-08-19 19:53:03]

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:13:47]

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

[commit-bot: I haz the power, 2016-08-19 22:13:48]

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

[ericrk, 2016-08-23 22:24:45]

Patchset #10 (id:220001) has been deleted

[ericrk, 2016-08-23 22:29:41]

Made some changes here to remove SetAggressivelyFreeResources and handle this
internally in GLES2Implementation based on visibility. Some more cleanup is
possible in GLES2Implementation, but we first need to update all context
consumers to use ScopedVisibility.

[ericrk, 2016-08-23 22:53:42]

On 2016/08/23 22:29:41, ericrk wrote:
> Made some changes here to remove SetAggressivelyFreeResources and handle this
> internally in GLES2Implementation based on visibility. Some more cleanup is
> possible in GLES2Implementation, but we first need to update all context
> consumers to use ScopedVisibility.

nvm.. this doesn't quite work - going back to the old version for now.

[ericrk, 2016-08-23 22:53:54]

Patchset #11 (id:260001) has been deleted

[ericrk, 2016-08-23 22:54:01]

Patchset #10 (id:240001) has been deleted

[ericrk, 2016-08-24 16:35:45]

Patchset #10 (id:280001) has been deleted

[ericrk, 2016-08-24 16:41:24]

Patchset #10 (id:300001) has been deleted

[ericrk, 2016-08-24 16:43:15]

Ok, this has been refactored to simplify the CC calling patterns and the
ContextSupport interface. PTAL, thanks!

[danakj, 2016-08-24 20:12:49]

LGTM thanks for cleaning up that API

https://codereview.chromium.org/2252163003/diff/320001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_context_provider.cc (right):

https://codereview.chromium.org/2252163003/diff/320001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_context_provider.cc:99:
ContextSupport()->TrimResources();
Did you mean to put this is DeleteCachedResources?

[ericrk, 2016-08-24 21:58:51]

ericrk@chromium.org changed reviewers:
+ boliu@chromium.org, wez@chromium.org

[ericrk, 2016-08-24 21:58:52]

Adding additional reviewers for small tweaks to the various ContextProviders:
+wez for blimp/
+boliu for android_webview/

https://codereview.chromium.org/2252163003/diff/320001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_context_provider.cc (right):

https://codereview.chromium.org/2252163003/diff/320001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_context_provider.cc:99:
ContextSupport()->TrimResources();
On 2016/08/24 20:12:49, danakj wrote:
> Did you mean to put this is DeleteCachedResources?

I absolutely did

[ericrk, 2016-08-24 21:59:56]

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

[commit-bot: I haz the power, 2016-08-24 22:01:02]

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

[boliu, 2016-08-24 22:15:40]

lgtm

[commit-bot: I haz the power, 2016-08-24 22:34:38]

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

[commit-bot: I haz the power, 2016-08-24 22:34:39]

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

[Wez, 2016-08-24 22:38:51]

blimp/ LGTM

[ericrk, 2016-08-25 17:30:24]

Bo, I made an additional change to synchronous_compositor_output_surface.cc if
you'd like to review - forgot that we called SetAggressivelyFreeResources there
as well.

New code is a bit simpler and should provide the same cleanup. I considered
adding visibility tracking to the webview code, but given that it uses the
synchronous compositor, it seems unlikely that additional work will come in
after we set memory limit to 0, so this seemed like overkill... WDYT?

[boliu, 2016-08-25 19:40:53]

On 2016/08/25 17:30:24, ericrk wrote:
> Bo, I made an additional change to synchronous_compositor_output_surface.cc if
> you'd like to review - forgot that we called SetAggressivelyFreeResources
there
> as well.
> 
> New code is a bit simpler and should provide the same cleanup. I considered
> adding visibility tracking to the webview code, but given that it uses the
> synchronous compositor, it seems unlikely that additional work will come in
> after we set memory limit to 0, so this seemed like overkill... WDYT?

seems fine in my head too :)

lgtm

[ericrk, 2016-08-25 20:51:09]

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

[commit-bot: I haz the power, 2016-08-25 20:52: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-08-25 21:54:22]

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

[commit-bot: I haz the power, 2016-08-25 21:54:23]

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