Clear shader disk cache after glProgramBinary failure.

Clear shader disk cache after glProgramBinary failure.

Currently, when the GPU process crashes trying to load a GL program from
our disk cache, we take no special action. This means we will very
likely try to load this same cache entry again, causing another crash.

This CL adds an "activity flags" object which is written by the GPU
process and read by the GPU process host. This allows us to detect
crashes while loading shaders and clear the cache in these instances.

R=vmiura
BUG=699122, 658784
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2744363002
Cr-Commit-Position: refs/heads/master@{#457869}
Committed: https://chromium.googlesource.com/chromium/src/+/8c0b7bf620cb0ff2d74470b17800281e65405cdd

[ericrk, 2017-03-13 19:06:33]

Victor, how does something like this look to you (for handling the GPU proc
crash during cached shader load)?

[vmiura, 2017-03-13 20:18:36]

This looks good.  Was there more you had in mind to add?

https://codereview.chromium.org/2744363002/diff/1/gpu/ipc/common/activity_fla...
File gpu/ipc/common/activity_flags.cc (right):

https://codereview.chromium.org/2744363002/diff/1/gpu/ipc/common/activity_fla...
gpu/ipc/common/activity_flags.cc:66: DCHECK_NE(old_value, new_value);
This doesn't really check anything since x ^ flag always toggles the flag.

If you want to check for double-unset, check the old_value has 'flag' set.

[ericrk, 2017-03-13 20:30:27]

Patchset #2 (id:20001) has been deleted

[ericrk, 2017-03-13 20:30:45]

https://codereview.chromium.org/2744363002/diff/1/gpu/ipc/common/activity_fla...
File gpu/ipc/common/activity_flags.cc (right):

https://codereview.chromium.org/2744363002/diff/1/gpu/ipc/common/activity_fla...
gpu/ipc/common/activity_flags.cc:66: DCHECK_NE(old_value, new_value);
On 2017/03/13 20:18:36, vmiura wrote:
> This doesn't really check anything since x ^ flag always toggles the flag.
> 
> If you want to check for double-unset, check the old_value has 'flag' set.

Yup, good point.

[ericrk, 2017-03-13 20:31:17]

On 2017/03/13 20:18:36, vmiura wrote:
> This looks good.  Was there more you had in mind to add?

Nope, just wanted to run this by you before sending more broadly for owners.

[vmiura, 2017-03-13 20:53:27]

Cool.  LGTM.

[ericrk, 2017-03-13 22:49:10]

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

[jbauman, 2017-03-15 00:29:05]

lgtm

[ericrk, 2017-03-15 00:47:54]

ericrk@chromium.org changed reviewers:
+ fsamuel@chromium.org, tsepez@chromium.org

[ericrk, 2017-03-15 00:47:54]

+tsepez for IPC owners
+fsamuel for services/ui owners

https://codereview.chromium.org/2744363002/diff/60001/services/ui/ws/gpu_host.cc
File services/ui/ws/gpu_host.cc (right):

https://codereview.chromium.org/2744363002/diff/60001/services/ui/ws/gpu_host...
services/ui/ws/gpu_host.cc:116: preferences, mojo::ScopedSharedBufferHandle());
This is just a no-op for now. Later, if this class wants to listen to any of the
GPU activity flags, it can create a GpuProcessHostActivityFlags and clone its
handle here.

Flags are currently only used with relation to the shader disk cache, which this
class currently no-ops.

[ericrk, 2017-03-15 00:48:07]

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

[commit-bot: I haz the power, 2017-03-15 00:49:08]

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, 2017-03-15 00:53:47]

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

[commit-bot: I haz the power, 2017-03-15 00:53:48]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Tom Sepez, 2017-03-15 17:37:12]

mojom LGTM

[ericrk, 2017-03-15 18:15:39]

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

[commit-bot: I haz the power, 2017-03-15 18:16:02]

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, 2017-03-15 18:20:09]

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

[commit-bot: I haz the power, 2017-03-15 18:20:10]

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

