Add the ability to clear the shader disk cache.

Add the ability to clear the shader disk cache.

When the browser cache for a given profile is cleared
we will now clear out any shaders stored in the
shader disk cache for that profile.

BUG=176289
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192511

[jonathan.backer, 2013-03-20 18:39:04]

Mostly just nits. I was concerned about a layering violation
content/public/browser/shader_disk_cache.cc:15

and multiple clears in flight
content/browser/gpu/shader_disk_cache_impl.h:128

https://codereview.chromium.org/12500009/diff/12001/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/12500009/diff/12001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.h:353: const base::Time
delete_end);
nit: formatting.

https://codereview.chromium.org/12500009/diff/12001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.h:356: void
DoClearShaderCache(int rv);
nix this.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache_impl.cc (left):

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:180: LOG(ERROR) << "Failed to
create shader cache entry: " << rv;
Still an error, no?

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache_impl.cc (right):

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:107: const std::string& shader)
Why reformat?

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:319: : op_type_(CREATE_CACHE) {
Why CREATE_CACHE? There hasn't been a clear request yet.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:418:
static_cast<ShaderCacheFactoryImpl*>(ShaderCacheFactory::GetInstance())->
Call the Impl getter and skip the cast?

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:423:
static_cast<ShaderCacheFactoryImpl*>(ShaderCacheFactory::GetInstance())->
Ditto.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:457: const std::string& shader) {
Why reformat?

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache_impl.h (left):

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:33: void set_host_id(int host_id) {
host_id_ = host_id; }
Why not inline?

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache_impl.h (right):

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:48: int SetAvailableCallback(const
net::CompletionCallback& callback);
s/availble/available

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:57: virtual ~ShaderDiskCache();
Why is this now virtual?

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:92: scoped_refptr<ShaderDiskCache>
GetByPath(const base::FilePath& path);
private?

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:122: // Used fo the cache Clear
call.
s/fo/for

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:128: scoped_refptr<ShaderDiskCache>
clear_shader_cache_;
What happens if you clear multiple times in rapid succession (say before a clear
completes)? What happens if you clear different profiles in rapid succession?

I suspect that you want some map of structs (or ClearHelpers if you're feeling
OO) of this stuff for pending clear ops.

https://codereview.chromium.org/12500009/diff/12001/content/public/browser/sh...
File content/public/browser/shader_disk_cache.cc (right):

https://codereview.chromium.org/12500009/diff/12001/content/public/browser/sh...
content/public/browser/shader_disk_cache.cc:15: }  // namespace content
Doesn't this violate layering (public pulling from non-public)?  I think that
you can just define this in shader_disk_cache_impl.cc. No?

[dsinclair, 2013-03-21 17:46:59]

PTAL.

https://codereview.chromium.org/12500009/diff/12001/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/12500009/diff/12001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.h:353: const base::Time
delete_end);
On 2013/03/20 18:39:05, jonathan.backer wrote:
> nit: formatting.

Done.

https://codereview.chromium.org/12500009/diff/12001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.h:356: void
DoClearShaderCache(int rv);
On 2013/03/20 18:39:05, jonathan.backer wrote:
> nix this.

Done.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache_impl.cc (left):

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:180: LOG(ERROR) << "Failed to
create shader cache entry: " << rv;
On 2013/03/20 18:39:05, jonathan.backer wrote:
> Still an error, no?

Done.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache_impl.cc (right):

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:107: const std::string& shader)
On 2013/03/20 18:39:05, jonathan.backer wrote:
> Why reformat?

