Implement cache counting for the simple and memory backends.

Implement cache counting for the simple and memory backends.

This is a followup to https://codereview.chromium.org/1304363013/, which implemented it for the blockfile backend.

BUG=510028
Committed: https://crrev.com/a319545886d0e297b88301f01b804432bea602f7
Cr-Commit-Position: refs/heads/master@{#354037}

[msramek, 2015-10-09 09:32:23]

msramek@chromium.org changed reviewers:
+ pasko@chromium.org, ttuttle@chromium.org

[msramek, 2015-10-09 09:32:24]

Hi Egor and Thomas,

can you please have a look at this?

Thanks!
Martin

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_backend_impl.cc:477: result =
static_cast<int>(index_->GetCacheSize());
cache_size_, and thus also GetCacheSize(), are uint64.

However, I designed CalculateSizeOfAllEntries() to return an int, because
CreateCacheBackend takes a "max_bytes" parameter which is also an int.

So I'm a bit confused here - can the simple cache have a size larger than MAXINT
or not?

[pasko, 2015-10-09 15:52:02]

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

[pasko, 2015-10-09 15:52:03]

Gavin is back, so adding him as the main reviewer.

I would prefer to postpone my review until it is approved by Gavin, since he is
the primary owner of the simplecache at the moment.

Otherwise, interesting subtleties, thanks!

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/backend_unit...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/backend_unit...
net/disk_cache/backend_unittest.cc:1697: // the stream 1 in simple cache
triggers a write to the stream 0 as well.
This is subtle. Will the stream 0 contain the checksum?

The time we write the stream 0 is exactly the time we write the the stream 1. So
without having a detailed look I don't see why this would be non-deterministic.

Generally, the sizes seem to be calculated quite differently for both backends.
Shall we just throw different tests for each? Reading through these differences
in a single test is not very pleasing.

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_backend_impl.cc:477: result =
static_cast<int>(index_->GetCacheSize());
On 2015/10/09 09:32:24, msramek wrote:
> cache_size_, and thus also GetCacheSize(), are uint64.
> 
> However, I designed CalculateSizeOfAllEntries() to return an int, because
> CreateCacheBackend takes a "max_bytes" parameter which is also an int.
> 
> So I'm a bit confused here - can the simple cache have a size larger than
MAXINT
> or not?

We do not want to introduce more constraints in the simplecache implementation,
such as  INT_MAX. The deficiency of the cache interface is bad, but let's not
make it worse.

It may be non-theoretical soon :)

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_index.cc:243: return cache_size_;
DCHECK(initialized_) please

[msramek, 2015-10-13 09:45:07]

Very well. Welcome back, Gavin, and please have a look :)

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/backend_unit...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/backend_unit...
net/disk_cache/backend_unittest.cc:1697: // the stream 1 in simple cache
triggers a write to the stream 0 as well.
On 2015/10/09 15:52:03, pasko wrote:
> This is subtle. Will the stream 0 contain the checksum?
> 
> The time we write the stream 0 is exactly the time we write the the stream 1.
So
> without having a detailed look I don't see why this would be
non-deterministic.
> 


So, I observed that writing to stream 1 triggers two calls of UpdateEntrySize.
That seems to be in line with this comment:

https://code.google.com/p/chromium/codesearch#chromium/src/net/disk_cache/sim...

Now the first write is synchronous in the test, because it's done through
DiskCacheTestWithCache::WriteData, but the other one is not. It's not clear to
me why it gets to the IO thread later than CalculateSizeOfAllEntries() below - I
guess it only happens after SimpleEntryImpl::RunNextOperationIfNeeded() which is
not invoked immediately.

This is as far as I got with my debugging when I decided that this behavior is
not important for this test, and I'll just fix it in the simplest way.


> Generally, the sizes seem to be calculated quite differently for both
backends.
> Shall we just throw different tests for each? Reading through these
differences
> in a single test is not very pleasing.