[ericrk, 2017-03-15 18:27:03]

Patchset #4 (id:80001) has been deleted

[ericrk, 2017-03-15 20:24:44]

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

[commit-bot: I haz the power, 2017-03-15 20:25:17]

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

[Fady Samuel, 2017-03-15 20:28:04]

Sadrul wrote more of this code than I did. Adding him and moving myself to cc.

[Fady Samuel, 2017-03-15 20:28:53]

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

[Fady Samuel, 2017-03-15 20:28:54]

Oops, adding sadrul@ for real.

[ericrk, 2017-03-15 21:03:07]

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

[ericrk, 2017-03-15 21:03:07]

+sky for the new dependency on mojo/public/cpp/system/buffer.h (from
gpu/command_buffer/common/activity_flags.h)

Let me know if this dependency makes sense, it seemed like the shared memory
class that I should be using (to send over a mojo IPC).

[commit-bot: I haz the power, 2017-03-15 21:10:57]

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

[commit-bot: I haz the power, 2017-03-15 21:10:58]

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

[sky, 2017-03-15 23:14:05]

LGTM

[sadrul, 2017-03-16 04:40:32]

https://codereview.chromium.org/2744363002/diff/100001/gpu/command_buffer/com...
File gpu/command_buffer/common/activity_flags.h (right):

https://codereview.chromium.org/2744363002/diff/100001/gpu/command_buffer/com...
gpu/command_buffer/common/activity_flags.h:50: static GpuProcessActivityFlags*
GetInstance();
Please consider not making it a singleton.

Looks like the only place where this is currently used is MemoryProgramCache.
Instead of using the global singleton, it could maybe get it during construction
from GpuChannelManager?

https://codereview.chromium.org/2744363002/diff/100001/services/ui/gpu/gpu_ma...
File services/ui/gpu/gpu_main.cc (right):

https://codereview.chromium.org/2744363002/diff/100001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:124:
gpu::GpuProcessActivityFlags::InitializeInstance(std::move(activity_flags));
Note that GpuChannelManager is not created in the current thread. Instead, it
gets created by GpuService, in GpuMain::CreateGpuServiceOnGpuThread(). Should
this be initialized in that thread too?

Maybe it'd be simplest to move this GpuProcessActivityFlags initialization into
GpuService::Bind(), or GpuService::InitializeWithHost()? (since GpuService is
used by both this GpuMain, and content::GpuChildThread)

https://codereview.chromium.org/2744363002/diff/100001/services/ui/gpu/interf...
File services/ui/gpu/interfaces/gpu_main.mojom (right):

https://codereview.chromium.org/2744363002/diff/100001/services/ui/gpu/interf...
services/ui/gpu/interfaces/gpu_main.mojom:22: handle<shared_buffer>
activity_flags);
The new param needs to be optional (the reason for the crashes in the trybots),
i.e.:

  handle<shared_buffer>? activity_flags

[ericrk, 2017-03-16 21:06:59]

Patchset #5 (id:120001) has been deleted

[ericrk, 2017-03-16 21:17:22]

Patchset #6 (id:160001) has been deleted

[ericrk, 2017-03-16 21:21:26]

Patchset #6 (id:180001) has been deleted

[ericrk, 2017-03-16 21:25:23]

Sadrul@, thanks for the feedback! I've removed the singleton and tried to clean
up init a bit. Please let me know how this looks.

https://codereview.chromium.org/2744363002/diff/100001/gpu/command_buffer/com...
File gpu/command_buffer/common/activity_flags.h (right):

https://codereview.chromium.org/2744363002/diff/100001/gpu/command_buffer/com...
gpu/command_buffer/common/activity_flags.h:50: static GpuProcessActivityFlags*
GetInstance();
On 2017/03/16 04:40:32, sadrul wrote:
> Please consider not making it a singleton.
> 
> Looks like the only place where this is currently used is MemoryProgramCache.
> Instead of using the global singleton, it could maybe get it during
construction
> from GpuChannelManager?

Sure, made this not a singleton.