Done. For a change which has since been reverted.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:319: : op_type_(CREATE_CACHE) {
On 2013/03/20 18:39:05, jonathan.backer wrote:
> Why CREATE_CACHE? There hasn't been a clear request yet.

Good point. Made it TERMINATE so nothing will happen.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:418:
static_cast<ShaderCacheFactoryImpl*>(ShaderCacheFactory::GetInstance())->
On 2013/03/20 18:39:05, jonathan.backer wrote:
> Call the Impl getter and skip the cast?

Done.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:423:
static_cast<ShaderCacheFactoryImpl*>(ShaderCacheFactory::GetInstance())->
On 2013/03/20 18:39:05, jonathan.backer wrote:
> Ditto.

Done.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:457: const std::string& shader) {
On 2013/03/20 18:39:05, jonathan.backer wrote:
> Why reformat?

Reverted change. Fixed.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache_impl.h (left):

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:33: void set_host_id(int host_id) {
host_id_ = host_id; }
On 2013/03/20 18:39:05, jonathan.backer wrote:
> Why not inline?

Will put back. Was removed because the compiler didn't like inlined when there
were virtual methods in here.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache_impl.h (right):

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:48: int SetAvailableCallback(const
net::CompletionCallback& callback);
On 2013/03/20 18:39:05, jonathan.backer wrote:
> s/availble/available

Done.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:57: virtual ~ShaderDiskCache();
On 2013/03/20 18:39:05, jonathan.backer wrote:
> Why is this now virtual?

Done. Did have an interface which is now removed.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:92: scoped_refptr<ShaderDiskCache>
GetByPath(const base::FilePath& path);
On 2013/03/20 18:39:05, jonathan.backer wrote:
> private?

Done.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:122: // Used fo the cache Clear
call.
On 2013/03/20 18:39:05, jonathan.backer wrote:
> s/fo/for

Done.

https://codereview.chromium.org/12500009/diff/12001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.h:128: scoped_refptr<ShaderDiskCache>
clear_shader_cache_;
On 2013/03/20 18:39:05, jonathan.backer wrote:
> What happens if you clear multiple times in rapid succession (say before a
clear
> completes)? What happens if you clear different profiles in rapid succession?
> 
> I suspect that you want some map of structs (or ClearHelpers if you're feeling
> OO) of this stuff for pending clear ops.

Done.

https://codereview.chromium.org/12500009/diff/12001/content/public/browser/sh...
File content/public/browser/shader_disk_cache.cc (right):

https://codereview.chromium.org/12500009/diff/12001/content/public/browser/sh...
content/public/browser/shader_disk_cache.cc:15: }  // namespace content
On 2013/03/20 18:39:05, jonathan.backer wrote:
> Doesn't this violate layering (public pulling from non-public)?  I think that
> you can just define this in shader_disk_cache_impl.cc. No?

Moved.

Component build still builds with this here but I haven't been able to run yet
as my tree seems to be broken.

[jonathan.backer, 2013-03-21 21:06:36]

This is looking good. Just nits.

https://codereview.chromium.org/12500009/diff/33001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache_impl.cc (right):

https://codereview.chromium.org/12500009/diff/33001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:105: : public base::ThreadChecker,
nix: ThreadChecker is for CalledOnValidThread, which you are not using.

https://codereview.chromium.org/12500009/diff/33001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:359:
ShaderClearHelper::~ShaderClearHelper() {
Does it matter what thread the |cache| is deleted on? If so, maybe verify that
you're on the IO thread here as well.

https://codereview.chromium.org/12500009/diff/33001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:387: path_));
nit: Comment for the PostTask. This won't be clear a few months down the road.
Perhaps an alternative is to take a scoped_ref on this. Then you know you won't
call the dtor.

[dsinclair, 2013-03-21 21:31:22]

jam@ can you please take a look for content OWNERS.

mkwst@ can you please take a look for chrome/browsing/browsing_data OWNERS.

[Mike West, 2013-03-21 21:57:41]

Thanks for adding this!

On 2013/03/21 21:31:22, dsinclair wrote:
> mkwst@ can you please take a look for chrome/browsing/browsing_data OWNERS.

One nit, two questions:

1. The commit message says "When the browser cache for a given profile is
cleared...". The code as written clears the shader cache only when "Cookies and
site data" is cleared. My (very) vague understanding of the shader cache is that
it's much more like transient resources cached temporarily than a cookie. If
that's the case it might make sense to add another bundled type (perhaps
`REMOVE_CACHED_DATA = REMOVE_CACHE | REMOVE_SHADER_CACHE`) rather than tie this
new type to `REMOVE_SITE_DATA`. I guess my question then is simply how
persistent the cached data actually is?

2. This patch doesn't include any tests. I'm not terribly familiar with the GPU
code; is there some mechanism you could use to verify that the disk cache was
cleared?

[jam, 2013-03-22 06:24:35]

I don't think you need to add a new interface to content\public\browser.

Instead how about a new entry to the StorageMask enum in StoragePartition that's
passed to AsyncClearData methods

