Reland of Implement cache counting for the simple and memory backends - with fixed test.

Reland of Implement cache counting for the simple and memory backends - with fixed tests.

Patchset #1 is a reland of https://codereview.chromium.org/1398053002/ which was reverted in https://codereview.chromium.org/1401053004/.

Patchset #2 is a quick fix of the failing unittest which caused the revert.

==========================================================

Reland of Implement cache counting for the simple and memory backends. (patchset #1 id:1 of https://codereview.chromium.org/1401053004/ )

Reason for revert:
The original CL was reverted because of a failing unittest. The results in the unittest are off by 8 bytes. 8 bytes is the size of the second-to-last entry, which is the last one where we wrote to the stream #1. As the comment in this unittest already mentions, writing to the stream #1 triggers another operation that is asynchronous and can possibly happen after the call to CalculateSizeOfAllEntries. Apparently, this didn't happen on Linux, but it did happen on Windows.

The test can be fixed by only writing to the stream #0.

Original issue's description:
> Revert of Implement cache counting for the simple and memory backends. (patchset #3 id:40001 of https://codereview.chromium.org/1398053002/ )
>
> Reason for revert:
> Causes test failures on XP bot:
> https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds/40465/steps/net_unittests%20on%20Windows-XP-SP3/logs/DiskCacheBackendTest.SimpleCacheCalculateSizeOfAllEntries
>
> 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)
>
> Original issue's description:
> > 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}
>
> TBR=gavinp@chromium.org,pasko@chromium.org,ttuttle@chromium.org,msramek@chromium.org
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=510028
>
> Committed: https://crrev.com/81c61dd0811b103aeca08e1a5184db725fdb19fb
> Cr-Commit-Position: refs/heads/master@{#354118}

R=gavinp@chromium.org,pasko@chromium.org,ttuttle@chromium.org,mpearson@chromium.org
TBR=gavinp@chromium.org
BUG=510028
Committed: https://crrev.com/88092426385ef40615a0cb47e7262f0b76848947
Cr-Commit-Position: refs/heads/master@{#354757}

[msramek, 2015-10-15 09:49:00]

Created Reland of Implement cache counting for the simple and memory backends.

[msramek, 2015-10-15 10:35:36]

Hi Gavin and Egor,

I'm trying to reland the CL that you LGTM'd for me yesterday. It was reverted
because of a failing test and it seems that the problem is the same as I
described in the comments before - synchronization when writing to stream #1.

Since I'm currently blocked on this CL, I simplified the test to always write to
stream #0 and added a TODO for myself. Does that work for you?

Martin

[pasko, 2015-10-15 11:55:54]

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is
because writing to the stream 1
I would prefer if we did not regress in our test coverage. Can you make this
conditional on simplecache?

It would be even better to factor out the below common code and just write the
simplecache test differently. This would mean having this comment in the
simplecache variant, but not mentioning these quirks in the test for other
backends.

Alternatively: count the size without size of various metadata (is this what the
blockfile backend is doing?), but this may make eviction more complex,
unfortunately.

[msramek, 2015-10-16 13:32:37]

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is
because writing to the stream 1
On 2015/10/15 11:55:54, pasko wrote:
> I would prefer if we did not regress in our test coverage. Can you make this
> conditional on simplecache?
> 
> It would be even better to factor out the below common code and just write the
> simplecache test differently. This would mean having this comment in the
> simplecache variant, but not mentioning these quirks in the test for other
> backends.
> 
> Alternatively: count the size without size of various metadata (is this what
the
> blockfile backend is doing?), but this may make eviction more complex,
> unfortunately.

I spent some more time trying to find out if I can fix this for the simple
cache, but I still don't see a way. As I learned in the meantime, the
stream-1-write-triggers-stream-0-write was red herring.

The actual problem is that SimpleEntry optimistically returns value from
WriteData immediately instead of net::ERR_IO_PENDING and a subsequent callback.
There is no callback, nothing to synchronize on. The entry itself indeed
continues with the next operation, but the actual writing happens
asynchronously. There is a SimpleEntryImpl::WriteOperationComplete callback, but
at that time the entry will have discarded the CompletionCallback that was
passed as a parameter to the original SimpleEntry::Write call.

And none of this is a problem for stream 0, where the writing really happens
synchronously through SimpleEntryImpl::SetStream0Data.

I split the test to two - one for blockfile and memory, and one for simple
cache. In the latter, we only write to stream 0, and I explain in the comment
what happens. I also merged the GetEntryMetadataSize() function back in, since
that one was also split between blockfile/memory and simple.

Unfortunately, a lot of code is still duplicated between tests and I don't think
that it can be elegantly extracted.

Regarding your alternative proposal, I actually like including the metadata, as
the result is more informative (i.e. how much disk space will be freed).
Although as we discussed before, the number is not precise here either.

[pasko, 2015-10-16 14:03:32]

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is
because writing to the stream 1
On 2015/10/16 13:32:37, msramek wrote:
> On 2015/10/15 11:55:54, pasko wrote:
> > I would prefer if we did not regress in our test coverage. Can you make this
> > conditional on simplecache?
> > 
> > It would be even better to factor out the below common code and just write
the
> > simplecache test differently. This would mean having this comment in the
> > simplecache variant, but not mentioning these quirks in the test for other
> > backends.
> > 
> > Alternatively: count the size without size of various metadata (is this what
> the
> > blockfile backend is doing?), but this may make eviction more complex,
> > unfortunately.
> 
> I spent some more time trying to find out if I can fix this for the simple
> cache, but I still don't see a way. As I learned in the meantime, the
> stream-1-write-triggers-stream-0-write was red herring.

yay! Thanks a lot! I could not really find a rational explanation for the
previous theory.

> The actual problem is that SimpleEntry optimistically returns value from
> WriteData immediately instead of net::ERR_IO_PENDING and a subsequent
callback.

Yes, this is called "optimistic writes". We also have an "optimistic creates".
Those are 'fast', but may confuse whole lots of clients. Sorry about that. We
haven't even measured that it's useful in practice :(

You can easily turn this off by doing SetCacheType(net::APP_CACHE).

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


> There is no callback, nothing to synchronize on. The entry itself indeed
> continues with the next operation, but the actual writing happens
> asynchronously. There is a SimpleEntryImpl::WriteOperationComplete callback,
but
> at that time the entry will have discarded the CompletionCallback that was
> passed as a parameter to the original SimpleEntry::Write call.

Also, you can add a non-optimistic operation after that (like ReadData), this
will have a callback, and it will be executed after the write has finished.

Since this sounds more like a hack, I would prefer the net::APP_CACHE way.
Please do. There is a handy net::TestCompletionCallback for waiting.

> And none of this is a problem for stream 0, where the writing really happens
> synchronously through SimpleEntryImpl::SetStream0Data.

good.

> I split the test to two - one for blockfile and memory, and one for simple
> cache. In the latter, we only write to stream 0, and I explain in the comment
> what happens. I also merged the GetEntryMetadataSize() function back in, since
> that one was also split between blockfile/memory and simple.
> 
> Unfortunately, a lot of code is still duplicated between tests and I don't
think
> that it can be elegantly extracted.
> 
> Regarding your alternative proposal, I actually like including the metadata,
as
> the result is more informative (i.e. how much disk space will be freed).
> Although as we discussed before, the number is not precise here either.

In my opinion, precise estimations are not important. Our estimations are
imprecise in any case (we do not count filesystem overhead at all, and I would
be surprised if blockfile cache properly calculated effects of sparsefiles on
linux).

What is important, is to keep complexity under control. The current complexity
in testing is a bit bigger than I expected :(

[pasko, 2015-10-16 14:24:04]

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is
because writing to the stream 1
On 2015/10/16 14:03:32, pasko wrote:
> On 2015/10/16 13:32:37, msramek wrote:
> > On 2015/10/15 11:55:54, pasko wrote:
> > > I would prefer if we did not regress in our test coverage. Can you make
this
> > > conditional on simplecache?
> > > 
> > > It would be even better to factor out the below common code and just write
> the
> > > simplecache test differently. This would mean having this comment in the
> > > simplecache variant, but not mentioning these quirks in the test for other
> > > backends.
> > > 
> > > Alternatively: count the size without size of various metadata (is this
what
> > the
> > > blockfile backend is doing?), but this may make eviction more complex,
> > > unfortunately.
> > 
> > I spent some more time trying to find out if I can fix this for the simple
> > cache, but I still don't see a way. As I learned in the meantime, the
> > stream-1-write-triggers-stream-0-write was red herring.
> 
> yay! Thanks a lot! I could not really find a rational explanation for the
> previous theory.
> 
> > The actual problem is that SimpleEntry optimistically returns value from
> > WriteData immediately instead of net::ERR_IO_PENDING and a subsequent
> callback.
> 
> Yes, this is called "optimistic writes". We also have an "optimistic creates".
> Those are 'fast', but may confuse whole lots of clients. Sorry about that. We
> haven't even measured that it's useful in practice :(

tiny correction: we actually measured that it's helping resources get fetched
1-2% faster, which I forgot, but now was reminded about :)

[msramek, 2015-10-16 14:35:16]

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is
because writing to the stream 1
On 2015/10/16 14:24:04, pasko wrote:
> On 2015/10/16 14:03:32, pasko wrote:
> > On 2015/10/16 13:32:37, msramek wrote:
> > > On 2015/10/15 11:55:54, pasko wrote:
> > > > I would prefer if we did not regress in our test coverage. Can you make
> this
> > > > conditional on simplecache?
> > > > 
> > > > It would be even better to factor out the below common code and just
write
> > the
> > > > simplecache test differently. This would mean having this comment in the
> > > > simplecache variant, but not mentioning these quirks in the test for
other
> > > > backends.
> > > > 
> > > > Alternatively: count the size without size of various metadata (is this
> what
> > > the
> > > > blockfile backend is doing?), but this may make eviction more complex,
> > > > unfortunately.
> > > 
> > > I spent some more time trying to find out if I can fix this for the simple
> > > cache, but I still don't see a way. As I learned in the meantime, the
> > > stream-1-write-triggers-stream-0-write was red herring.
> > 
> > yay! Thanks a lot! I could not really find a rational explanation for the
> > previous theory.
> > 
> > > The actual problem is that SimpleEntry optimistically returns value from
> > > WriteData immediately instead of net::ERR_IO_PENDING and a subsequent
> > callback.
> > 
> > Yes, this is called "optimistic writes". We also have an "optimistic
creates".
> > Those are 'fast', but may confuse whole lots of clients. Sorry about that.
We
> > haven't even measured that it's useful in practice :(
> 
> tiny correction: we actually measured that it's helping resources get fetched
> 1-2% faster, which I forgot, but now was reminded about :)

Ok, then I can revert to Patchset 1 and just add SetCacheType(net::APP_CACHE)
for SimpleCache. I did notice that optimistic operations are switched on only
for DISK_CACHE, but I wasn't sure what are the differences (i.e. what coverage
do I lose now by not testing DISK_CACHE).

[pasko, 2015-10-16 14:43:59]

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is
because writing to the stream 1
On 2015/10/16 14:35:15, msramek wrote:
> On 2015/10/16 14:24:04, pasko wrote:
> > On 2015/10/16 14:03:32, pasko wrote:
> > > On 2015/10/16 13:32:37, msramek wrote:
> > > > On 2015/10/15 11:55:54, pasko wrote:
> > > > > I would prefer if we did not regress in our test coverage. Can you
make
> > this
> > > > > conditional on simplecache?
> > > > > 
> > > > > It would be even better to factor out the below common code and just
> write
> > > the
> > > > > simplecache test differently. This would mean having this comment in
the
> > > > > simplecache variant, but not mentioning these quirks in the test for
> other
> > > > > backends.
> > > > > 
> > > > > Alternatively: count the size without size of various metadata (is
this
> > what
> > > > the
> > > > > blockfile backend is doing?), but this may make eviction more complex,
> > > > > unfortunately.
> > > > 
> > > > I spent some more time trying to find out if I can fix this for the
simple
> > > > cache, but I still don't see a way. As I learned in the meantime, the
> > > > stream-1-write-triggers-stream-0-write was red herring.
> > > 
> > > yay! Thanks a lot! I could not really find a rational explanation for the
> > > previous theory.
> > > 
> > > > The actual problem is that SimpleEntry optimistically returns value from
> > > > WriteData immediately instead of net::ERR_IO_PENDING and a subsequent
> > > callback.
> > > 
> > > Yes, this is called "optimistic writes". We also have an "optimistic
> creates".
> > > Those are 'fast', but may confuse whole lots of clients. Sorry about that.
> We
> > > haven't even measured that it's useful in practice :(
> > 
> > tiny correction: we actually measured that it's helping resources get
fetched
> > 1-2% faster, which I forgot, but now was reminded about :)
> 
> Ok, then I can revert to Patchset 1 and just add SetCacheType(net::APP_CACHE)
> for SimpleCache. I did notice that optimistic operations are switched on only
> for DISK_CACHE, but I wasn't sure what are the differences (i.e. what coverage
> do I lose now by not testing DISK_CACHE).

you would not loose any important coverage in size estimations with that

[msramek, 2015-10-19 08:28:57]

So, Egor and Gavin, can I reland it as it is now? Almost the same as the
original CL, just with SetCacheType(net::APP_CACHE) for the simple cache test.

[pasko, 2015-10-19 08:39:01]

thank you, much simpler now!

lgtm with an extra comment. Given that it's a reland, I think it should be OK to
TBR gavinp@.

https://codereview.chromium.org/1401923006/diff/210001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1401923006/diff/210001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:1764: SetCacheType(net::APP_CACHE);
please also add a comment saying:

// Use net::APP_CACHE to make size estimations deterministic via non-optimistic
// writes.

[msramek, 2015-10-19 08:53:01]

Thanks!

Ok then, given that I have some dependent CLs queued already, I'll land this
now. Gavin, in case you have any other comments, I'm happy to address them in a
follow-up CL.

https://codereview.chromium.org/1401923006/diff/210001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1401923006/diff/210001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:1764: SetCacheType(net::APP_CACHE);
On 2015/10/19 08:39:01, pasko wrote:
> please also add a comment saying:
> 
> // Use net::APP_CACHE to make size estimations deterministic via
non-optimistic
> // writes.

Done.

[msramek, 2015-10-19 08:53:12]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-10-19 08:53:13]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1401923006/#ps230001
(title: "Comment about APP_CACHE.")

[commit-bot: I haz the power, 2015-10-19 08:53:43]

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

[commit-bot: I haz the power, 2015-10-19 10:14:56]

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

[commit-bot: I haz the power, 2015-10-19 10:14:57]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[msramek, 2015-10-19 10:24:06]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2015-10-19 10:24:24]

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

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