https://codereview.chromium.org/2744363002/diff/100001/services/ui/gpu/gpu_ma...
File services/ui/gpu/gpu_main.cc (right):

https://codereview.chromium.org/2744363002/diff/100001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:124:
gpu::GpuProcessActivityFlags::InitializeInstance(std::move(activity_flags));
On 2017/03/16 04:40:32, sadrul wrote:
> Note that GpuChannelManager is not created in the current thread. Instead, it
> gets created by GpuService, in GpuMain::CreateGpuServiceOnGpuThread(). Should
> this be initialized in that thread too?
> 
> Maybe it'd be simplest to move this GpuProcessActivityFlags initialization
into
> GpuService::Bind(), or GpuService::InitializeWithHost()? (since GpuService is
> used by both this GpuMain, and content::GpuChildThread)

Seems like it shouldn't matter where this is initialized, it's not doing
anything in init other than moving the ScopedSharedBufferHandle and mapping it.

I've re-factored things so we don't use a singleton any more, but instead pass
the GpuProcessActivityFlags through to the GpuChannelManager.

I still create the GpuProcessActivityFlags here, as it seems more
self-documenting to pass that object around rather than a
ScopedSharedBufferHandle... WDYT?

https://codereview.chromium.org/2744363002/diff/100001/services/ui/gpu/interf...
File services/ui/gpu/interfaces/gpu_main.mojom (right):

https://codereview.chromium.org/2744363002/diff/100001/services/ui/gpu/interf...
services/ui/gpu/interfaces/gpu_main.mojom:22: handle<shared_buffer>
activity_flags);
On 2017/03/16 04:40:32, sadrul wrote:
> The new param needs to be optional (the reason for the crashes in the
trybots),
> i.e.:
> 
>   handle<shared_buffer>? activity_flags

Ah, thanks!

[ericrk, 2017-03-16 21:46:19]

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

[commit-bot: I haz the power, 2017-03-16 21:47:11]

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, 2017-03-16 23:20:56]

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

[commit-bot: I haz the power, 2017-03-16 23:20:57]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-03-16 23:26:08]

Thank you! lgtm

[ericrk, 2017-03-17 19:58:16]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2017-03-17 19:58:17]

The patchset sent to the CQ was uploaded after l-g-t-m from vmiura@chromium.org,
tsepez@chromium.org, jbauman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2744363002/#ps200001
(title: "Remove singleton and clean up init")

[commit-bot: I haz the power, 2017-03-17 19:58:51]

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, 2017-03-17 20:07:28]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489780696763200,
"parent_rev": "6cfe941e235849f210b3d1290f1d4d5e7a45f438", "commit_rev":
"8c0b7bf620cb0ff2d74470b17800281e65405cdd"}

[commit-bot: I haz the power, 2017-03-17 20:08:13]

Description was changed from

==========
Clear shader disk cache after glProgramBinary failure.

Currently, when the GPU process crashes trying to load a GL program from
our disk cache, we take no special action. This means we will very
likely try to load this same cache entry again, causing another crash.

This CL adds an "activity flags" object which is written by the GPU
process and read by the GPU process host. This allows us to detect
crashes while loading shaders and clear the cache in these instances.

R=vmiura
BUG=699122, 658784
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Clear shader disk cache after glProgramBinary failure.

Currently, when the GPU process crashes trying to load a GL program from
our disk cache, we take no special action. This means we will very
likely try to load this same cache entry again, causing another crash.

This CL adds an "activity flags" object which is written by the GPU
process and read by the GPU process host. This allows us to detect
crashes while loading shaders and clear the cache in these instances.

R=vmiura
BUG=699122, 658784
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2744363002
Cr-Commit-Position: refs/heads/master@{#457869}
Committed:
https://chromium.googlesource.com/chromium/src/+/8c0b7bf620cb0ff2d74470b17800...
==========

[commit-bot: I haz the power, 2017-03-17 20:08:14]

Committed patchset #6 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/8c0b7bf620cb0ff2d74470b17800...