Implement a method to get the total size of a cache entry

Implement a method to get the total size of a cache entry. This method will be used to calculate the size of subsets of the cache in http://crrev.com/2612033002.

BUG=671196

[dullweber, 2016-12-20 13:36:17]

Description was changed from

==========
Implement a GetEntrySize() for all cache entry implementations. This will be
used to calculate the size of subsets of the cache in
http://crrev.com/2556363003

BUG=671196
==========

to

==========
Implement a method to get the total size of a cache entry for all
implementations. This method will be used to calculate the size of subsets of
the cache in http://crrev.com/2556363003

BUG=671196
==========

[dullweber, 2016-12-20 13:36:54]

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

[commit-bot: I haz the power, 2016-12-20 13:37: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-12-20 14:30:13]

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

[commit-bot: I haz the power, 2016-12-20 14:30:14]

Dry run: This issue passed the CQ dry run.

[dullweber, 2016-12-21 13:39:41]

Description was changed from

==========
Implement a method to get the total size of a cache entry for all
implementations. This method will be used to calculate the size of subsets of
the cache in http://crrev.com/2556363003

BUG=671196
==========

to

==========
Implement a method to get the total size of a cache entry. This method will be
used to calculate the size of subsets of the cache in
http://crrev.com/2556363003.

BUG=671196
==========

[dullweber, 2016-12-21 13:53:08]

dullweber@chromium.org changed reviewers:
+ gavinp@chromium.org

[dullweber, 2016-12-21 13:53:09]

Gavin, please review this CL.

[gavinp, 2016-12-22 16:06:57]

On 2016/12/21 13:53:09, dullweber wrote:
> Gavin, please review this CL.

I'm not convinced that we need a separate implementation on each backend. This
API doesn't need to be more complex, and I don't see any benefit.

  - None of the implementations are really all that accurate, except perhaps the
one on the Simple Cache backend.
  - And even that one isn't accurate, because of course there's loss from
rounding up allocation units (typically 4096 bytes).

So, if you really want this method, why not implement it directly on
disk_cache::Entry?

[gavinp, 2016-12-22 18:37:27]

On 2016/12/22 16:06:57, gavinp wrote:
> On 2016/12/21 13:53:09, dullweber wrote:
> > Gavin, please review this CL.
> 
> I'm not convinced that we need a separate implementation on each backend. This
> API doesn't need to be more complex, and I don't see any benefit.
> 
>   - None of the implementations are really all that accurate, except perhaps
the
> one on the Simple Cache backend.
>   - And even that one isn't accurate, because of course there's loss from
> rounding up allocation units (typically 4096 bytes).
> 
> So, if you really want this method, why not implement it directly on
> disk_cache::Entry?

I peeked briefly at the upstream review. I see what you were thinking about
getting it efficiently from the index metadata; that could make this all very
fast. So fast that you probably won't need to bother having the race described
in the bug.

The simple cache implementation of "DoomEntriesBetween" doesn't open the entries
I believe; so an interface like that could give you your size pretty quickly
from the index.

But as long as you're iterating, there's no savings from using the index.
Iteration, by its nature, requires opening each entry. And if you have a
disk_cache::Entry*, then you already opened the entry. The size data is stored
in core on the IO thread, so you've already paid for it.

[dullweber, 2016-12-27 09:46:38]

On 2016/12/22 16:06:57, gavinp wrote:
> On 2016/12/21 13:53:09, dullweber wrote:
> > Gavin, please review this CL.
> 
> I'm not convinced that we need a separate implementation on each backend. This
> API doesn't need to be more complex, and I don't see any benefit.
> 
>   - None of the implementations are really all that accurate, except perhaps
the
> one on the Simple Cache backend.
>   - And even that one isn't accurate, because of course there's loss from
> rounding up allocation units (typically 4096 bytes).
> 
> So, if you really want this method, why not implement it directly on
> disk_cache::Entry?

My initial decision to have almost identical code in each entry was, that it
looks like kNumStreams=3 is an implementation detail and not necessarily the
same on all implementations?

I could move all implementations to disk_cache::Entry except for the size of a
SimpleCache Entry because it adds its header size to the size of each stream of
entry.

The size only has to be as exact as the size returned by
disk_cache::Backend::CalculateSizeOfAllEntries() in order to be consistent e.g.
when a user only has cached data for a few days and selects "Past 4 weeks" or
"all time" in the CBD dialog.

[dullweber, 2016-12-27 09:57:09]

