Minor SimpleCacheBackend improvements.

net/disk_cache/simple/simple_index.cc

 // Copyright (c) 2013 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 "net/disk_cache/simple/simple_index.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 146 matching lines @@
                          const base::FilePath& path)
     : cache_size_(0),
       max_size_(0),
       high_watermark_(0),
       low_watermark_(0),
       eviction_in_progress_(false),
       initialized_(false),
       index_filename_(path.AppendASCII("the-real-index")),
       cache_thread_(cache_thread),
       io_thread_(io_thread),
-      app_on_background_(false) {
-}
+      // Creating the callback once so it is reused every time
+      // write_to_disk_timer_.Start() is called.
+      write_to_disk_cb_(base::Bind(&SimpleIndex::WriteToDisk, AsWeakPtr())),
+      app_on_background_(false) {}
 
 SimpleIndex::~SimpleIndex() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
 
   // Fail all callbacks waiting for the index to come up.
   for (CallbackList::iterator it = to_run_when_initialized_.begin(),
        end = to_run_when_initialized_.end(); it != end; ++it) {
     it->Run(net::ERR_ABORTED);
   }
 }
@@ skipping 74 matching lines @@
   const uint64 hash_key = simple_util::GetEntryHashKey(key);
   InsertInEntrySet(EntryMetadata(hash_key, base::Time::Now(), 0),
                    &entries_set_);
   if (!initialized_)
     removed_entries_.erase(hash_key);
   PostponeWritingToDisk();
 }
 
 void SimpleIndex::Remove(const std::string& key) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
-  UpdateEntrySize(key, 0);
   const uint64 hash_key = simple_util::GetEntryHashKey(key);
-  entries_set_.erase(hash_key);
+  EntrySet::iterator it = entries_set_.find(hash_key);
+  if (it != entries_set_.end()) {
+    UpdateEntryIteratorSize(&it, 0);
+    entries_set_.erase(it);
+  }
 
   if (!initialized_)
     removed_entries_.insert(hash_key);
   PostponeWritingToDisk();
 }
 
 bool SimpleIndex::Has(const std::string& key) const {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   // If not initialized, always return true, forcing it to go to the disk.
   return !initialized_ ||
-      entries_set_.count(simple_util::GetEntryHashKey(key)) != 0;
+         entries_set_.count(simple_util::GetEntryHashKey(key)) > 0;
 }
 
 bool SimpleIndex::UseIfExists(const std::string& key) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   // Always update the last used time, even if it is during initialization.
   // It will be merged later.
   EntrySet::iterator it = entries_set_.find(simple_util::GetEntryHashKey(key));
   if (it == entries_set_.end())
     // If not initialized, always return true, forcing it to go to the disk.
     return !initialized_;
@@ skipping 50 matching lines @@
                                    base::Passed(&result));
   base::WorkerPool::PostTaskAndReply(FROM_HERE, task, reply, true);
 }
 
 bool SimpleIndex::UpdateEntrySize(const std::string& key, uint64 entry_size) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   EntrySet::iterator it = entries_set_.find(simple_util::GetEntryHashKey(key));
   if (it == entries_set_.end())
     return false;
 
-  // Update the total cache size with the new entry size.
-  DCHECK(cache_size_ - it->second.GetEntrySize() <= cache_size_);
-  cache_size_ -= it->second.GetEntrySize();
-  cache_size_ += entry_size;
-  it->second.SetEntrySize(entry_size);
+  UpdateEntryIteratorSize(&it, entry_size);

[pasko, 2013/05/30 13:47:10]

Somehow adding this function looks less awkward to me today. Naming suggestion:
UpdateEntrySize(). We are in the Index, so the name does not need to say that we
are updating the index. There is no need to say "Iterator" in the name because
we are passing the iterator in anyway.

Still I would prefer to do it->second.SetEntrySize(entry_size) outside this
function, with that the reader is not forced to think whether updating the
iterator is done in the function, and in which way.

   PostponeWritingToDisk();
   StartEvictionIfNeeded();
   return true;
 }
 
 void SimpleIndex::EvictionDone(scoped_ptr<int> result) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   DCHECK(result);
 
   // Ignore the result of eviction. We did our best.
