gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory (ShaderCacheFactory::InitInstance()). Also, make the
  factory a ThreadChecker, and DCHECK that it is being used from a single
  thread.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.

This removes the last dependency on the rest of the content code from
the gpu shader cache code.

BUG=643746
Committed: https://crrev.com/0bb0ba87fa9ea8441575b798b91309986aee400d
Cr-Commit-Position: refs/heads/master@{#429549}

[sadrul, 2016-11-01 18:59:27]

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

[commit-bot: I haz the power, 2016-11-01 18:59:56]

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

[sadrul, 2016-11-01 19:01:29]

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

[commit-bot: I haz the power, 2016-11-01 19:02:10]

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-11-01 19:25:14]

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

[commit-bot: I haz the power, 2016-11-01 19:25:15]

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

[sadrul, 2016-11-01 19:58:04]

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

[commit-bot: I haz the power, 2016-11-01 19:58:24]

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-11-01 22:03:05]

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

[commit-bot: I haz the power, 2016-11-01 22:03:06]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-11-01 23:08:35]

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

[commit-bot: I haz the power, 2016-11-01 23:09: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, 2016-11-01 23:32:52]

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

[commit-bot: I haz the power, 2016-11-01 23:32:52]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[sadrul, 2016-11-02 00:41:36]

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

[commit-bot: I haz the power, 2016-11-02 00:42:04]

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

[sadrul, 2016-11-02 01:23:25]

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

[commit-bot: I haz the power, 2016-11-02 01:23:26]

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-11-02 02:04:22]

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

[commit-bot: I haz the power, 2016-11-02 02:04:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2016-11-02 02:46:44]

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

[commit-bot: I haz the power, 2016-11-02 02:47:04]

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-11-02 03:39:22]

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

[commit-bot: I haz the power, 2016-11-02 03:39:23]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-11-02 03:46:15]

Description was changed from

==========
gpu shader: ...

BUG=...
==========

to

==========
gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects. This allows removing the somewhat weird
  |entry_map_| from ShaderDiskCache, and replace that with a vector of
  unique_ptr<>s.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.


BUG=643746
==========

[sadrul, 2016-11-02 03:47:48]

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

[commit-bot: I haz the power, 2016-11-02 03:47:56]

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

[sadrul, 2016-11-02 03:50:46]

Description was changed from

==========
gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects. This allows removing the somewhat weird
  |entry_map_| from ShaderDiskCache, and replace that with a vector of
  unique_ptr<>s.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.


BUG=643746
==========

to

==========
gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory (ShaderCacheFactory::InitInstance()). Also, make the
  factory a ThreadChecker, and DCHECK that it is being used from a single
  thread.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects. This allows removing the somewhat weird
  |entry_map_| from ShaderDiskCache, and replace that with a vector of
  unique_ptr<>s.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.


BUG=643746
==========

[sadrul, 2016-11-02 03:52:55]

Description was changed from

==========
gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory (ShaderCacheFactory::InitInstance()). Also, make the
  factory a ThreadChecker, and DCHECK that it is being used from a single
  thread.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects. This allows removing the somewhat weird
  |entry_map_| from ShaderDiskCache, and replace that with a vector of
  unique_ptr<>s.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.


BUG=643746
==========

to

==========
gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory (ShaderCacheFactory::InitInstance()). Also, make the
  factory a ThreadChecker, and DCHECK that it is being used from a single
  thread.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects. This allows removing the somewhat weird
  |entry_map_| from ShaderDiskCache, and replace that with a vector of
  unique_ptr<>s.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.

This removes the last dependency on the rest of the content code from
the gpu shader cache code.

BUG=643746
==========

[sadrul, 2016-11-02 03:53:12]

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

[commit-bot: I haz the power, 2016-11-02 04:44:41]

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

[commit-bot: I haz the power, 2016-11-02 04:44:42]

Dry run: This issue passed the CQ dry run.