[dsinclair, 2013-03-22 14:20:46]

On 2013/03/22 06:24:35, jam wrote:
> I don't think you need to add a new interface to content\public\browser.
> 
> Instead how about a new entry to the StorageMask enum in StoragePartition
that's
> passed to AsyncClearData methods

Using StoragePartition would require some updates to storage_partition. I'd need
to add an:

void AsyncClearDataBetween(uint32 storage_mask, const base::Time& begin, const
base::Time& end, const base::Closure& callback);

I need the two time parameters because the browser_data_remover allows you to
clear cache for specific periods of time. I need the callback so I can notify
the browser_data_remover when I'm done so it can update the API when all data is
cleared. To be complete, I'd then need to update the items getting cleared in
StoragePartition to also honour the begin/end times for clearing their data.

Currently, browser_data_remover doesn't use the AsyncClearData method, what it
does is call GetDOMStorageContext() and GetQuotaManager() and then work with the
dom_storage and quota_manager directly.

[dsinclair, 2013-03-22 18:06:49]

On 2013/03/21 21:57:41, Mike West (chromium) wrote:

> One nit, two questions:

I see two questions, what was the nit?

 
> 1. The commit message says "When the browser cache for a given profile is
> cleared...". The code as written clears the shader cache only when "Cookies
and
> site data" is cleared. My (very) vague understanding of the shader cache is
that
> it's much more like transient resources cached temporarily than a cookie. If

This is correct. The underlying caching is done by the net::disk_cache code, so
similar semantics to the HTTP cache in that it's an LRU cache with a fixed size.
The shaders will stay in the cache until they're evicted (or the cache is
cleared).


> that's the case it might make sense to add another bundled type (perhaps
> `REMOVE_CACHED_DATA = REMOVE_CACHE | REMOVE_SHADER_CACHE`) rather than tie
this
> new type to `REMOVE_SITE_DATA`. I guess my question then is simply how
> persistent the cached data actually is?
 
Done. I wasn't sure where all I needed to add the new REMOVE_CACHED_DATA type
so, currently, it's only hooked up to the UI when you select "Empty the cache".
Are there other places that need to be converted to call the new remove instead
of just REMOVE_CACHED data?



> 2. This patch doesn't include any tests. I'm not terribly familiar with the
GPU
> code; is there some mechanism you could use to verify that the disk cache was
> cleared?

Added a test to make sure when you do a Clear on the disk cache the underline
disk_cache mechanism is reporting the cache size a zero.

[dsinclair, 2013-03-22 18:08:23]

https://codereview.chromium.org/12500009/diff/33001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache_impl.cc (right):

https://codereview.chromium.org/12500009/diff/33001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:105: : public base::ThreadChecker,
On 2013/03/21 21:06:36, jonathan.backer wrote:
> nix: ThreadChecker is for CalledOnValidThread, which you are not using.

Done.

