Initialize GPU shader disk cache on brower thread.

Initialize the GPU shader disk cache in the browser.

The disk cache allows us to load the compositor shaders on startup,
saving quite a bit of time on platforms that support caching.

BUG=510151
Committed: https://crrev.com/57b52d4786d8262c07e7eb5ba3a55cabf801ebff
Cr-Commit-Position: refs/heads/master@{#344816}

[Jamie Madill, 2015-08-05 17:52:31]

jmadill@chromium.org changed reviewers:
+ jam@chromium.org, kbr@chromium.org

[Jamie Madill, 2015-08-05 17:52:47]

John, Ken, this is my work-in-process patch, for reference during our chat.

[Ken Russell (switch to Gerrit), 2015-08-06 01:01:01]

kbr@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org, piman@chromium.org

[Ken Russell (switch to Gerrit), 2015-08-06 01:01:01]

+piman, danakj and enne; I don't have experience with the browser compositor and
its setup.

https://codereview.chromium.org/1274763003/diff/1/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/1274763003/diff/1/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_channel_host_factory.cc:157: user_data_dir);
Something seems suspicious about this. GpuChannels mostly map 1:1 to renderer
processes. This is going to be called once per each renderer process that
attempts to communicate with the GPU process, which seems wrong.
RenderProcessHostImpl::RenderProcessHostImpl already sets up the shader cache
for every spawned renderer.

As far as I understand the problem, the GpuChannels that the browser compositor
creates are the ones that don't have the shader disk cache set up properly. I
think we need to track down the code path that initializes those GpuChannels
specifically.

[piman, 2015-08-06 19:53:13]

https://codereview.chromium.org/1274763003/diff/1/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/1274763003/diff/1/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_channel_host_factory.cc:157: user_data_dir);
On 2015/08/06 01:01:01, Ken Russell wrote:
> Something seems suspicious about this. GpuChannels mostly map 1:1 to renderer
> processes. This is going to be called once per each renderer process that
> attempts to communicate with the GPU process, which seems wrong.
> RenderProcessHostImpl::RenderProcessHostImpl already sets up the shader cache
> for every spawned renderer.
> 
> As far as I understand the problem, the GpuChannels that the browser
compositor
> creates are the ones that don't have the shader disk cache set up properly. I
> think we need to track down the code path that initializes those GpuChannels
> specifically.

FYI, this code path is to setup the GpuChannelHost for the browser - it's the
equivalent of what RenderThreadImpl::EstablishGpuChannelSync does in the
renderer. gpu_client_id_ is the channel id for the browser.

This is going to be called once per GPU process restart though. It seems like an
odd place to put it. Maybe instead you could post a task to the IO thread from
the BrowserGpuChannelHostFactory constructor, only once?

[Jamie Madill, 2015-08-07 19:06:40]

PTAL - think this is mostly ready for review, just looking at making
a browser test now.

On 2015/08/06 19:53:13, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/1274763003/diff/1/content/browser/gpu/browser...
> File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):
> 
>
https://codereview.chromium.org/1274763003/diff/1/content/browser/gpu/browser...
> content/browser/gpu/browser_gpu_channel_host_factory.cc:157: user_data_dir);
> On 2015/08/06 01:01:01, Ken Russell wrote:
> > Something seems suspicious about this. GpuChannels mostly map 1:1 to
renderer
> > processes. This is going to be called once per each renderer process that
> > attempts to communicate with the GPU process, which seems wrong.
> > RenderProcessHostImpl::RenderProcessHostImpl already sets up the shader
cache
> > for every spawned renderer.
> > 
> > As far as I understand the problem, the GpuChannels that the browser
> compositor
> > creates are the ones that don't have the shader disk cache set up properly.
I
> > think we need to track down the code path that initializes those GpuChannels
> > specifically.
> 
> FYI, this code path is to setup the GpuChannelHost for the browser - it's the
> equivalent of what RenderThreadImpl::EstablishGpuChannelSync does in the
> renderer. gpu_client_id_ is the channel id for the browser.

