Calculate the size of all cache entries between two points in time.

Calculate the size of all cache entries between two points in time if it can be done efficiently. 
Size calculation is implemented for SimpleBackend using the SimpleIndex and the MemBackend. The blockfile Backend returns ERR_NOT_IMPLEMENTED because it would have to read all entries from disk.

BUG=671196
Review-Url: https://codereview.chromium.org/2626173003
Cr-Commit-Position: refs/heads/master@{#443870}
Committed: https://chromium.googlesource.com/chromium/src/+/a0030054d0f06d89c91a0b6198a1956cd4915861

[dullweber, 2017-01-12 12:17:59]

Description was changed from

==========
Implement calculation the size of all cache entries between two points in time.

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

to

==========
Implement calculating the size of all cache entries between two points in time.

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

[dullweber, 2017-01-12 12:18:05]

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

[commit-bot: I haz the power, 2017-01-12 12:18:20]

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-01-12 12:27:35]

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

[commit-bot: I haz the power, 2017-01-12 12:27:36]

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

[dullweber, 2017-01-12 12:28:21]

Description was changed from

==========
Implement calculating the size of all cache entries between two points in time.

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

to

==========
Calculate the size of all cache entries between two points in time if it can be
done efficiently. 
This is implemented for SimpleBackend and MemBackend. The blockfile Backend
returns ERR_NOT_IMPLEMENTED because it would have to read all entries from disk.

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

[dullweber, 2017-01-12 12:28:56]

Description was changed from

==========
Calculate the size of all cache entries between two points in time if it can be
done efficiently. 
This is implemented for SimpleBackend and MemBackend. The blockfile Backend
returns ERR_NOT_IMPLEMENTED because it would have to read all entries from disk.

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

to

==========
Calculate the size of all cache entries between two points in time if it can be
done efficiently. 
Size calculation is implemented for SimpleBackend and MemBackend. The blockfile
Backend returns ERR_NOT_IMPLEMENTED because it would have to read all entries
from disk.

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

[dullweber, 2017-01-12 12:36:49]

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

[commit-bot: I haz the power, 2017-01-12 12:37:07]

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

[dullweber, 2017-01-12 12:57:32]

Description was changed from

==========
Calculate the size of all cache entries between two points in time if it can be
done efficiently. 
Size calculation is implemented for SimpleBackend and MemBackend. The blockfile
Backend returns ERR_NOT_IMPLEMENTED because it would have to read all entries
from disk.

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

to

==========
Calculate the size of all cache entries between two points in time if it can be
done efficiently. 
Size calculation is implemented for SimpleBackend using the SimpleIndex and the
MemBackend. The blockfile Backend returns ERR_NOT_IMPLEMENTED because it would
have to read all entries from disk.

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

[commit-bot: I haz the power, 2017-01-12 13:52:17]

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

[commit-bot: I haz the power, 2017-01-12 13:52:18]

Dry run: This issue passed the CQ dry run.

[dullweber, 2017-01-12 13:58:06]

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

[dullweber, 2017-01-12 13:58:06]

Hi Gavin, I implemented cache size calculation for the SimpleBackend and closed
the other reviews. I also implemented it for MemBackend as it looks fast as
well. Please have a look at it.

[gavinp, 2017-01-12 16:07:36]

this looks pretty reasonable, and probably pretty close. some questions below
about organisation.

Thanks!

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/blockfil...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/blockfil...
net/disk_cache/blockfile/backend_impl.cc:1279: return net::ERR_NOT_IMPLEMENTED;
Why not move all of these ERR_NOT_IMPLEMENTED versions up into the
disk_cache::Backend directly? Seems more logical than repeating this over and
over again.

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:22: #include "net/base/net_errors.h"
Amazing that this wasn't there already!!! Good catch!

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:158: // of ERR_NOT_IMPLEMTED means that there is no
efficient way for the backend
typo: ERR_NOT_IMPLEMENTED, isn't it?

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:165: const CompletionCallback& callback) = 0;
Does this method need to return a callback? Why not insist on it always being
synchronous?

The big question here is, if we make it synchronous, how do we handle the case
of the simple cache index not being ready. Typically that's not a long wait, but
it can be relatively long (check out the UMA on this if you're curious; index
rebuilds can be brutal here)...