https://codereview.chromium.org/12500009/diff/33001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:359:
ShaderClearHelper::~ShaderClearHelper() {
On 2013/03/21 21:06:36, jonathan.backer wrote:
> Does it matter what thread the |cache| is deleted on? If so, maybe verify that
> you're on the IO thread here as well.

Done.

https://codereview.chromium.org/12500009/diff/33001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache_impl.cc:387: path_));
On 2013/03/21 21:06:36, jonathan.backer wrote:
> nit: Comment for the PostTask. This won't be clear a few months down the road.
> Perhaps an alternative is to take a scoped_ref on this. Then you know you
won't
> call the dtor.

Done.

[jam, 2013-03-25 19:57:04]

On 2013/03/22 14:20:46, dsinclair wrote:
> On 2013/03/22 06:24:35, jam wrote:
> > I don't think you need to add a new interface to content\public\browser.
> > 
> > Instead how about a new entry to the StorageMask enum in StoragePartition
> that's
> > passed to AsyncClearData methods
> 
> Using StoragePartition would require some updates to storage_partition. I'd
need
> to add an:
> 
> void AsyncClearDataBetween(uint32 storage_mask, const base::Time& begin, const
> base::Time& end, const base::Closure& callback);
> 
> I need the two time parameters because the browser_data_remover allows you to
> clear cache for specific periods of time. I need the callback so I can notify
> the browser_data_remover when I'm done so it can update the API when all data
is
> cleared. To be complete, I'd then need to update the items getting cleared in
> StoragePartition to also honour the begin/end times for clearing their data.

you can, for now, make this new method say that it only works with shader disk
cache. you can also dcheck that that's the only flag that's passed in.

> 
> Currently, browser_data_remover doesn't use the AsyncClearData method, what it
> does is call GetDOMStorageContext() and GetQuotaManager() and then work with
the
> dom_storage and quota_manager directly.

yeah that code needs to get cleaned up so that it always goes through
StorageParition. there's been only discussion about this, see
https://codereview.chromium.org/12317062/

[dsinclair, 2013-03-26 14:08:54]

Done. PTAL.

I removed the shader_disk_cache from the public API, the clear goes through an
AsyncClearDataBetween call in the StoragePartition now.



On 2013/03/25 19:57:04, jam wrote:
> On 2013/03/22 14:20:46, dsinclair wrote:
> > On 2013/03/22 06:24:35, jam wrote:
> > > I don't think you need to add a new interface to content\public\browser.
> > > 
> > > Instead how about a new entry to the StorageMask enum in StoragePartition
> > that's
> > > passed to AsyncClearData methods
> > 
> > Using StoragePartition would require some updates to storage_partition. I'd
> need
> > to add an:
> > 
> > void AsyncClearDataBetween(uint32 storage_mask, const base::Time& begin,
const
> > base::Time& end, const base::Closure& callback);
> > 
> > I need the two time parameters because the browser_data_remover allows you
to
> > clear cache for specific periods of time. I need the callback so I can
notify
> > the browser_data_remover when I'm done so it can update the API when all
data
> is
> > cleared. To be complete, I'd then need to update the items getting cleared
in
> > StoragePartition to also honour the begin/end times for clearing their data.
> 
> you can, for now, make this new method say that it only works with shader disk
> cache. you can also dcheck that that's the only flag that's passed in.
> 
> > 
> > Currently, browser_data_remover doesn't use the AsyncClearData method, what
it
> > does is call GetDOMStorageContext() and GetQuotaManager() and then work with
> the
> > dom_storage and quota_manager directly.
> 
> yeah that code needs to get cleaned up so that it always goes through
> StorageParition. there's been only discussion about this, see
> https://codereview.chromium.org/12317062/

[jam, 2013-03-26 17:04:16]

https://codereview.chromium.org/12500009/diff/68001/content/browser/storage_p...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/12500009/diff/68001/content/browser/storage_p...
content/browser/storage_partition_impl.cc:144: const base::FilePath&
context_path,
this makes me wonder, should the shader cache be per browser context, or per
storage paritition?

ajwong: wdyt?

[awong, 2013-03-26 18:50:15]

On 2013/03/26 17:04:16, jam wrote:
>
https://codereview.chromium.org/12500009/diff/68001/content/browser/storage_p...
> File content/browser/storage_partition_impl.cc (right):
> 
>
https://codereview.chromium.org/12500009/diff/68001/content/browser/storage_p...
> content/browser/storage_partition_impl.cc:144: const base::FilePath&
> context_path,
> this makes me wonder, should the shader cache be per browser context, or per
> storage paritition?
> 
> ajwong: wdyt?

A good litmus test is whether or not the shader cache for an Apps V2 App should
affect the shader cache in the browser.  If the answer is "no" then the shader
cache should be in the storage partition.

[dsinclair, 2013-03-26 18:57:37]

On 2013/03/26 18:50:15, awong wrote:
> A good litmus test is whether or not the shader cache for an Apps V2 App
should
> affect the shader cache in the browser.  If the answer is "no" then the shader
> cache should be in the storage partition.


I believe we want to the apps v2 shader cache and browser shader cache to go to
the same place.

[awong, 2013-03-26 19:08:10]

On Tue, Mar 26, 2013 at 11:57 AM, <dsinclair@chromium.org> wrote:

> On 2013/03/26 18:50:15, awong wrote:
>
>> A good litmus test is whether or not the shader cache for an Apps V2 App
>>
> should
>
>> affect the shader cache in the browser.  If the answer is "no" then the
>> shader
>> cache should be in the storage partition.
>>
>
>
> I believe we want to the apps v2 shader cache and browser shader cache to
> go to
> the same place.
>

I don't quite know what goes in the shader cache so it's hard for me to
evaluate this statement.  Can you elaborate on this a little.  Naively, I
would have expected this to have the same properties as an the http cache
which we don't share because that would allow v2 apps to affect the general
session, or worse yet, allow v2 apps to read data that only the general
session should have access to.  Where are the shader cache entries coming
from?

-Albert



>
>
https://codereview.chromium.**org/12500009/<https://codereview.chromium.org/1...
>

[dsinclair, 2013-03-26 19:20:01]

On 2013/03/26 19:08:10, awong wrote:
> On Tue, Mar 26, 2013 at 11:57 AM, <mailto:dsinclair@chromium.org> wrote:
> > I believe we want to the apps v2 shader cache and browser shader cache to
> > go to
> > the same place.
> >
> 
> I don't quite know what goes in the shader cache so it's hard for me to
> evaluate this statement.  Can you elaborate on this a little.  Naively, I
> would have expected this to have the same properties as an the http cache
> which we don't share because that would allow v2 apps to affect the general
> session, or worse yet, allow v2 apps to read data that only the general
> session should have access to.  Where are the shader cache entries coming
> from?
> 


The shader disk cache contains compiled GL shaders. When you go to a page that
has WebGL content (WebGL aquarium, get.webgl.org, etc) we cache the compiled
shader to make future visits to the page faster. Currently this is done with a
memory cache in the GPU process. The shader disk cache is the on-disk
representation of the GPU memory cache so we get the benefits over browser
restarts.

The shaders for a given profile are read when a renderer is created and sent to
the GPU process which places the shaders into its in memory cache. From that
point, we write any new shaders to the shader disk cache but don't read until
the next time the renderer is started.

For Apps v2, there is no mechanism to read the shaders disk cache directly,
they're sent to the GPU process. I beleive, at most the app would work faster as
the shader would already be compiled and we'd use it out of the memory cache.

[awong, 2013-03-26 19:57:15]

Gotcha.  How is the cache indexed then?  The main concern I could see is if
a <webview /> tag in a V2 App and the main browsing session both try to
read the same webpage with different cookies and thus could receive the
same shader program from the cache even though the server might provide
different ones.

Is that possible or is there some other way these shader cache entries are
separated?

Even if it is shared, I would just consider this an edge case in the
isolation semantics of apps since I don't think shaders really contain that
much info.  Since this is just a cache, if we found it became a problem, we
could move it in the future w/o too much problem.

-Albert



On Tue, Mar 26, 2013 at 12:20 PM, <dsinclair@chromium.org> wrote:

> On 2013/03/26 19:08:10, awong wrote:
>
>  On Tue, Mar 26, 2013 at 11:57 AM, <mailto:dsinclair@chromium.org**>
>> wrote:
>> > I believe we want to the apps v2 shader cache and browser shader cache
>> to
>> > go to
>> > the same place.
>> >
>>
>
>  I don't quite know what goes in the shader cache so it's hard for me to
>> evaluate this statement.  Can you elaborate on this a little.  Naively, I
>> would have expected this to have the same properties as an the http cache
>> which we don't share because that would allow v2 apps to affect the
>> general
>> session, or worse yet, allow v2 apps to read data that only the general
>> session should have access to.  Where are the shader cache entries coming
>> from?
>>
>
>
>
> The shader disk cache contains compiled GL shaders. When you go to a page
> that
> has WebGL content (WebGL aquarium, get.webgl.org, etc) we cache the
> compiled
> shader to make future visits to the page faster. Currently this is done
> with a
> memory cache in the GPU process. The shader disk cache is the on-disk
> representation of the GPU memory cache so we get the benefits over browser
> restarts.
>
> The shaders for a given profile are read when a renderer is created and
> sent to
> the GPU process which places the shaders into its in memory cache. From
> that
> point, we write any new shaders to the shader disk cache but don't read
> until
> the next time the renderer is started.
>
> For Apps v2, there is no mechanism to read the shaders disk cache directly,
> they're sent to the GPU process. I beleive, at most the app would work
> faster as
> the shader would already be compiled and we'd use it out of the memory
> cache.
>
>
https://codereview.chromium.**org/12500009/<https://codereview.chromium.org/1...
>

[jam, 2013-03-26 23:19:08]

On 2013/03/26 19:57:15, awong wrote:
> Gotcha.  How is the cache indexed then?  The main concern I could see is if
> a <webview /> tag in a V2 App and the main browsing session both try to
> read the same webpage with different cookies and thus could receive the
> same shader program from the cache even though the server might provide
> different ones.
> 
> Is that possible or is there some other way these shader cache entries are
> separated?
> 
> Even if it is shared, I would just consider this an edge case in the
> isolation semantics of apps since I don't think shaders really contain that
> much info.  Since this is just a cache, if we found it became a problem, we
> could move it in the future w/o too much problem.
> 
> -Albert

from a code-view perspective, it seems it would be simpler if it belonged on the
storage partition instead of the browser context.

[dsinclair, 2013-03-27 00:39:49]

On 2013/03/26 23:19:08, jam wrote:
> from a code-view perspective, it seems it would be simpler if it belonged on
the
> storage partition instead of the browser context.


Would it be acceptable to merge this as-is, so I can merge it into M27. Then,
create a bug to move from profile to storage partition that I'll start working
on?

The code to create and work with the shader disk cache is already committed and
we'd like to have to cleanup code go into M27 along with the creation/usage
code. (The bug is currently marked as ReleaseBlock-Stable.)

[awong, 2013-03-27 01:59:42]

From a isolation perspective, there's no problem with this because you are
just preserving existing behavior. Note though that if you do this, when
you migrate, you will have to write code to clean up old profiles by
deleting the old cache directory from disk.  This is often somewhat
annoying.

-Albert


On Tue, Mar 26, 2013 at 5:39 PM, <dsinclair@chromium.org> wrote:

> On 2013/03/26 23:19:08, jam wrote:
>
>> from a code-view perspective, it seems it would be simpler if it belonged
>> on
>>
> the
>
>> storage partition instead of the browser context.
>>
>
>
> Would it be acceptable to merge this as-is, so I can merge it into M27.
> Then,
> create a bug to move from profile to storage partition that I'll start
> working
> on?
>
> The code to create and work with the shader disk cache is already
> committed and
> we'd like to have to cleanup code go into M27 along with the creation/usage
> code. (The bug is currently marked as ReleaseBlock-Stable.)
>
>
https://codereview.chromium.**org/12500009/<https://codereview.chromium.org/1...
>

[dsinclair, 2013-03-27 13:12:21]

On 2013/03/27 01:59:42, awong wrote:
> From a isolation perspective, there's no problem with this because you are
> just preserving existing behavior. Note though that if you do this, when
> you migrate, you will have to write code to clean up old profiles by
> deleting the old cache directory from disk.  This is often somewhat
> annoying.
> 

Would you be able to point me at any code where people have done this kind of
migration in the past within Chromium?

[jam, 2013-03-27 16:14:48]

On 2013/03/27 00:39:49, dsinclair wrote:
> On 2013/03/26 23:19:08, jam wrote:
> > from a code-view perspective, it seems it would be simpler if it belonged on
> the
> > storage partition instead of the browser context.
> 
> 
> Would it be acceptable to merge this as-is, so I can merge it into M27. Then,
> create a bug to move from profile to storage partition that I'll start working
> on?

I would prefer that it lands with the cache per storage partition, because then
it would remove some code from this change.

you can do another change later which deletes the old cache directory if it's
found

> 
> The code to create and work with the shader disk cache is already committed
and
> we'd like to have to cleanup code go into M27 along with the creation/usage
> code. (The bug is currently marked as ReleaseBlock-Stable.)

[dsinclair, 2013-03-29 14:41:18]

On 2013/03/27 16:14:48, jam wrote:
> I would prefer that it lands with the cache per storage partition, because
then
> it would remove some code from this change.

Done. PTAL.

[jam, 2013-04-02 17:34:50]

lgtm

https://codereview.chromium.org/12500009/diff/92001/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/12500009/diff/92001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.cc:782: // This function
should be called on the UI thread.
nit: redundant comment, remove per style guide

https://codereview.chromium.org/12500009/diff/92001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache.h (right):

https://codereview.chromium.org/12500009/diff/92001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache.h:43: int Clear(
nit: here and below, add comments for (at least) public methods

https://codereview.chromium.org/12500009/diff/92001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache.h:114: virtual ~ShaderCacheFactory();
nit: not needed anymore?

[Mike West, 2013-04-02 17:43:39]

BrowsingDataRemover bits are fine, as long as you add a test for
AsyncClearDataBetween as we just discussed.

Preemptive LGTM, so you can avoid another time-zone hop tonight.

[dsinclair, 2013-04-03 18:59:23]

jam PTAL. I added a unit test for AsyncClearDataBetween.

https://codereview.chromium.org/12500009/diff/92001/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/12500009/diff/92001/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.cc:782: // This function
should be called on the UI thread.
On 2013/04/02 17:34:50, jam wrote:
> nit: redundant comment, remove per style guide

Done.

https://codereview.chromium.org/12500009/diff/92001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache.h (right):

https://codereview.chromium.org/12500009/diff/92001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache.h:43: int Clear(
On 2013/04/02 17:34:50, jam wrote:
> nit: here and below, add comments for (at least) public methods

Done.

https://codereview.chromium.org/12500009/diff/92001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache.h:114: virtual ~ShaderCacheFactory();
On 2013/04/02 17:34:50, jam wrote:
> nit: not needed anymore?

error: [chromium-style] Complex class/struct needs an explicit out-of-line
destructor.

[jam, 2013-04-04 14:53:31]

https://codereview.chromium.org/12500009/diff/92001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache.h (right):

https://codereview.chromium.org/12500009/diff/92001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache.h:114: virtual ~ShaderCacheFactory();
On 2013/04/03 18:59:23, dsinclair wrote:
> On 2013/04/02 17:34:50, jam wrote:
> > nit: not needed anymore?
> 
> error: [chromium-style] Complex class/struct needs an explicit out-of-line
> destructor.

I meant the virtual

https://codereview.chromium.org/12500009/diff/106001/content/browser/storage_...
File content/browser/storage_partition_impl_unittest.cc (right):

https://codereview.chromium.org/12500009/diff/106001/content/browser/storage_...
content/browser/storage_partition_impl_unittest.cc:20: int kCallbackTimeout =
1000;
why are you adding this timeout step? it seems to mask real bugs if they occur,
and also it seems like an anti-pattern (test isn't sure if it'll complete or
not)

https://codereview.chromium.org/12500009/diff/106001/content/browser/storage_...
content/browser/storage_partition_impl_unittest.cc:130: EXPECT_EQ(0ul, Size());
nit: 0u is enough i think

[dsinclair, 2013-04-04 16:03:59]

https://codereview.chromium.org/12500009/diff/92001/content/browser/gpu/shade...
File content/browser/gpu/shader_disk_cache.h (right):

https://codereview.chromium.org/12500009/diff/92001/content/browser/gpu/shade...
content/browser/gpu/shader_disk_cache.h:114: virtual ~ShaderCacheFactory();
On 2013/04/04 14:53:31, jam wrote:
> On 2013/04/03 18:59:23, dsinclair wrote:
> > On 2013/04/02 17:34:50, jam wrote:
> > > nit: not needed anymore?
> > 
> > error: [chromium-style] Complex class/struct needs an explicit out-of-line
> > destructor.
> 
> I meant the virtual

Oh, sorry. Done.

https://codereview.chromium.org/12500009/diff/106001/content/browser/storage_...
File content/browser/storage_partition_impl_unittest.cc (right):

https://codereview.chromium.org/12500009/diff/106001/content/browser/storage_...
content/browser/storage_partition_impl_unittest.cc:20: int kCallbackTimeout =
1000;
On 2013/04/04 14:53:31, jam wrote:
> why are you adding this timeout step? it seems to mask real bugs if they
occur,
> and also it seems like an anti-pattern (test isn't sure if it'll complete or
> not)

Removed as discussed.

https://codereview.chromium.org/12500009/diff/106001/content/browser/storage_...
content/browser/storage_partition_impl_unittest.cc:130: EXPECT_EQ(0ul, Size());
On 2013/04/04 14:53:31, jam wrote:
> nit: 0u is enough i think

Done.

[jam, 2013-04-04 16:11:44]

lgtm

[commit-bot: I haz the power, 2013-04-04 16:22:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsinclair@chromium.org/12500009/117001

[commit-bot: I haz the power, 2013-04-04 16:30:43]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-04-05 02:56:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsinclair@chromium.org/12500009/121003

[commit-bot: I haz the power, 2013-04-05 05:58:20]

Change committed as 192511