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

net/disk_cache/backend_unittest.cc

Index: net/disk_cache/backend_unittest.cc
diff --git a/net/disk_cache/backend_unittest.cc b/net/disk_cache/backend_unittest.cc
index 8833729f6965ce3a78f837a9e08d97f08105d3a5..6a27e24da7d1ea43d721a4f662ed6a320510addd 100644
--- a/net/disk_cache/backend_unittest.cc
+++ b/net/disk_cache/backend_unittest.cc
@@ -89,6 +89,10 @@ class DiskCacheBackendTest : public DiskCacheTestWithCache {
                              std::set<std::string>* keys_to_match,
                              size_t* count);
 
+  // Computes the expected size of entry metadata, i.e. the total size without
+  // the actual data stored. This depends only on the entry's |key| size.
+  int GetEntryMetadataSize(std::string key);
+
   // Actual tests:
   void BackendBasics();
   void BackendKeying();
@@ -282,6 +286,18 @@ bool DiskCacheBackendTest::EnumerateAndMatchKeys(
   return true;
 }
 
+int DiskCacheBackendTest::GetEntryMetadataSize(std::string key) {
+  // For blockfile and memory backends, it is just the key size.
+  if (!simple_cache_mode_)
+    return key.size();
+
+  // For the simple cache, we must add the file header and EOF, and that for
+  // every stream.
+  return disk_cache::kSimpleEntryStreamCount *
+         (sizeof(disk_cache::SimpleFileHeader) +
+          sizeof(disk_cache::SimpleFileEOF) + key.size());
+}
+
 void DiskCacheBackendTest::BackendBasics() {
   InitCache();
   disk_cache::Entry *entry1 = NULL, *entry2 = NULL;
@@ -1679,12 +1695,7 @@ void DiskCacheBackendTest::BackendCalculateSizeOfAllEntries() {
   InitCache();
 
   // The cache is initially empty.
-  if (memory_only_ || simple_cache_mode_) {
-    // TODO(msramek): Implement.
-    EXPECT_EQ(net::ERR_NOT_IMPLEMENTED, CalculateSizeOfAllEntries());
-  } else {
-    EXPECT_EQ(0, CalculateSizeOfAllEntries());
-  }
+  EXPECT_EQ(0, CalculateSizeOfAllEntries());
 
   // Generate random entries and populate them with data of respective
   // sizes 0, 1, ..., count - 1 bytes.
@@ -1696,11 +1707,15 @@ void DiskCacheBackendTest::BackendCalculateSizeOfAllEntries() {
     std::string data(count, ' ');
     scoped_refptr<net::StringIOBuffer> buffer = new net::StringIOBuffer(data);
 
-    // Alternate between writing to the first and second stream to test that
-    // we are not taking just the first stream into account.
+    // Write only to the stream 0. This is because writing to the stream 1

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

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]

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]

yay! Thanks a lot! I could not really find a rational explanation for the
previous theory.
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...
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.
good.
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]

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:15]

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]

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

+    // in simple cache triggers a write to the stream 0 as well. This will
+    // happen asynchronously, because |DiskCacheTestWithCache::WriteData|
+    // only waits for the callback from the first write operation, and possibly
+    // later than our call to |CalculateSizeOfAllEntries|.
+    // TODO(msramek): Investigate how to write synchronously to other streams.
     disk_cache::Entry* entry;
     ASSERT_EQ(net::OK, OpenEntry(key, &entry));
-    ASSERT_EQ(count, WriteData(entry, count % 2, 0, buffer.get(), count, true));
+    ASSERT_EQ(count, WriteData(entry, 0, 0, buffer.get(), count, true));
     entry->Close();
 
     ++count;
@@ -1708,16 +1723,10 @@ void DiskCacheBackendTest::BackendCalculateSizeOfAllEntries() {
 
   // The resulting size should be (0 + 1 + ... + count - 1) plus keys.
   int result = CalculateSizeOfAllEntries();
-  if (memory_only_ || simple_cache_mode_) {
-    // TODO(msramek): Implement.
-    EXPECT_EQ(net::ERR_NOT_IMPLEMENTED, result);
-  } else {
-    int total_key_size = 0;
-    for (std::string key : key_pool)
-      total_key_size += key.size();
-
-    EXPECT_EQ((count - 1) * count / 2 + total_key_size, result);
-  }
+  int total_metadata_size = 0;
+  for (std::string key : key_pool)
+    total_metadata_size += GetEntryMetadataSize(key);
+  EXPECT_EQ((count - 1) * count / 2 + total_metadata_size, result);
 
   // Add another entry and test if the size is updated. Then remove it and test
   // if the size is back to original value.
@@ -1734,32 +1743,16 @@ void DiskCacheBackendTest::BackendCalculateSizeOfAllEntries() {
     entry->Close();
 
     int new_result = CalculateSizeOfAllEntries();
-    if (memory_only_ || simple_cache_mode_) {
-      // TODO(msramek): Implement.
-      EXPECT_EQ(net::ERR_NOT_IMPLEMENTED, new_result);
-    } else {
-      EXPECT_EQ(result + last_entry_size + static_cast<int>(key.size()),
-                new_result);
-    }
+    EXPECT_EQ(result + last_entry_size + GetEntryMetadataSize(key), new_result);
 
     DoomEntry(key);
     new_result = CalculateSizeOfAllEntries();
-    if (memory_only_ || simple_cache_mode_) {
-      // TODO(msramek): Implement.
-      EXPECT_EQ(net::ERR_NOT_IMPLEMENTED, new_result);
-    } else {
-      EXPECT_EQ(result, new_result);
-    }
+    EXPECT_EQ(result, new_result);
   }
 
   // After dooming the entries, the size should be back to zero.
   ASSERT_EQ(net::OK, DoomAllEntries());
-  if (memory_only_ || simple_cache_mode_) {
-    // TODO(msramek): Implement.
-    EXPECT_EQ(net::ERR_NOT_IMPLEMENTED, CalculateSizeOfAllEntries());
-  } else {
-    EXPECT_EQ(0, CalculateSizeOfAllEntries());
-  }
+  EXPECT_EQ(0, CalculateSizeOfAllEntries());
 }
 
 TEST_F(DiskCacheBackendTest, CalculateSizeOfAllEntries) {