Returning with an error then falling back to item-by-item is pretty lame, since
the first step in item-by-item iteration is waiting for the index. So I guess
maybe this should be asynchronous. What do you think?

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_backend_impl.cc (right):

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_backend_impl.cc:214: end_time = Time::Max();
If you're going to cut and paste from DoomEntriesBetween above, might as well
match the vertical spacing. I think you're right to remove this vertical
whitespace, but why not go fix it up above while we're here so the two match
completely?

[dullweber, 2017-01-12 17:03:15]

Thanks for the fast review! I fixed the small issues and responded to the other
questions below

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:22: #include "net/base/net_errors.h"
On 2017/01/12 16:07:36, gavinp wrote:
> Amazing that this wasn't there already!!! Good catch!

Oh, that's funny. The include is here because I tried to add the "return
ERR_NOT_IMPLEMENTED;" here first but it complained that I can't put this in a
header file and there was no .cc, so I moved it to the individual
implementations. I'm not sure if this class should have implementations because
currently it is a completely virtual interface? If you prefer to have the
default implementation here, I will add the .cc file.

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:158: // of ERR_NOT_IMPLEMTED means that there is no
efficient way for the backend
On 2017/01/12 16:07:36, gavinp wrote:
> typo: ERR_NOT_IMPLEMENTED, isn't it?

Done.

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:165: const CompletionCallback& callback) = 0;
On 2017/01/12 16:07:35, gavinp wrote:
> Does this method need to return a callback? Why not insist on it always being
> synchronous?
> 
> The big question here is, if we make it synchronous, how do we handle the case
> of the simple cache index not being ready. Typically that's not a long wait,
but
> it can be relatively long (check out the UMA on this if you're curious; index
> rebuilds can be brutal here)...
> 
> Returning with an error then falling back to item-by-item is pretty lame,
since
> the first step in item-by-item iteration is waiting for the index. So I guess
> maybe this should be asynchronous. What do you think?

The CalculateSizeOfAllEntries() method that just returns the value of an int
attribute is also asynchronous because it also has to wait for the index to be
ready. I use CalculateSizeOfAllEntries as a fallback if
CalculateSizeOfEntriesBetween returns ERR_NOT_IMPLEMENTED, so the caller of this
method would be asynchronous anyway. The integration of this code is already
finished in this CL: http://crrev.com/2626223002.
I don't think we would gain anything by making this method synchronous.

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_backend_impl.cc (right):

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_backend_impl.cc:214: end_time = Time::Max();
On 2017/01/12 16:07:36, gavinp wrote:
> If you're going to cut and paste from DoomEntriesBetween above, might as well
> match the vertical spacing. I think you're right to remove this vertical
> whitespace, but why not go fix it up above while we're here so the two match
> completely?

Done.

[gavinp, 2017-01-12 20:38:35]

I think you should add the explicit .cc file for disk_cache.h. I think that's
conceptually cleaner than having so many redundant implementations (which,
admittedly, would be caught and cleaned up by link time duplicate function
elimination on some platforms, but still...).

lgtm if you add the disk_cache.h file; I'm happy to take another pass once
you're done if you'd prefer but feel free to go straight to CQ.

Thanks!

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/2626173003/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:165: const CompletionCallback& callback) = 0;
On 2017/01/12 17:03:15, dullweber wrote:
> On 2017/01/12 16:07:35, gavinp wrote:
> > Does this method need to return a callback? Why not insist on it always
being
> > synchronous?
> > 
> > The big question here is, if we make it synchronous, how do we handle the
case
> > of the simple cache index not being ready. Typically that's not a long wait,
> but
> > it can be relatively long (check out the UMA on this if you're curious;
index
> > rebuilds can be brutal here)...
> > 
> > Returning with an error then falling back to item-by-item is pretty lame,
> since
> > the first step in item-by-item iteration is waiting for the index. So I
guess
> > maybe this should be asynchronous. What do you think?
> 
> The CalculateSizeOfAllEntries() method that just returns the value of an int
> attribute is also asynchronous because it also has to wait for the index to be
> ready. I use CalculateSizeOfAllEntries as a fallback if
> CalculateSizeOfEntriesBetween returns ERR_NOT_IMPLEMENTED, so the caller of
this
> method would be asynchronous anyway. The integration of this code is already
> finished in this CL: http://crrev.com/2626223002.
> I don't think we would gain anything by making this method synchronous. 

