Refactor our SimpleIndex file format and serialization to use Pickle instead of the previously bugg…

net/disk_cache/simple/simple_index_file.cc

Index: net/disk_cache/simple/simple_index_file.cc
diff --git a/net/disk_cache/simple/simple_index.cc b/net/disk_cache/simple/simple_index_file.cc
similarity index 24%
copy from net/disk_cache/simple/simple_index.cc
copy to net/disk_cache/simple/simple_index_file.cc
index 2a915bec6425ed80a78a94681c40f1436a390732..05b85f3e0a04420b03084d42156568393d1374a5 100644
--- a/net/disk_cache/simple/simple_index.cc
+++ b/net/disk_cache/simple/simple_index_file.cc
@@ -2,223 +2,126 @@
 // 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 "net/disk_cache/simple/simple_index_file.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/file_util.h"
+#include "base/hash.h"
+#include "base/logging.h"
 #include "base/message_loop.h"
+#include "base/pickle.h"
 #include "base/task_runner.h"
-#include "base/threading/worker_pool.h"
-#include "net/base/io_buffer.h"
-#include "net/base/net_errors.h"
 #include "net/disk_cache/simple/simple_disk_format.h"
+#include "net/disk_cache/simple/simple_index_util.h"
 #include "third_party/zlib/zlib.h"
 
 namespace {
 
 const uint64 kMaxEntiresInIndex = 100000000;
 
-bool CheckHeader(disk_cache::SimpleIndexFile::Header header) {
-  return header.number_of_entries <= kMaxEntiresInIndex &&
-      header.initial_magic_number ==
-      disk_cache::kSimpleIndexInitialMagicNumber &&
-      header.version == disk_cache::kSimpleVersion;
-}
-
-class FileAutoCloser {
- public:
-  explicit FileAutoCloser(const base::PlatformFile& file) : file_(file) { }
-  ~FileAutoCloser() {
-    base::ClosePlatformFile(file_);
-  }
- private:
-  base::PlatformFile file_;
-  DISALLOW_COPY_AND_ASSIGN(FileAutoCloser);
-};
-
 }  // namespace
 
 namespace disk_cache {
 
-SimpleIndex::SimpleIndex(
-    const scoped_refptr<base::TaskRunner>& cache_thread,
-    const scoped_refptr<base::TaskRunner>& io_thread,
-    const base::FilePath& path)
-    : cache_size_(0),
-      initialized_(false),
-      index_filename_(path.AppendASCII("simple-index")),
-      cache_thread_(cache_thread),
-      io_thread_(io_thread) {}
-
-SimpleIndex::~SimpleIndex() {
-  DCHECK(io_thread_checker_.CalledOnValidThread());
+SimpleIndexFile::IndexMetadata::IndexMetadata() :
+    initial_magic_number_(kSimpleIndexInitialMagicNumber),
+    version_(kSimpleVersion),
+    number_of_entries_(0),
+    cache_size_(0) {}
+
+SimpleIndexFile::IndexMetadata::IndexMetadata(
+    uint64 number_of_entries, uint64 cache_size) :
+    initial_magic_number_(kSimpleIndexInitialMagicNumber),
+    version_(kSimpleVersion),
+    number_of_entries_(number_of_entries),
+    cache_size_(cache_size) {}
+
+void SimpleIndexFile::IndexMetadata::Serialize(Pickle* pickle) const {
+  DCHECK(pickle);
+  pickle->WriteUInt64(initial_magic_number_);
+  pickle->WriteUInt32(version_);
+  pickle->WriteUInt64(number_of_entries_);
+  pickle->WriteUInt64(cache_size_);
+}
 
+bool SimpleIndexFile::IndexMetadata::DeSerialize(PickleIterator* it,
+                                                 IndexMetadata* out) {
+  DCHECK(it);
+  DCHECK(out);
+  return it->ReadUInt64(&(out->initial_magic_number_)) &&
+      it->ReadUInt32(&(out->version_)) &&
+      it->ReadUInt64(&(out->number_of_entries_))&&
+      it->ReadUInt64(&(out->cache_size_));
 }
 
-void SimpleIndex::Initialize() {
-  DCHECK(io_thread_checker_.CalledOnValidThread());
-  MergeCallback merge_callback = base::Bind(&SimpleIndex::MergeInitializingSet,
-                                            this->AsWeakPtr());
-  base::WorkerPool::PostTask(FROM_HERE,
-                             base::Bind(&SimpleIndex::LoadFromDisk,
-                                        index_filename_,
-                                        io_thread_,
-                                        merge_callback),
-                             true);
+bool SimpleIndexFile::IndexMetadata::CheckIndexMetadata() {
+  return number_of_entries_ <= kMaxEntiresInIndex &&
+      initial_magic_number_ == disk_cache::kSimpleIndexInitialMagicNumber &&
+      version_ == disk_cache::kSimpleVersion;
 }
 
 // static
-void SimpleIndex::LoadFromDisk(
+void SimpleIndexFile::LoadFromDisk(
     const base::FilePath& index_filename,
-    const scoped_refptr<base::TaskRunner>& io_thread,
-    const MergeCallback& merge_callback) {
-  // Open the index file.
-  base::PlatformFileError error;
-  base::PlatformFile index_file = base::CreatePlatformFile(
-      index_filename,
-      base::PLATFORM_FILE_OPEN_ALWAYS |
-      base::PLATFORM_FILE_READ |
-      base::PLATFORM_FILE_WRITE,
-      NULL,
-      &error);
-  FileAutoCloser auto_close_index_file(index_file);
-  if (error != base::PLATFORM_FILE_OK) {
-    LOG(ERROR) << "Error opening file " << index_filename.value();
-    return RestoreFromDisk(index_filename, io_thread, merge_callback);
+    const scoped_refptr<base::TaskRunner>& callback_runner,
+    const IndexCompletionCallback& completion_callback) {
+  std::string contents;
+  if(!file_util::ReadFileToString(index_filename,
+                                  &contents)) {
+    LOG(WARNING) << "Could not read Simple Index file.";
+    return RestoreFromDisk(
+        index_filename, callback_runner, completion_callback);
   }
 
-  uLong incremental_crc = crc32(0L, Z_NULL, 0);
-  int64 index_file_offset = 0;
-  SimpleIndexFile::Header header;
-  if (base::ReadPlatformFile(index_file,
-                             index_file_offset,
-                             reinterpret_cast<char*>(&header),
-                             sizeof(header)) != sizeof(header)) {
-    return RestoreFromDisk(index_filename, io_thread, merge_callback);
-  }
-  index_file_offset += sizeof(header);
-  incremental_crc = crc32(incremental_crc,
-                          reinterpret_cast<const Bytef*>(&header),
-                          implicit_cast<uInt>(sizeof(header)));
+  Pickle pickle (contents.data(), contents.size());
+  PickleIterator pickle_it(pickle);
 
-  if (!CheckHeader(header)) {
-    LOG(ERROR) << "Invalid header on Simple Cache Index.";
-    return RestoreFromDisk(index_filename, io_thread, merge_callback);
-  }
+  SimpleIndexFile::PickleHeader* header_p =
+      pickle.headerT<SimpleIndexFile::PickleHeader>();
+  const uint32 crc_read = header_p->crc;
+  const uint32 crc_calculated = CalculatePickleCRC(pickle);
 
-  const int entries_buffer_size =
-      header.number_of_entries * SimpleIndexFile::kEntryMetadataSize;
+  if (crc_read != crc_calculated) {
+    LOG(WARNING) << "Invalid CRC in Simple Index file.";
+    return RestoreFromDisk(
+        index_filename, callback_runner, completion_callback);
+  }
 
-  scoped_ptr<char[]> entries_buffer(new char[entries_buffer_size]);
-  if (base::ReadPlatformFile(index_file,
-                             index_file_offset,
-                             entries_buffer.get(),
-                             entries_buffer_size) != entries_buffer_size) {
-    return RestoreFromDisk(index_filename, io_thread, merge_callback);
+  SimpleIndexFile::IndexMetadata index_metadata;
+  if (!SimpleIndexFile::IndexMetadata::DeSerialize(
+          &pickle_it, &index_metadata)) {
+    LOG(ERROR) << "Invalid index_metadata on Simple Cache Index.";
+    return RestoreFromDisk(
+        index_filename, callback_runner, completion_callback);
   }
-  index_file_offset += entries_buffer_size;
-  incremental_crc = crc32(incremental_crc,
-                          reinterpret_cast<const Bytef*>(entries_buffer.get()),
-                          implicit_cast<uInt>(entries_buffer_size));
 
-  SimpleIndexFile::Footer footer;
-  if (base::ReadPlatformFile(index_file,
-                             index_file_offset,
-                             reinterpret_cast<char*>(&footer),
-                             sizeof(footer)) != sizeof(footer)) {
-    return RestoreFromDisk(index_filename, io_thread, merge_callback);
+  if (!index_metadata.CheckIndexMetadata()) {
+    LOG(ERROR) << "Invalid index_metadata on Simple Cache Index.";
+    return RestoreFromDisk(
+        index_filename, callback_runner, completion_callback);
   }
-  const uint32 crc_read = footer.crc;
-  const uint32 crc_calculated = incremental_crc;
-  if (crc_read != crc_calculated)
-    return RestoreFromDisk(index_filename, io_thread, merge_callback);
 
   scoped_ptr<EntrySet> index_file_entries(new EntrySet());
-  int entries_buffer_offset = 0;
-  while(entries_buffer_offset < entries_buffer_size) {
-    SimpleIndexFile::EntryMetadata entry_metadata;
-    SimpleIndexFile::EntryMetadata::DeSerialize(
-        &entries_buffer.get()[entries_buffer_offset], &entry_metadata);
-    InsertInternal(index_file_entries.get(), entry_metadata);
-    entries_buffer_offset += SimpleIndexFile::kEntryMetadataSize;
+  while (index_file_entries->size() < index_metadata.GetNumberOfEntries()) {
+    EntryMetadata entry_metadata;
+    if (!EntryMetadata::DeSerialize(&pickle_it, &entry_metadata)) {
+      LOG(WARNING) << "Invalid EntryMetadata in Simple Index file.";
+      return RestoreFromDisk(
+          index_filename, callback_runner, completion_callback);
+    }
+    InsertInEntrySet(index_file_entries.get(), entry_metadata);
   }
-  DCHECK_EQ(header.number_of_entries, index_file_entries->size());
-
-  io_thread->PostTask(FROM_HERE,
-                      base::Bind(merge_callback,
-                                 base::Passed(&index_file_entries)));
-}
-
-void SimpleIndex::Insert(const std::string& key) {
-  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.
-  const uint64 hash_key = GetEntryHashKey(key);
-  InsertInternal(&entries_set_, SimpleIndexFile::EntryMetadata(
-      hash_key,
-      base::Time::Now(), 0));
-  if (!initialized_)
-    removed_entries_.erase(hash_key);
-}
-
-void SimpleIndex::Remove(const std::string& key) {
-  DCHECK(io_thread_checker_.CalledOnValidThread());
-  UpdateEntrySize(key, 0);
-  const uint64 hash_key = GetEntryHashKey(key);
-  entries_set_.erase(hash_key);
-
-  if (!initialized_)
-    removed_entries_.insert(hash_key);
-}
-
-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(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(GetEntryHashKey(key));
-  if (it == entries_set_.end())
-    // If not initialized, always return true, forcing it to go to the disk.
-    return !initialized_;
-  it->second.SetLastUsedTime(base::Time::Now());
-  return true;
-}
-
-bool SimpleIndex::UpdateEntrySize(const std::string& key, uint64 entry_size) {
-  DCHECK(io_thread_checker_.CalledOnValidThread());
-  EntrySet::iterator it = entries_set_.find(GetEntryHashKey(key));
-  if (it == entries_set_.end())
-    return false;
-
-  // Update the total cache size with the new entry size.
-  cache_size_ -= it->second.entry_size;
-  cache_size_ += entry_size;
-  it->second.entry_size = entry_size;
-
-  return true;
+  callback_runner->PostTask(FROM_HERE,
+                            base::Bind(completion_callback,
+                                       base::Passed(&index_file_entries)));
 }
 
 // static
-void SimpleIndex::InsertInternal(
-    EntrySet* entry_set,
-    const SimpleIndexFile::EntryMetadata& entry_metadata) {
-  // TODO(felipeg): Use a hash_set instead of a hash_map.
-  DCHECK(entry_set);
-  entry_set->insert(
-      std::make_pair(entry_metadata.GetHashKey(), entry_metadata));
-}
-
-// static
-void SimpleIndex::RestoreFromDisk(
+void SimpleIndexFile::RestoreFromDisk(
     const base::FilePath& index_filename,
-    const scoped_refptr<base::TaskRunner>& io_thread,
-    const MergeCallback& merge_callback) {
+    const scoped_refptr<base::TaskRunner>& callback_runner,
+    const IndexCompletionCallback& completion_callback) {
   using file_util::FileEnumerator;
   LOG(INFO) << "Simple Cache Index is being restored from disk.";
 
@@ -263,103 +166,49 @@ void SimpleIndex::RestoreFromDisk(
     int64 file_size = FileEnumerator::GetFilesize(find_info);
     EntrySet::iterator it = index_file_entries->find(hash_key);

[Philippe, 2013/04/15 13:57:57]

See my comment in a previous header about the lookup + insert anti-pattern.

[felipeg, 2013/04/15 14:39:07]

I think this is an early optimization.
The code is more clear this way.

Once we change it to hash_set, I will revisit this decision.

     if (it == index_file_entries->end()) {
-      InsertInternal(index_file_entries.get(), SimpleIndexFile::EntryMetadata(
+      InsertInEntrySet(index_file_entries.get(), EntryMetadata(
           hash_key, last_used_time, file_size));
     } else {
       // Summing up the total size of the entry through all the *_[0-2] files
-      it->second.entry_size += file_size;
-    }
-  }
-
-  io_thread->PostTask(FROM_HERE,
-                      base::Bind(merge_callback,
-                                 base::Passed(&index_file_entries)));
-}
-
-void SimpleIndex::MergeInitializingSet(
-    scoped_ptr<EntrySet> index_file_entries) {
-  DCHECK(io_thread_checker_.CalledOnValidThread());
-  // First, remove the entries that are in the |removed_entries_| from both
-  // sets.
-  for (base::hash_set<uint64>::const_iterator it =
-           removed_entries_.begin(); it != removed_entries_.end(); ++it) {
-    entries_set_.erase(*it);
-    index_file_entries->erase(*it);
-  }
-
-  // Recalculate the cache size while merging the two sets.
-  cache_size_ = 0;
-  for (EntrySet::const_iterator it = index_file_entries->begin();
-       it != index_file_entries->end(); ++it) {
-    // If there is already an entry in the current entries_set_, we need to
-    // merge the new data there with the data loaded in the initialization.
-    EntrySet::iterator current_entry = entries_set_.find(it->first);
-    if (current_entry != entries_set_.end()) {
-      // When Merging, existing valid data in the |current_entry| will prevail.
-      SimpleIndexFile::EntryMetadata::Merge(
-          it->second, &(current_entry->second));
-      cache_size_ += current_entry->second.entry_size;
-    } else {
-      InsertInternal(&entries_set_, it->second);
-      cache_size_ += it->second.entry_size;
+      it->second.SetEntrySize(it->second.GetEntrySize() + file_size);
     }
   }
 
-  initialized_ = true;
+  callback_runner->PostTask(FROM_HERE,
+                            base::Bind(completion_callback,
+                                       base::Passed(&index_file_entries)));
 }
 
-void SimpleIndex::Serialize(std::string* out_buffer) {
-  DCHECK(io_thread_checker_.CalledOnValidThread());
-  DCHECK(out_buffer);
-  SimpleIndexFile::Header header;
-  SimpleIndexFile::Footer footer;
-
-  header.initial_magic_number = kSimpleIndexInitialMagicNumber;
-  header.version = kSimpleVersion;
-  header.number_of_entries = entries_set_.size();
-
-  out_buffer->reserve(
-      sizeof(header) +
-      sizeof(SimpleIndexFile::EntryMetadata) * entries_set_.size() +
-      sizeof(footer));
-
-  // The Header goes first.
-  out_buffer->append(reinterpret_cast<const char*>(&header),
-                     sizeof(header));
-
-  // Then all the entries from |entries_set_|.
-  for (EntrySet::const_iterator it = entries_set_.begin();
-       it != entries_set_.end(); ++it) {
-    SimpleIndexFile::EntryMetadata::Serialize(it->second, out_buffer);
+// static
+scoped_ptr<Pickle> SimpleIndexFile::Serialize(
+    const SimpleIndexFile::IndexMetadata& index_metadata,
+    const EntrySet& entries) {
+  scoped_ptr<Pickle> pickle(new Pickle(sizeof(SimpleIndexFile::PickleHeader)));
+
+  // Serialize the |index_metadata|.
+  index_metadata.Serialize(pickle.get());
+  // Then all the entries from |entries| set.
+  for (EntrySet::const_iterator it = entries.begin();
+       it != entries.end(); ++it) {
+    it->second.Serialize(pickle.get());
   }
-
-  // Then, CRC.
-  footer.crc = crc32(crc32(0, Z_NULL, 0),
-                     reinterpret_cast<const Bytef*>(out_buffer->data()),
-                     implicit_cast<uInt>(out_buffer->size()));
-
-  out_buffer->append(reinterpret_cast<const char*>(&footer), sizeof(footer));
-}
-
-void SimpleIndex::WriteToDisk() {
-  DCHECK(io_thread_checker_.CalledOnValidThread());
-  scoped_ptr<std::string> buffer(new std::string());
-  Serialize(buffer.get());
-  cache_thread_->PostTask(FROM_HERE, base::Bind(
-      &SimpleIndex::UpdateFile,
-      index_filename_,
-      index_filename_.DirName().AppendASCII("index_temp"),
-      base::Passed(&buffer)));
+  SimpleIndexFile::PickleHeader* header_p =
+      pickle->headerT<SimpleIndexFile::PickleHeader>();
+  header_p->crc = CalculatePickleCRC(*pickle);
+  return pickle.Pass();
 }
 
 // static
-void SimpleIndex::UpdateFile(const base::FilePath& index_filename,
-                             const base::FilePath& temp_filename,
-                             scoped_ptr<std::string> buffer) {
+void SimpleIndexFile::WriteToDisk(const base::FilePath& index_filename,
+                                  scoped_ptr<Pickle> pickle) {
+  const base::FilePath temp_filename =
+      index_filename.DirName().AppendASCII("index_temp");
   int bytes_written = file_util::WriteFile(
-      temp_filename, buffer->data(), buffer->size());
-  DCHECK_EQ(bytes_written, implicit_cast<int>(buffer->size()));
-  if (bytes_written != static_cast<int>(buffer->size())) {
+      temp_filename,
+      reinterpret_cast<const char*>(pickle->data()),
+      pickle->size());
+  DCHECK_EQ(bytes_written, implicit_cast<int>(pickle->size()));
+  if (bytes_written != static_cast<int>(pickle->size())) {
     // TODO(felipeg): Add better error handling.
     LOG(ERROR) << "Could not write Simple Cache index to temporary file: "
                << temp_filename.value();
@@ -371,4 +220,11 @@ void SimpleIndex::UpdateFile(const base::FilePath& index_filename,
   DCHECK(result);
 }
 
+// static
+uint32 SimpleIndexFile::CalculatePickleCRC(const Pickle& pickle) {
+  return crc32(crc32(0, Z_NULL, 0),
+               reinterpret_cast<const Bytef*>(pickle.payload()),
+               implicit_cast<uInt>(pickle.payload_size()));
+}
+
 }  // namespace disk_cache