Correct, this should only happen in the browser process, not the Renderers.

> This is going to be called once per GPU process restart though. It seems like
an
> odd place to put it. Maybe instead you could post a task to the IO thread from
> the BrowserGpuChannelHostFactory constructor, only once?

Done, seems reasonable.

[piman, 2015-08-07 19:33:59]

https://codereview.chromium.org/1274763003/diff/20001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/1274763003/diff/20001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:249:
base::Unretained(this)));
Unretained seems unsafe.

How about testing the command line and grabbing the directory here, and posting
the task only if we need to, passing the directory and the client id by value?
Then the function to set the cache info doesn't need |this| and can be static.

[Jamie Madill, 2015-08-07 20:07:58]

PTAL.

https://codereview.chromium.org/1274763003/diff/20001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/1274763003/diff/20001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:249:
base::Unretained(this)));
On 2015/08/07 19:33:59, piman (Very slow to review) wrote:
> Unretained seems unsafe.
> 
> How about testing the command line and grabbing the directory here, and
posting
> the task only if we need to, passing the directory and the client id by value?
> Then the function to set the cache info doesn't need |this| and can be static.

Done.

[piman, 2015-08-07 20:19:03]

lgtm

[jam, 2015-08-07 20:41:04]

https://codereview.chromium.org/1274763003/diff/40001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1274763003/diff/40001/content/public/browser/...
content/public/browser/content_browser_client.h:517: // Returns the path to the
browser shader disk cache root.
nit: please make chrome return a path that includes the shader directory (i.e.
not root = udd). i would like to avoid sending udd directly to content module to
avoid layering violations with content mucking with chrome's files

[Ken Russell (switch to Gerrit), 2015-08-07 22:47:41]

Realizing that I misunderstood the semantics of BrowserGpuChannelHostFactory in
the earlier patch, LGTM modulo John's comments.

[Jamie Madill, 2015-08-10 17:18:00]

PTAL. Still thinking about how to add a test, might need help on that.

https://codereview.chromium.org/1274763003/diff/40001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1274763003/diff/40001/content/public/browser/...
content/public/browser/content_browser_client.h:517: // Returns the path to the
browser shader disk cache root.
On 2015/08/07 20:41:04, jam wrote:
> nit: please make chrome return a path that includes the shader directory (i.e.
> not root = udd). i would like to avoid sending udd directly to content module
to
> avoid layering violations with content mucking with chrome's files

Done.

[jam, 2015-08-17 16:21:17]

lgtm sorry for the delay, i didn't see your reply

[Jamie Madill, 2015-08-21 12:09:42]

PTAL. After speaking to sievers@, Made a change so the Android content_unittests
wouldn't fail due to trying to initialize the GPU host without a content client.

[piman, 2015-08-21 17:37:46]

LGTM if sievers@ is ok with this.

[Ken Russell (switch to Gerrit), 2015-08-21 18:03:42]

LGTM again too FWIW.

[no sievers, 2015-08-21 18:08:23]

On 2015/08/21 17:37:46, piman (slow to review) wrote:
> LGTM if sievers@ is ok with this.

yes lgtm

[Jamie Madill, 2015-08-21 18:09:10]

Great, going to land this!

[Jamie Madill, 2015-08-21 18:09:15]

The CQ bit was checked by jmadill@chromium.org

[Jamie Madill, 2015-08-21 18:09:15]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1274763003/#ps80001
(title: "Address content_unittests failure on Android")

[commit-bot: I haz the power, 2015-08-21 18:09:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1274763003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1274763003/80001

[commit-bot: I haz the power, 2015-08-21 18:16:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-21 18:17:05]

Patchset 5 (id:??) landed as
https://crrev.com/57b52d4786d8262c07e7eb5ba3a55cabf801ebff
Cr-Commit-Position: refs/heads/master@{#344816}

[Stephen White, 2015-08-21 18:41:12]

w00t! \o/