[piman, 2016-11-02 18:11:15]

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
File content/browser/gpu/shader_disk_cache.cc (right):

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
content/browser/gpu/shader_disk_cache.cc:183: return net::ERR_IO_PENDING;
I'm not a huge fan of this transformation, because it makes the code less
readable IMO. net::ERR_IO_PENDING is an implementation detail of the loop in
OnOpComplete, but looking here, it's not obvious what's pending (actually,
nothing is).

In particular if we want to handle error cases differently than success, we
would need this rv to correctly flow to OnOpComplete, I think.

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
content/browser/gpu/shader_disk_cache.cc:394:
base::LeakySingletonTraits<ShaderCacheFactory>>::get();
Can we instantiate the singleton in CreateFactoryInstance, and instead here
DCHECK(factory_instance)?
This is to ensure no-one tries to call ShaderCacheFactory::GetInstance before it
gets initialized.

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
content/browser/gpu/shader_disk_cache.cc:586: }));
This changes from O(log n) to O(n) - is it really ok? I think we can have a lot
of entries.

Could we use a std::set (std::unordered_set?) instead of std::vector to keep the
O(log n) (or O(1)) behavior?

[sadrul, 2016-11-02 19:20:25]

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

[sadrul, 2016-11-02 19:20:41]

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

[commit-bot: I haz the power, 2016-11-02 19:20:58]

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

[sadrul, 2016-11-02 19:24:32]

Description was changed from

==========
gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory (ShaderCacheFactory::InitInstance()). Also, make the
  factory a ThreadChecker, and DCHECK that it is being used from a single
  thread.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects. This allows removing the somewhat weird
  |entry_map_| from ShaderDiskCache, and replace that with a vector of
  unique_ptr<>s.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.

This removes the last dependency on the rest of the content code from
the gpu shader cache code.

BUG=643746
==========

to

==========
gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory (ShaderCacheFactory::InitInstance()). Also, make the
  factory a ThreadChecker, and DCHECK that it is being used from a single
  thread.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.

This removes the last dependency on the rest of the content code from
the gpu shader cache code.

BUG=643746
==========

[sadrul, 2016-11-02 19:24:53]

Updated the CL desc. to reflect the changes in the patchset.

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
File content/browser/gpu/shader_disk_cache.cc (right):

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
content/browser/gpu/shader_disk_cache.cc:183: return net::ERR_IO_PENDING;
On 2016/11/02 18:11:15, piman wrote:
> I'm not a huge fan of this transformation, because it makes the code less
> readable IMO. net::ERR_IO_PENDING is an implementation detail of the loop in
> OnOpComplete, but looking here, it's not obvious what's pending (actually,
> nothing is).
> 
> In particular if we want to handle error cases differently than success, we
> would need this rv to correctly flow to OnOpComplete, I think.

I have reverted this part of the code. Instead of this, OnOpComplete() holds one
a WeakPtr<> and inspects that to determine if it has been destroyed.

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
content/browser/gpu/shader_disk_cache.cc:394:
base::LeakySingletonTraits<ShaderCacheFactory>>::get();
On 2016/11/02 18:11:15, piman wrote:
> Can we instantiate the singleton in CreateFactoryInstance, and instead here
> DCHECK(factory_instance)?
> This is to ensure no-one tries to call ShaderCacheFactory::GetInstance before
it
> gets initialized.

Done.

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
content/browser/gpu/shader_disk_cache.cc:586: }));
On 2016/11/02 18:11:15, piman wrote:
> This changes from O(log n) to O(n) - is it really ok? I think we can have a
lot
> of entries.
> 
> Could we use a std::set (std::unordered_set?) instead of std::vector to keep
the
> O(log n) (or O(1)) behavior?

I couldn't find a clean way of using std::[unordered_]set. So I went back to
using std::unordered_map<>.

[commit-bot: I haz the power, 2016-11-02 19:46:16]

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

[commit-bot: I haz the power, 2016-11-02 19:46:17]

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

