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

net/disk_cache/backend_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stdint.h>
 
 #include "base/basictypes.h"
 #include "base/files/file_util.h"
 #include "base/metrics/field_trial.h"
 #include "base/run_loop.h"
@@ skipping 71 matching lines @@
   // will be filled with times, used by DoomEntriesSince and DoomEntriesBetween.
   // There are 4 entries after doomed_start and 2 after doomed_end.
   void InitSparseCache(base::Time* doomed_start, base::Time* doomed_end);
 
   bool CreateSetOfRandomEntries(std::set<std::string>* key_pool);
   bool EnumerateAndMatchKeys(int max_to_open,
                              TestIterator* iter,
                              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();
   void BackendShutdownWithPendingFileIO(bool fast);
   void BackendShutdownWithPendingIO(bool fast);
   void BackendShutdownWithPendingCreate(bool fast);
   void BackendSetSize();
   void BackendLoad();
   void BackendChain();
   void BackendValidEntry();
@@ skipping 173 matching lines @@
     EXPECT_EQ(1U, keys_to_match->erase(entry->GetKey()));
     entry->Close();
     ++(*count);
     if (max_to_open >= 0 && static_cast<int>(*count) >= max_to_open)
       break;
   };
 
   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;
   EXPECT_NE(net::OK, OpenEntry("the first key", &entry1));
   ASSERT_EQ(net::OK, CreateEntry("the first key", &entry1));
   ASSERT_TRUE(NULL != entry1);
   entry1->Close();
   entry1 = NULL;
 
   ASSERT_EQ(net::OK, OpenEntry("the first key", &entry1));
@@ skipping 1377 matching lines @@
   start = end;
   end = base::Time::Now();
   DoomEntriesBetween(start, end);
   EXPECT_EQ(3, cache_->GetEntryCount());
 }
 
 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.
   std::set<std::string> key_pool;
   CreateSetOfRandomEntries(&key_pool);
 
   int count = 0;
   for (std::string key : key_pool) {
     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;
   }
 
   // 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.
   {
     const int last_entry_size = 47;
     std::string data(last_entry_size, ' ');
     scoped_refptr<net::StringIOBuffer> buffer = new net::StringIOBuffer(data);
 
     disk_cache::Entry* entry;
     std::string key = GenerateKey(true);
     ASSERT_EQ(net::OK, CreateEntry(key, &entry));
     ASSERT_EQ(last_entry_size,
               WriteData(entry, 0, 0, buffer.get(), last_entry_size, true));
     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) {
   BackendCalculateSizeOfAllEntries();
 }
 
 TEST_F(DiskCacheBackendTest, MemoryOnlyCalculateSizeOfAllEntries) {
   SetMemoryOnlyMode();
   BackendCalculateSizeOfAllEntries();
 }
@@ skipping 1852 matching lines @@
 // after closing.
 // NOTE: IF THIS TEST IS FLAKY THEN IT IS FAILING. See https://crbug.com/416940
 TEST_F(DiskCacheBackendTest, SimpleCacheDeleteQuickly) {
   SetSimpleCacheMode();
   for (int i = 0; i < 100; ++i) {
     InitCache();
     cache_.reset();
     EXPECT_TRUE(CleanupCacheDir());
   }
 }