How about now? I extracted the size estimation into a separate function. I
wouldn't go as far as having a separate test, because they share all the rest
- expectation that the size of empty cache is 0
- expectation that adding and removing and entry doesn't change the result
- and all the boilerplate around generating entries.

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_backend_impl.cc:477: result =
static_cast<int>(index_->GetCacheSize());
On 2015/10/09 15:52:03, pasko wrote:
> On 2015/10/09 09:32:24, msramek wrote:
> > cache_size_, and thus also GetCacheSize(), are uint64.
> > 
> > However, I designed CalculateSizeOfAllEntries() to return an int, because
> > CreateCacheBackend takes a "max_bytes" parameter which is also an int.
> > 
> > So I'm a bit confused here - can the simple cache have a size larger than
> MAXINT
> > or not?
> 
> We do not want to introduce more constraints in the simplecache
implementation,
> such as  INT_MAX. The deficiency of the cache interface is bad, but let's not
> make it worse.
> 
> It may be non-theoretical soon :)

Ah ok, good to know. Still, I'd like to keep the signature as int instead of
uint64 in this CL for several reasons:

1. I would need to change the signature in disk_cache.h as well, bringing it to
an inconsistent state where getting size uses wider ints than setting size.
2. My consumer currently expect getting int from disk cache, so I'll have to
touch a few more files.
3. I'll have to define a new callback, since nothing in completion_callback.h
can return uint64 and negative NET_ERR values as well.
4. There is a precedent for this in SimpleBackendImpl::GetMaxFileSize().

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/1398053002/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_index.cc:243: return cache_size_;
On 2015/10/09 15:52:03, pasko wrote:
> DCHECK(initialized_) please

Done.

[gavinp, 2015-10-13 15:50:12]

Looks mostly good. Sorry for the delay; it was a long weekend in North America
for many.

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:295: // every stream.
Is this right? We now store stream 0 and stream 1 in the same file, yes? And we
don't store stream 2 if there's no data in it. Tricky.

I realize you're just copy/pasting SimpleEntryImpl::GetDiskUsage() here.

How important is it to be byte-perfect here?

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:473: void
SimpleBackendImpl::IndexReadyForCalculation(
Isn't this an ordering violation? See
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:486: callback));
Did git cl format not join these lines? Weird...

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.h:144: void
IndexReadyForCalculation(const CompletionCallback& callback,
Calculation is a really generic name. I'm also not super wedded to parallelism
with "IndexReadyForDoom" above, which also has a confusing name.

How about IndexReadyForSizeCalculation (keeping parallelism)?

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.h:133: // Returns the size of the entire
cache in bytes.
Probably should document that the cache must be initialized.

[msramek, 2015-10-13 17:25:50]

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:295: // every stream.
On 2015/10/13 15:50:12, gavinp wrote:
> Is this right? We now store stream 0 and stream 1 in the same file, yes? And
we
> don't store stream 2 if there's no data in it. Tricky.
> 
> I realize you're just copy/pasting SimpleEntryImpl::GetDiskUsage() here.
> 
> How important is it to be byte-perfect here?

In DiskCacheBackendTest::BackendCalculateSizeOfAllEntries() where I alternate
between writing to streams 0 and 1, I tried writing to all three streams
instead, and the tests still pass. Which means that GetEntryMetadataSize() is
correct in the sense that

sum for entry in cache
  GetEntryMetadataSize() + sum for stream in entry
                             stream data size

equals CalculateSizeOfAllEntries(), which is SimpleIndex::cache_size_ in the
case of simple cache.

Now if I understand your concern correctly, GetDiskUsage() does not check
whether stream 2 is empty, and we use GetDiskUsage() in UpdateEntrySize(). This
means that the cache pretends that the file for stream 2 always exists?

It would be nice to be more precise with this computation, to give the user the
exact disk size freed when deleting the cache, but if the cache itself thinks
that it's larger than it really is, well.. I think with ~250 bytes per entry the
relative error should be low enough that it's OK.

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:473: void
SimpleBackendImpl::IndexReadyForCalculation(
On 2015/10/13 15:50:12, gavinp wrote:
> Isn't this an ordering violation? See
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...

Yep. I tried to follow IndexReadyForDoom, but that one was already misplaced.

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:486: callback));
On 2015/10/13 15:50:12, gavinp wrote:
> Did git cl format not join these lines? Weird...