Committed patchset #5 (id:230001)

[commit-bot: I haz the power, 2015-10-19 11:02:08]

Patchset 5 (id:??) landed as
https://crrev.com/88092426385ef40615a0cb47e7262f0b76848947
Cr-Commit-Position: refs/heads/master@{#354757}

[gavinp, 2015-10-19 14:33:23]

On 2015/10/19 11:02:08, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/88092426385ef40615a0cb47e7262f0b76848947
> Cr-Commit-Position: refs/heads/master@{#354757}

apologies for not speaking explicitly: I took the "TBR=" to mean my word wasn't
required; I broadly agree with what Egor was driving at, but he was doing a good
job so I didn't muddy the waters.

Thanks everyone!

[pasko, 2015-10-19 14:48:59]

On 2015/10/19 14:33:23, gavinp wrote:
> On 2015/10/19 11:02:08, commit-bot: I haz the power wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/88092426385ef40615a0cb47e7262f0b76848947
> > Cr-Commit-Position: refs/heads/master@{#354757}
> 
> apologies for not speaking explicitly: I took the "TBR=" to mean my word
wasn't
> required; I broadly agree with what Egor was driving at, but he was doing a
good
> job so I didn't muddy the waters.
> 
> Thanks everyone!

Yeah, I speculatively assumed that Gavin would agree to the change in
backend_unittest.cc, otherwise I would not have suggested a TBR. I am glad that
it worked as intended.