[sadrul, 2016-11-02 20:36:40]

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
File content/browser/gpu/shader_disk_cache.cc (right):

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
content/browser/gpu/shader_disk_cache.cc:394:
base::LeakySingletonTraits<ShaderCacheFactory>>::get();
On 2016/11/02 19:24:53, sadrul wrote:
> On 2016/11/02 18:11:15, piman wrote:
> > Can we instantiate the singleton in CreateFactoryInstance, and instead here
> > DCHECK(factory_instance)?
> > This is to ensure no-one tries to call ShaderCacheFactory::GetInstance
before
> it
> > gets initialized.
> 
> Done.

This change ended up catching a few places that calls GetInstance() directly
without initializing it first. But a number of those tests don't actually do
anything with the cache/factory. So would it make sense to have GetInstance()
return null if it hasn't been initialized, instead of being fatal? Or maybe have
a separate 'bool HasInstance()'?

The atlernative is to create the ShaderCacheFactory in
content::UnitTestTestSuite so that it's always there. The problem with this is
though that there are no task-runner when UnitTestTestSuite runs. So
ShaderCacheFactory will have to deal with null task-runners.

[piman, 2016-11-02 20:56:17]

LGTM with option below.

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
File content/browser/gpu/shader_disk_cache.cc (right):

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
content/browser/gpu/shader_disk_cache.cc:394:
base::LeakySingletonTraits<ShaderCacheFactory>>::get();
On 2016/11/02 20:36:39, sadrul wrote:
> On 2016/11/02 19:24:53, sadrul wrote:
> > On 2016/11/02 18:11:15, piman wrote:
> > > Can we instantiate the singleton in CreateFactoryInstance, and instead
here
> > > DCHECK(factory_instance)?
> > > This is to ensure no-one tries to call ShaderCacheFactory::GetInstance
> before
> > it
> > > gets initialized.
> > 
> > Done.
> 
> This change ended up catching a few places that calls GetInstance() directly
> without initializing it first. But a number of those tests don't actually do
> anything with the cache/factory. So would it make sense to have GetInstance()
> return null if it hasn't been initialized, instead of being fatal? Or maybe
have
> a separate 'bool HasInstance()'?
> 
> The atlernative is to create the ShaderCacheFactory in
> content::UnitTestTestSuite so that it's always there. The problem with this is
> though that there are no task-runner when UnitTestTestSuite runs. So
> ShaderCacheFactory will have to deal with null task-runners.


I'm ok with returning null from GetInstance() if it hasn't been initialized.
Maybe add a comment that it's the case?

[sadrul, 2016-11-03 02:15:37]

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

[commit-bot: I haz the power, 2016-11-03 02:15:58]

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-11-03 02:26:59]

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

[commit-bot: I haz the power, 2016-11-03 02:27:00]

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

[sadrul, 2016-11-03 02:43:35]

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

[commit-bot: I haz the power, 2016-11-03 02:43:57]

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

[sadrul, 2016-11-03 03:57:51]

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
File content/browser/gpu/shader_disk_cache.cc (right):

https://codereview.chromium.org/2472473002/diff/130001/content/browser/gpu/sh...
content/browser/gpu/shader_disk_cache.cc:394:
base::LeakySingletonTraits<ShaderCacheFactory>>::get();
On 2016/11/02 20:56:17, piman wrote:
> On 2016/11/02 20:36:39, sadrul wrote:
> > On 2016/11/02 19:24:53, sadrul wrote:
> > > On 2016/11/02 18:11:15, piman wrote:
> > > > Can we instantiate the singleton in CreateFactoryInstance, and instead
> here
> > > > DCHECK(factory_instance)?
> > > > This is to ensure no-one tries to call ShaderCacheFactory::GetInstance
> > before
> > > it
> > > > gets initialized.
> > > 
> > > Done.
> > 
> > This change ended up catching a few places that calls GetInstance() directly
> > without initializing it first. But a number of those tests don't actually do
> > anything with the cache/factory. So would it make sense to have
GetInstance()
> > return null if it hasn't been initialized, instead of being fatal? Or maybe
> have
> > a separate 'bool HasInstance()'?
> > 
> > The atlernative is to create the ShaderCacheFactory in
> > content::UnitTestTestSuite so that it's always there. The problem with this
is
> > though that there are no task-runner when UnitTestTestSuite runs. So
> > ShaderCacheFactory will have to deal with null task-runners.
> 
> 
> I'm ok with returning null from GetInstance() if it hasn't been initialized.
> Maybe add a comment that it's the case?