You've sold me. It's fine as it is.

[gavinp, 2017-01-12 20:51:49]

And, incidentally, disk_cache.h isn't so pure; note that
disk_cache::CreateCacheBackend() is declared there and defined elsewhere.

Now, I am asking the methods you add not be pure virtual, but also worth
mentioning: there's no rule in C++ and never has been in any of the ISO
standards that pure virtual functions not have an implementation.

[gavinp, 2017-01-12 21:19:45]

On 2017/01/12 20:51:49, gavinp wrote:
> And, incidentally, disk_cache.h isn't so pure; note that
> disk_cache::CreateCacheBackend() is declared there and defined elsewhere.
> 
> Now, I am asking the methods you add not be pure virtual, but also worth
> mentioning: there's no rule in C++ and never has been in any of the ISO
> standards that pure virtual functions not have an implementation.

Which brings to mind: maybe you just rename "cache_creator.cc" to
"disk_cache.cc" because if you don't do that, it seems kind of awful.

[gavinp, 2017-01-12 21:19:48]

On 2017/01/12 20:51:49, gavinp wrote:
> And, incidentally, disk_cache.h isn't so pure; note that
> disk_cache::CreateCacheBackend() is declared there and defined elsewhere.
> 
> Now, I am asking the methods you add not be pure virtual, but also worth
> mentioning: there's no rule in C++ and never has been in any of the ISO
> standards that pure virtual functions not have an implementation.

Which brings to mind: maybe you just rename "cache_creator.cc" to
"disk_cache.cc" because if you don't do that, it seems kind of awful.

[dullweber, 2017-01-13 09:45:33]

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

[commit-bot: I haz the power, 2017-01-13 09:45:52]

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

[dullweber, 2017-01-13 10:04:35]

On 2017/01/12 21:19:48, gavinp wrote:
> On 2017/01/12 20:51:49, gavinp wrote:
> > And, incidentally, disk_cache.h isn't so pure; note that
> > disk_cache::CreateCacheBackend() is declared there and defined elsewhere.
> > 
> > Now, I am asking the methods you add not be pure virtual, but also worth
> > mentioning: there's no rule in C++ and never has been in any of the ISO
> > standards that pure virtual functions not have an implementation.
> 
> Which brings to mind: maybe you just rename "cache_creator.cc" to
> "disk_cache.cc" because if you don't do that, it seems kind of awful.

I created a default implementation and removed the duplicated functions. I fixed
a few other small things and the formatter changed the code from cache_creator a
bit, so I will wait until you took another look at it.

[commit-bot: I haz the power, 2017-01-13 10:42:54]

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

[commit-bot: I haz the power, 2017-01-13 10:42:55]

Dry run: This issue passed the CQ dry run.

[gavinp, 2017-01-13 20:51:16]

lgtm.

Good work! Thanks!

[dullweber, 2017-01-16 10:06:51]

The CQ bit was checked by dullweber@chromium.org

[commit-bot: I haz the power, 2017-01-16 10:07:01]

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-01-16 11:56:09]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484561211503800,
"parent_rev": "e1926525b79fdbd181867190c7ef012e7f594e98", "commit_rev":
"a0030054d0f06d89c91a0b6198a1956cd4915861"}

[commit-bot: I haz the power, 2017-01-16 11:57:02]

Description was changed from

==========
Calculate the size of all cache entries between two points in time if it can be
done efficiently. 
Size calculation is implemented for SimpleBackend using the SimpleIndex and the
MemBackend. The blockfile Backend returns ERR_NOT_IMPLEMENTED because it would
have to read all entries from disk.

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

to

==========
Calculate the size of all cache entries between two points in time if it can be
done efficiently. 
Size calculation is implemented for SimpleBackend using the SimpleIndex and the
MemBackend. The blockfile Backend returns ERR_NOT_IMPLEMENTED because it would
have to read all entries from disk.

BUG=671196

Review-Url: https://codereview.chromium.org/2626173003
Cr-Commit-Position: refs/heads/master@{#443870}
Committed:
https://chromium.googlesource.com/chromium/src/+/a0030054d0f06d89c91a0b6198a1...
==========

[commit-bot: I haz the power, 2017-01-16 11:57:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a0030054d0f06d89c91a0b6198a1...