On 2016/12/22 18:37:27, gavinp wrote:
> On 2016/12/22 16:06:57, gavinp wrote:
> > On 2016/12/21 13:53:09, dullweber wrote:
> > > Gavin, please review this CL.
> > 
> > I'm not convinced that we need a separate implementation on each backend.
This
> > API doesn't need to be more complex, and I don't see any benefit.
> > 
> >   - None of the implementations are really all that accurate, except perhaps
> the
> > one on the Simple Cache backend.
> >   - And even that one isn't accurate, because of course there's loss from
> > rounding up allocation units (typically 4096 bytes).
> > 
> > So, if you really want this method, why not implement it directly on
> > disk_cache::Entry?
> 
> I peeked briefly at the upstream review. I see what you were thinking about
> getting it efficiently from the index metadata; that could make this all very
> fast. So fast that you probably won't need to bother having the race described
> in the bug.
> 
> The simple cache implementation of "DoomEntriesBetween" doesn't open the
entries
> I believe; so an interface like that could give you your size pretty quickly
> from the index.
> 
> But as long as you're iterating, there's no savings from using the index.
> Iteration, by its nature, requires opening each entry. And if you have a
> disk_cache::Entry*, then you already opened the entry. The size data is stored
> in core on the IO thread, so you've already paid for it.

I mostly did the complicated counting with timeout logic for the blockfile
cache, which is still used on Windows. Is there any plan to replace it with
SimpleCache as well? Then it would make this all much easier :) I didn't find a
way to implement counting the size without opening each entry in the blockfile
cache. 
My idea is to add a "CacheSizeBetween()" method to the backend that by default
returns something like ERR_NOT_IMPLEMENTED if there is no efficient way to do
this and override it for the SimpleCache with a method using the index.
The CacheCountingHelper with timeout will only be used if the backend returns
ERR_NOT_IMPLEMENTED from "CacheSizeBetween()".

[dullweber, 2017-01-05 09:24:19]

Description was changed from

==========
Implement a method to get the total size of a cache entry. This method will be
used to calculate the size of subsets of the cache in
http://crrev.com/2556363003.

BUG=671196
==========

to

==========
Implement a method to get the total size of a cache entry. This method will be
used to calculate the size of subsets of the cache in
http://crrev.com/2612033002.

BUG=671196
==========

[gavinp, 2017-01-11 17:28:48]

On 2016/12/27 09:57:09, dullweber wrote:
> On 2016/12/22 18:37:27, gavinp wrote:
> > On 2016/12/22 16:06:57, gavinp wrote:
> > > On 2016/12/21 13:53:09, dullweber wrote:
> > > > Gavin, please review this CL.
> > > 
> > > I'm not convinced that we need a separate implementation on each backend.
> This
> > > API doesn't need to be more complex, and I don't see any benefit.
> > > 
> > >   - None of the implementations are really all that accurate, except
perhaps
> > the
> > > one on the Simple Cache backend.
> > >   - And even that one isn't accurate, because of course there's loss from
> > > rounding up allocation units (typically 4096 bytes).
> > > 
> > > So, if you really want this method, why not implement it directly on
> > > disk_cache::Entry?
> > 
> > I peeked briefly at the upstream review. I see what you were thinking about
> > getting it efficiently from the index metadata; that could make this all
very
> > fast. So fast that you probably won't need to bother having the race
described
> > in the bug.
> > 
> > The simple cache implementation of "DoomEntriesBetween" doesn't open the
> entries
> > I believe; so an interface like that could give you your size pretty quickly
> > from the index.
> > 
> > But as long as you're iterating, there's no savings from using the index.
> > Iteration, by its nature, requires opening each entry. And if you have a
> > disk_cache::Entry*, then you already opened the entry. The size data is
stored
> > in core on the IO thread, so you've already paid for it.
> 
> I mostly did the complicated counting with timeout logic for the blockfile
> cache, which is still used on Windows. Is there any plan to replace it with
> SimpleCache as well? Then it would make this all much easier :) I didn't find
a
> way to implement counting the size without opening each entry in the blockfile
> cache. 

But why is GetEntrySize() being implemented separately on each backend. Couldn't
the same implementation serve all three backends?

> My idea is to add a "CacheSizeBetween()" method to the backend that by default
> returns something like ERR_NOT_IMPLEMENTED if there is no efficient way to do
> this and override it for the SimpleCache with a method using the index.
> The CacheCountingHelper with timeout will only be used if the backend returns
> ERR_NOT_IMPLEMENTED from "CacheSizeBetween()".

Sure, this seems fair; or possibly just generalise DoomEntriesBetween() to do
other calculations. This makes more sense to me as a direction to go in the long
run, but I just don't understand why this CL is part of the path to get there.