Ehm, I didn't run it.

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.h:144: void
IndexReadyForCalculation(const CompletionCallback& callback,
On 2015/10/13 15:50:12, gavinp wrote:
> Calculation is a really generic name. I'm also not super wedded to parallelism
> with "IndexReadyForDoom" above, which also has a confusing name.
> 
> How about IndexReadyForSizeCalculation (keeping parallelism)?

Done.

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.h:133: // Returns the size of the entire
cache in bytes.
On 2015/10/13 15:50:12, gavinp wrote:
> Probably should document that the cache must be initialized.

Done.

[gavinp, 2015-10-14 15:08:06]

lgtm. Thanks!

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:486: callback));
On 2015/10/13 17:25:50, msramek wrote:
> On 2015/10/13 15:50:12, gavinp wrote:
> > Did git cl format not join these lines? Weird...
> 
> Ehm, I didn't run it.

We're very diligent about that in net/. Generally if you're not going to run it,
please file a bug against git cl format (I believe tag is Build-Tools) for the
specific problem you're working around.

https://codereview.chromium.org/1398053002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/1398053002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:560: void
SimpleBackendImpl::IndexReadyForDoom(Time initial_time,
Thanks for cleaning this up.

[pasko, 2015-10-14 15:47:23]

lgtm

[msramek, 2015-10-14 16:11:59]

Thanks, guys!

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/1398053002/diff/20001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:486: callback));
On 2015/10/14 15:08:06, gavinp wrote:
> On 2015/10/13 17:25:50, msramek wrote:
> > On 2015/10/13 15:50:12, gavinp wrote:
> > > Did git cl format not join these lines? Weird...
> > 
> > Ehm, I didn't run it.
> 
> We're very diligent about that in net/. Generally if you're not going to run
it,
> please file a bug against git cl format (I believe tag is Build-Tools) for the
> specific problem you're working around.

Acknowledged.

[msramek, 2015-10-14 16:12:05]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2015-10-14 16:12:35]

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

[commit-bot: I haz the power, 2015-10-14 16:23:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-14 16:24:41]

Patchset 3 (id:??) landed as
https://crrev.com/a319545886d0e297b88301f01b804432bea602f7
Cr-Commit-Position: refs/heads/master@{#354037}

[Mark P, 2015-10-14 21:33:55]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1401053004/ by mpearson@chromium.org.

The reason for reverting is: Causes test failures on XP bot:
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build...

DiskCacheBackendTest.SimpleCacheCalculateSizeOfAllEntries (run #1):
[ RUN      ] DiskCacheBackendTest.SimpleCacheCalculateSizeOfAllEntries
c:uild\slave\win_builderuild\src
et\disk_cacheackend_unittest.cc(1730): error: Value of: result
  Actual: 7447
Expected: (count - 1) * count / 2 + total_metadata_size
Which is: 7455
c:uild\slave\win_builderuild\src
et\disk_cacheackend_unittest.cc(1747): error: Value of: new_result
  Actual: 8243
Expected: result + last_entry_size + GetEntryMetadataSize(key)
Which is: 8235
c:uild\slave\win_builderuild\src
et\disk_cacheackend_unittest.cc(1751): error: Value of: new_result
  Actual: 7455
Expected: result
Which is: 7447
[  FAILED  ] DiskCacheBackendTest.SimpleCacheCalculateSizeOfAllEntries (171 ms)

.