SimpleCache: Avoid unnecessary index writes in APP_CACHE mode.

When a simple disk_cache is in APP_CACHE mode eviction is disabled and
access times do not need to be tracked.  This means that it should be
possible to perform read-only operations without triggering any index
disk writes.

This CL implements this optimization by making the following changes:

* Avoid updating access times in APP_CACHE mode.
* Avoid writing a clean index to disk when closing the backend.
* Avoid dirtying the index when a size update does not make a change.
* Avoid dirtying the index when opening an entry that is in the index.
* Avoid dirtying the index when dooming an entry not in the index.

With these changes its possible to use simple disk_cache to perform
read-only operations in cache_storage without any index writes.

Bug: 905351
Change-Id: I8eed0a72ed166e7b1b3623a7f4c17f7cec7f96d4
Reviewed-on: https://chromium-review.googlesource.com/c/1351539
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612934}

net/disk_cache/simple/simple_backend_impl.cc

@@ -259,7 +259,10 @@ SimpleBackendImpl::SimpleBackendImpl(
 }
 
 SimpleBackendImpl::~SimpleBackendImpl() {
-  index_->WriteToDisk(SimpleIndex::INDEX_WRITE_REASON_SHUTDOWN);
+  // Write the index out if there is a pending write from a
+  // previous operation.
+  if (index_->HasPendingWrite())
+    index_->WriteToDisk(SimpleIndex::INDEX_WRITE_REASON_SHUTDOWN);
 }
 
 void SimpleBackendImpl::SetWorkerPoolForTesting(

net/disk_cache/simple/simple_entry_impl.cc

@@ -352,6 +352,7 @@ std::string SimpleEntryImpl::GetKey() const {
 
 Time SimpleEntryImpl::GetLastUsed() const {
   DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(cache_type_ != net::APP_CACHE);
   return last_used_;
 }
 

net/disk_cache/simple/simple_entry_impl.h

@@ -107,6 +107,8 @@ class NET_EXPORT_PRIVATE SimpleEntryImpl : public Entry,
   void Doom() override;
   void Close() override;
   std::string GetKey() const override;
+  // GetLastUsed() should not be called in net::APP_CACHE mode since the times
+  // are not updated.
   base::Time GetLastUsed() const override;
   base::Time GetLastModified() const override;
   int32_t GetDataSize(int index) const override;

net/disk_cache/simple/simple_index.cc

@@ -222,6 +222,11 @@ std::unique_ptr<SimpleIndex::HashList> SimpleIndex::GetEntriesBetween(
     base::Time end_time) {
   DCHECK_EQ(true, initialized_);
 
+  // The net::APP_CACHE mode does not track access times.  Assert that external
+  // consumers are not relying on access time ranges.
+  DCHECK(cache_type_ != net::APP_CACHE ||
+         (initial_time.is_null() && end_time.is_null()));
+
   if (!initial_time.is_null())
     initial_time -= EntryMetadata::GetLowerEpsilonForTimeComparisons();
   if (end_time.is_null())
@@ -288,29 +293,38 @@ void SimpleIndex::SetLastUsedTimeForTest(uint64_t entry_hash,
   it->second.SetLastUsedTime(last_used);
 }
 
+bool SimpleIndex::HasPendingWrite() const {
+  return write_to_disk_timer_.IsRunning();
+}
+
 void SimpleIndex::Insert(uint64_t entry_hash) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   // Upon insert we don't know yet the size of the entry.
   // It will be updated later when the SimpleEntryImpl finishes opening or
   // creating the new entry, and then UpdateEntrySize will be called.
-  InsertInEntrySet(entry_hash, EntryMetadata(base::Time::Now(), 0u),
-                   &entries_set_);
+  bool inserted = InsertInEntrySet(
+      entry_hash, EntryMetadata(base::Time::Now(), 0u), &entries_set_);
   if (!initialized_)
     removed_entries_.erase(entry_hash);
-  PostponeWritingToDisk();
+  if (inserted)
+    PostponeWritingToDisk();
 }
 
 void SimpleIndex::Remove(uint64_t entry_hash) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
+  bool need_write = false;
   auto it = entries_set_.find(entry_hash);
   if (it != entries_set_.end()) {
     UpdateEntryIteratorSize(&it, 0u);
     entries_set_.erase(it);
+    need_write = true;
   }
 
   if (!initialized_)
     removed_entries_.insert(entry_hash);
-  PostponeWritingToDisk();
+
+  if (need_write)
+    PostponeWritingToDisk();
 }
 
 bool SimpleIndex::Has(uint64_t hash) const {
@@ -343,6 +357,9 @@ bool SimpleIndex::UseIfExists(uint64_t entry_hash) {
   if (it == entries_set_.end())
     // If not initialized, always return true, forcing it to go to the disk.
     return !initialized_;
+  // We do not need to track access times in APP_CACHE mode.
+  if (cache_type_ == net::APP_CACHE)
+    return true;
   it->second.SetLastUsedTime(base::Time::Now());
   PostponeWritingToDisk();
   return true;
@@ -412,7 +429,11 @@ bool SimpleIndex::UpdateEntrySize(uint64_t entry_hash,
   if (it == entries_set_.end())
     return false;
 
-  UpdateEntryIteratorSize(&it, entry_size);
+  // Update the entry size.  If there was no change, then there is nothing
+  // else to do here.
+  if (!UpdateEntryIteratorSize(&it, entry_size))
+    return true;
+
   PostponeWritingToDisk();
   StartEvictionIfNeeded();
   return true;
@@ -433,19 +454,20 @@ void SimpleIndex::EvictionDone(int result) {
 }
 
 // static
-void SimpleIndex::InsertInEntrySet(
+bool SimpleIndex::InsertInEntrySet(
     uint64_t entry_hash,
     const disk_cache::EntryMetadata& entry_metadata,
     EntrySet* entry_set) {
   DCHECK(entry_set);
-  entry_set->insert(std::make_pair(entry_hash, entry_metadata));
+  auto result = entry_set->insert(std::make_pair(entry_hash, entry_metadata));
+  return result.second;
 }
 
 void SimpleIndex::InsertEntryForTesting(uint64_t entry_hash,
                                         const EntryMetadata& entry_metadata) {
   DCHECK(entries_set_.find(entry_hash) == entries_set_.end());
-  InsertInEntrySet(entry_hash, entry_metadata, &entries_set_);
-  cache_size_ += entry_metadata.GetEntrySize();
+  if (InsertInEntrySet(entry_hash, entry_metadata, &entries_set_))
+    cache_size_ += entry_metadata.GetEntrySize();
 }
 
 void SimpleIndex::PostponeWritingToDisk() {
@@ -458,16 +480,20 @@ void SimpleIndex::PostponeWritingToDisk() {
       FROM_HERE, base::TimeDelta::FromMilliseconds(delay), write_to_disk_cb_);
 }
 
-void SimpleIndex::UpdateEntryIteratorSize(
+bool SimpleIndex::UpdateEntryIteratorSize(
     EntrySet::iterator* it,
     base::StrictNumeric<uint32_t> entry_size) {
   // Update the total cache size with the new entry size.
   DCHECK(io_thread_checker_.CalledOnValidThread());
   DCHECK_GE(cache_size_, (*it)->second.GetEntrySize());
+  uint32_t original_size = (*it)->second.GetEntrySize();
   cache_size_ -= (*it)->second.GetEntrySize();
   (*it)->second.SetEntrySize(entry_size);
   // We use GetEntrySize to get consistent rounding.
   cache_size_ += (*it)->second.GetEntrySize();
+  // Return true if the size of the entry actually changed.  Make sure to
+  // compare the rounded values provided by GetEntrySize().
+  return original_size != (*it)->second.GetEntrySize();
 }
 
 void SimpleIndex::MergeInitializingSet(
@@ -553,6 +579,10 @@ void SimpleIndex::WriteToDisk(IndexWriteToDiskReason reason) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   if (!initialized_)
     return;
+
+  // Cancel any pending writes since we are about to write to disk now.
+  write_to_disk_timer_.AbandonAndStop();
+
   SIMPLE_CACHE_UMA(CUSTOM_COUNTS,
                    "IndexNumEntriesOnWrite", cache_type_,
                    entries_set_.size(), 0, 100000, 50);

net/disk_cache/simple/simple_index.h

@@ -151,7 +151,9 @@ class NET_EXPORT_PRIVATE SimpleIndex
 
   using EntrySet = std::unordered_map<uint64_t, EntryMetadata>;
 
-  static void InsertInEntrySet(uint64_t entry_hash,
+  // Insert an entry in the given set if there is not already entry present.
+  // Returns true if the set was modified.
+  static bool InsertInEntrySet(uint64_t entry_hash,
                                const EntryMetadata& entry_metadata,
                                EntrySet* entry_set);
 
@@ -167,6 +169,10 @@ class NET_EXPORT_PRIVATE SimpleIndex
   // range between |initial_time| and |end_time| where open intervals are
   // possible according to the definition given in |DoomEntriesBetween()| in the
   // disk cache backend interface.
+  //
+  // Access times are not updated in net::APP_CACHE mode.  GetEntriesBetween()
+  // should only be called with null times indicating the full range when in
+  // this mode.
   std::unique_ptr<HashList> GetEntriesBetween(const base::Time initial_time,
                                               const base::Time end_time);
 
@@ -203,19 +209,26 @@ class NET_EXPORT_PRIVATE SimpleIndex
   }
 #endif
 
+  // Return true if a pending disk write has been scheduled from
+  // PostponeWritingToDisk().
+  bool HasPendingWrite() const;
+
  private:
   friend class SimpleIndexTest;
   FRIEND_TEST_ALL_PREFIXES(SimpleIndexTest, IndexSizeCorrectOnMerge);
   FRIEND_TEST_ALL_PREFIXES(SimpleIndexTest, DiskWriteQueued);
   FRIEND_TEST_ALL_PREFIXES(SimpleIndexTest, DiskWriteExecuted);
   FRIEND_TEST_ALL_PREFIXES(SimpleIndexTest, DiskWritePostponed);
+  FRIEND_TEST_ALL_PREFIXES(SimpleIndexAppCacheTest, DiskWriteQueued);
 
   void StartEvictionIfNeeded();
   void EvictionDone(int result);
 
   void PostponeWritingToDisk();
 
-  void UpdateEntryIteratorSize(EntrySet::iterator* it,
+  // Update the size of the entry pointed to by the given iterator.  Return
+  // true if the new size actually results in a change.
+  bool UpdateEntryIteratorSize(EntrySet::iterator* it,
                                base::StrictNumeric<uint32_t> entry_size);
 
   // Must run on IO Thread.

net/disk_cache/simple/simple_index_unittest.cc

@@ -66,8 +66,8 @@ class EntryMetadataTest : public testing::Test {
 class MockSimpleIndexFile : public SimpleIndexFile,
                             public base::SupportsWeakPtr<MockSimpleIndexFile> {
  public:
-  MockSimpleIndexFile()
-      : SimpleIndexFile(NULL, NULL, net::DISK_CACHE, base::FilePath()),
+  explicit MockSimpleIndexFile(net::CacheType cache_type)
+      : SimpleIndexFile(NULL, NULL, cache_type, base::FilePath()),
         load_result_(NULL),
         load_index_entries_calls_(0),
         disk_writes_(0) {}
@@ -120,11 +120,12 @@ class SimpleIndexTest : public net::TestWithScopedTaskEnvironment,
   }
 
   void SetUp() override {
-    std::unique_ptr<MockSimpleIndexFile> index_file(new MockSimpleIndexFile());
+    std::unique_ptr<MockSimpleIndexFile> index_file(
+        new MockSimpleIndexFile(CacheType()));
     index_file_ = index_file->AsWeakPtr();
     index_.reset(new SimpleIndex(/* io_thread = */ nullptr,
                                  /* cleanup_tracker = */ nullptr, this,
-                                 net::DISK_CACHE, std::move(index_file)));
+                                 CacheType(), std::move(index_file)));
 
     index_->Initialize(base::Time());
   }
@@ -177,6 +178,8 @@ class SimpleIndexTest : public net::TestWithScopedTaskEnvironment,
   }
   int doom_entries_calls() const { return doom_entries_calls_; }
 
+  virtual net::CacheType CacheType() const { return net::DISK_CACHE; }
+
   const simple_util::ImmutableArray<uint64_t, 16> hashes_;
   std::unique_ptr<SimpleIndex> index_;
   base::WeakPtr<MockSimpleIndexFile> index_file_;
@@ -188,6 +191,11 @@ class SimpleIndexTest : public net::TestWithScopedTaskEnvironment,
   int doom_entries_calls_;
 };
 
+class SimpleIndexAppCacheTest : public SimpleIndexTest {
+ protected:
+  net::CacheType CacheType() const override { return net::APP_CACHE; }
+};
+
 TEST_F(EntryMetadataTest, Basics) {
   EntryMetadata entry_metadata;
   EXPECT_EQ(base::Time(), entry_metadata.GetLastUsedTime());
@@ -704,37 +712,50 @@ TEST_F(SimpleIndexTest, DiskWriteQueued) {
   index()->SetMaxSize(1000);
   ReturnIndexFile();
 
-  EXPECT_FALSE(index()->write_to_disk_timer_.IsRunning());
+  EXPECT_FALSE(index()->HasPendingWrite());
 
   const uint64_t kHash1 = hashes_.at<1>();
   index()->Insert(kHash1);
-  EXPECT_TRUE(index()->write_to_disk_timer_.IsRunning());
+  EXPECT_TRUE(index()->HasPendingWrite());
   index()->write_to_disk_timer_.Stop();
-  EXPECT_FALSE(index()->write_to_disk_timer_.IsRunning());
+  EXPECT_FALSE(index()->HasPendingWrite());
+
+  // Attempting to insert a hash that already exists should not queue the
+  // write timer.
+  index()->Insert(kHash1);
+  EXPECT_FALSE(index()->HasPendingWrite());
 
   index()->UseIfExists(kHash1);
-  EXPECT_TRUE(index()->write_to_disk_timer_.IsRunning());
+  EXPECT_TRUE(index()->HasPendingWrite());
   index()->write_to_disk_timer_.Stop();
 
   index()->UpdateEntrySize(kHash1, 20u);
-  EXPECT_TRUE(index()->write_to_disk_timer_.IsRunning());
+  EXPECT_TRUE(index()->HasPendingWrite());
   index()->write_to_disk_timer_.Stop();
 
+  // Updating to the same size should not queue the write timer.
+  index()->UpdateEntrySize(kHash1, 20u);
+  EXPECT_FALSE(index()->HasPendingWrite());
+
   index()->Remove(kHash1);
-  EXPECT_TRUE(index()->write_to_disk_timer_.IsRunning());
+  EXPECT_TRUE(index()->HasPendingWrite());
   index()->write_to_disk_timer_.Stop();
+
+  // Removing a non-existent hash should not queue the write timer.
+  index()->Remove(kHash1);
+  EXPECT_FALSE(index()->HasPendingWrite());
 }
 
 TEST_F(SimpleIndexTest, DiskWriteExecuted) {
   index()->SetMaxSize(1000);
   ReturnIndexFile();
 
-  EXPECT_FALSE(index()->write_to_disk_timer_.IsRunning());
+  EXPECT_FALSE(index()->HasPendingWrite());
 
   const uint64_t kHash1 = hashes_.at<1>();
   index()->Insert(kHash1);
   index()->UpdateEntrySize(kHash1, 20u);
-  EXPECT_TRUE(index()->write_to_disk_timer_.IsRunning());
+  EXPECT_TRUE(index()->HasPendingWrite());
 
   EXPECT_EQ(0, index_file_->disk_writes());
   index()->write_to_disk_timer_.FireNow();
@@ -756,11 +777,11 @@ TEST_F(SimpleIndexTest, DiskWritePostponed) {
   index()->SetMaxSize(1000);
   ReturnIndexFile();
 
-  EXPECT_FALSE(index()->write_to_disk_timer_.IsRunning());
+  EXPECT_FALSE(index()->HasPendingWrite());
 
   index()->Insert(hashes_.at<1>());
   index()->UpdateEntrySize(hashes_.at<1>(), 20u);
-  EXPECT_TRUE(index()->write_to_disk_timer_.IsRunning());
+  EXPECT_TRUE(index()->HasPendingWrite());
   base::TimeTicks expected_trigger(
       index()->write_to_disk_timer_.desired_run_time());
 
@@ -768,9 +789,50 @@ TEST_F(SimpleIndexTest, DiskWritePostponed) {
   EXPECT_EQ(expected_trigger, index()->write_to_disk_timer_.desired_run_time());
   index()->Insert(hashes_.at<2>());
   index()->UpdateEntrySize(hashes_.at<2>(), 40u);
-  EXPECT_TRUE(index()->write_to_disk_timer_.IsRunning());
+  EXPECT_TRUE(index()->HasPendingWrite());
   EXPECT_LT(expected_trigger, index()->write_to_disk_timer_.desired_run_time());
   index()->write_to_disk_timer_.Stop();
 }
 
+// net::APP_CACHE mode should not need to queue disk writes in as many places
+// as the default net::DISK_CACHE mode.
+TEST_F(SimpleIndexAppCacheTest, DiskWriteQueued) {
+  index()->SetMaxSize(1000);
+  ReturnIndexFile();
+
+  EXPECT_FALSE(index()->HasPendingWrite());
+
+  const uint64_t kHash1 = hashes_.at<1>();
+  index()->Insert(kHash1);
+  EXPECT_TRUE(index()->HasPendingWrite());
+  index()->write_to_disk_timer_.Stop();
+  EXPECT_FALSE(index()->HasPendingWrite());
+
+  // Attempting to insert a hash that already exists should not queue the
+  // write timer.
+  index()->Insert(kHash1);
+  EXPECT_FALSE(index()->HasPendingWrite());
+
+  // Since net::APP_CACHE does not evict or track access times using an
+  // entry should not queue the write timer.
+  index()->UseIfExists(kHash1);
+  EXPECT_FALSE(index()->HasPendingWrite());
+
+  index()->UpdateEntrySize(kHash1, 20u);
+  EXPECT_TRUE(index()->HasPendingWrite());
+  index()->write_to_disk_timer_.Stop();
+
+  // Updating to the same size should not queue the write timer.
+  index()->UpdateEntrySize(kHash1, 20u);
+  EXPECT_FALSE(index()->HasPendingWrite());
+
+  index()->Remove(kHash1);
+  EXPECT_TRUE(index()->HasPendingWrite());
+  index()->write_to_disk_timer_.Stop();
+
+  // Removing a non-existent hash should not queue the write timer.
+  index()->Remove(kHash1);
+  EXPECT_FALSE(index()->HasPendingWrite());
+}
+
 }  // namespace disk_cache