@@ skipping 13 matching lines @@
       std::make_pair(entry_metadata.GetHashKey(), entry_metadata));
 }
 
 void SimpleIndex::PostponeWritingToDisk() {
   if (!initialized_)
     return;
   const int delay = app_on_background_ ? kWriteToDiskOnBackgroundDelayMSecs
                                        : kWriteToDiskDelayMSecs;
   // If the timer is already active, Start() will just Reset it, postponing it.
   write_to_disk_timer_.Start(
-      FROM_HERE,
-      base::TimeDelta::FromMilliseconds(delay),
-      base::Bind(&SimpleIndex::WriteToDisk, AsWeakPtr()));
+      FROM_HERE, base::TimeDelta::FromMilliseconds(delay), write_to_disk_cb_);
+}
+
+void SimpleIndex::UpdateEntryIteratorSize(EntrySet::iterator* it,
+                                          uint64 entry_size) {
+  // Update the total cache size with the new entry size.
+  DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK_GE(cache_size_, (*it)->second.GetEntrySize());
+  cache_size_ -= (*it)->second.GetEntrySize();
+  cache_size_ += entry_size;
+  (*it)->second.SetEntrySize(entry_size);
 }
 
 // static
 bool SimpleIndex::IsIndexFileStale(const base::FilePath& index_filename) {
   base::Time index_mtime;
   base::Time dir_mtime;
   if (!GetMTime(index_filename.DirName(), &dir_mtime))
     return true;
   if (!GetMTime(index_filename, &index_mtime))
     return true;
@@ skipping 72 matching lines @@
   using file_util::FileEnumerator;
   LOG(INFO) << "Simple Cache Index is being restored from disk.";
 
   file_util::Delete(index_filename, /* recursive = */ false);
   scoped_ptr<EntrySet> index_file_entries(new EntrySet());
 
   // TODO(felipeg,gavinp): Fix this once we have a one-file per entry format.
   COMPILE_ASSERT(kSimpleEntryFileCount == 3,
                  file_pattern_must_match_file_count);
 
-  const int kFileSuffixLenght = std::string("_0").size();
+  const int kFileSuffixLength = strlen("_0");

[gavinp, 2013/05/30 13:50:40]

As long as we're playing this game, you could make this compile time with
sizeof("_0") - 1, right?

   const base::FilePath::StringType file_pattern = FILE_PATH_LITERAL("*_[0-2]");
   FileEnumerator enumerator(index_filename.DirName(),
                             false /* recursive */,
                             FileEnumerator::FILES,
                             file_pattern);
   for (base::FilePath file_path = enumerator.Next(); !file_path.empty();
        file_path = enumerator.Next()) {
     const base::FilePath::StringType base_name = file_path.BaseName().value();
     // Converting to std::string is OK since we never use UTF8 wide chars in our
     // file names.
-    const std::string hash_name(base_name.begin(), base_name.end());
-    const std::string hash_key_string =
-        hash_name.substr(0, hash_name.size() - kFileSuffixLenght);
+    const std::string hash_key_string(base_name.begin(),
+                                      base_name.end() - kFileSuffixLength);
     uint64 hash_key = 0;
     if (!simple_util::GetEntryHashKeyFromHexString(
             hash_key_string, &hash_key)) {
       LOG(WARNING) << "Invalid Entry Hash Key filename while restoring "
-                   << "Simple Index from disk: " << hash_name;
+                   << "Simple Index from disk: " << base_name;
       // TODO(felipeg): Should we delete the invalid file here ?
       continue;
     }
 
     FileEnumerator::FindInfo find_info = {};
     enumerator.GetFindInfo(&find_info);
     base::Time last_used_time;
 #if defined(OS_POSIX)
     // For POSIX systems, a last access time is available. However, it's not
     // guaranteed to be more accurate than mtime. It is no worse though.
@@ skipping 117 matching lines @@
                                                          entries_set_);
   cache_thread_->PostTask(FROM_HERE, base::Bind(
       &SimpleIndex::WriteToDiskInternal,
       index_filename_,
       base::Passed(&pickle),
       start,
       app_on_background_));
 }
 
 }  // namespace disk_cache