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 147 matching lines @@
     : 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());

[gavinp, 2013/05/21 19:59:21]

Why not place this in the ctor list?

[digit1, 2013/05/30 13:17:58]

Done.

 }
 
 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 75 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_.find(simple_util::GetEntryHashKey(key)) !=

[gavinp, 2013/05/21 19:59:21]

 I don't think this is an improvement. count() is a lot easier to read than
find.

[digit1, 2013/05/30 13:17:58]

count() is actually slowed than find() != end() with STLport, but I agree the
difference should be minimal, so I've reverted this.

+             entries_set_.end();
 }
 
 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 44 matching lines @@
 
   scoped_ptr<int> result(new int());
   base::Closure task = base::Bind(&SimpleSynchronousEntry::DoomEntrySet,
                                   base::Passed(&entry_hashes),
                                   index_filename_.DirName(), result.get());
   base::Closure reply = base::Bind(&SimpleIndex::EvictionDone, AsWeakPtr(),
                                    base::Passed(&result));
   base::WorkerPool::PostTaskAndReply(FROM_HERE, task, reply, true);
 }
 
 bool SimpleIndex::UpdateEntrySize(const std::string& key, uint64 entry_size) {

[gavinp, 2013/05/21 19:59:21]

Is there any way to lose this?

[digit1, 2013/05/30 13:17:58]

The only place where it's called is  SimpleEntryImpl::SetSynchronousData() (and
a unit test). I'm not sure what this does so didn't try to touch it.

   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);
   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) {

[gavinp, 2013/05/21 19:59:21]

Please add a thread checker.

[digit1, 2013/05/30 13:17:58]

Done.

+  // Update the total cache size with the new entry size.
+  DCHECK(cache_size_ - (*it)->second.GetEntrySize() <= cache_size_);

[gavinp, 2013/05/21 19:59:21]

DCHECK_GE(cache_size, ...);

[digit1, 2013/05/30 13:17:58]

Done.

+  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");
   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