Done.

[sadrul, 2016-11-03 03:58:58]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-11-03 03:58:58]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2472473002/#ps190001
(title: "iwyu")

[commit-bot: I haz the power, 2016-11-03 03:59:02]

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-11-03 04:44:48]

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

[commit-bot: I haz the power, 2016-11-03 04:44:49]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[sadrul, 2016-11-03 04:49:22]

The CQ bit was checked by sadrul@chromium.org

[commit-bot: I haz the power, 2016-11-03 04:49:39]

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-11-03 06:50:14]

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

[commit-bot: I haz the power, 2016-11-03 06:50:16]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[sadrul, 2016-11-03 12:34:46]

The CQ bit was checked by sadrul@chromium.org

[commit-bot: I haz the power, 2016-11-03 12:34:58]

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-11-03 12:52:08]

Description was changed from

==========
gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory (ShaderCacheFactory::InitInstance()). Also, make the
  factory a ThreadChecker, and DCHECK that it is being used from a single
  thread.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.

This removes the last dependency on the rest of the content code from
the gpu shader cache code.

BUG=643746
==========

to

==========
gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory (ShaderCacheFactory::InitInstance()). Also, make the
  factory a ThreadChecker, and DCHECK that it is being used from a single
  thread.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.

This removes the last dependency on the rest of the content code from
the gpu shader cache code.

BUG=643746
==========

[commit-bot: I haz the power, 2016-11-03 12:52:12]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2016-11-03 12:54:21]

Description was changed from

==========
gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory (ShaderCacheFactory::InitInstance()). Also, make the
  factory a ThreadChecker, and DCHECK that it is being used from a single
  thread.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.

This removes the last dependency on the rest of the content code from
the gpu shader cache code.

BUG=643746
==========

to

==========
gpu shader cache: Clarify lifetime/ownership/threadiness of objects.

Notable changes:
. Instead of using BrowserThread from ShaderCacheFactory, inject the
  relevant task runners. This requires having an explicit creation step
  for the factory (ShaderCacheFactory::InitInstance()). Also, make the
  factory a ThreadChecker, and DCHECK that it is being used from a single
  thread.

. Make ShaderDiskReadHelper non-refcounted. Instead, make ShaderDiskCache
  its explicit owner. This is safe, since ShaderDiskReadHelper used to
  have a weak pointer to the ShaderDiskCache, and not perform any action
  if the cache had been destroyed. So it makes sense to simply destroy
  the helper with the cache.

. Make ShaderDiskCacheEntry non-refcounted. Like read-helper, this too
  used to have a weak pointer to the ShaderDiskCache, and not perform any
  action if the cache had been destroyed. So have the ShaderDiskCache own
  the cache-entry objects.

. DCHECK that ShaderDiskReadHelper and ShaderDiskCacheEntry objects are
  destroyed in the same thread as they are created in. This also
  simplifies the dtors.

. ShaderDiskCache no longer needs to be a SupportsWeakPtr<>, since only
  the cache-entry and the read-helper objects used to have the weak
  pointers, and they no longer do.

This removes the last dependency on the rest of the content code from
the gpu shader cache code.

BUG=643746

Committed: https://crrev.com/0bb0ba87fa9ea8441575b798b91309986aee400d
Cr-Commit-Position: refs/heads/master@{#429549}
==========

[commit-bot: I haz the power, 2016-11-03 12:54:22]

Patchset 11 (id:??) landed as
https://crrev.com/0bb0ba87fa9ea8441575b798b91309986aee400d
Cr-Commit-Position: refs/heads/master@{#429549}