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());
 }
 
 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);

[pasko, 2013/05/17 10:31:33]

I am guessing the main purpose of this is to remove extra
PostponeWritingToDisk() and StartEvictionIfNeeded(). And also make less searches
through the small collection.

I think it is premature:
* all these operations are cheap in this case
* makes it harder to read

The abstraction that UpdateEntryIteratorSize presents is a bit subtle: it
updates both object state (cache_size_) and the entry pointed at, which is hard
to reflect in the function name. It leaves a few second thoughts on the iterator
nature in the different context: whether it is safe to update the value the
iterator points to.

One way to simplify it:
SimpleIndex::UpdateSizeForEntrySizeChange(const disk_cache::Entry& entry, uint64
entry_size)

and entry size modification would happen separately near the spot where we
created the iterator. But then it's only 2 lines and a DCHECK. Is it worth it?

[digit1, 2013/05/17 14:36:18]

Yes.
Unfortunately, due to the way our timers are implemented,
PostponeWritingToDisk() is costly. Everytime you call Start() or Reset() on a
timer, a new (internal/hidden) task gets posted on the message loop. Said task
will always execute, but may or may end up calling the timer's callback (the
time implementation ensures that only the last scheduled task will do that).

Since the current code calls Start() twice in a row, this results in at least
one completely un-necessary task every time. Each one of them takes time and
heap to setup / operate / disappear, and pollute the message loop for no good
reason. In other words, I think it's valuable to avoid the repetition here.

I agree that the lookup and StartEvictionIfNeeded() call are cheap though.

I wish our message loops implemented timers natively instead, but that's not
currently the way it works.
Probably, any suggestion welcome :)
I'll try to come up with something. I agree it'd be good to clarify this
behaviour.

[pasko, 2013/05/17 16:57:19]

Thanks! After this overview I am a little scared: there are tons of these timers
across the codebase.
Would it be a viable optimization to to make it a single PostTask? Messing up
with alarm() and signal handling scares me off as well, do we have a relevant
benchmark to prove the approach before actually making it working properly?
Probably not.. Sad face.
Thanks.

+    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)) !=
+             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 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);
   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_);

[pasko, 2013/05/17 10:31:33]

nice

+}
+
+void SimpleIndex::UpdateEntryIteratorSize(EntrySet::iterator* it,
+                                          uint64 entry_size) {
+  // 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);
 }
 
 // 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);

[pasko, 2013/05/17 10:31:33]

nice

     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