Asynchronous initialization in Simple Index.

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 "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/message_loop.h"
 #include "base/task_runner.h"
@@ skipping 19 matching lines @@
   return true;
 }
 
 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(base::PlatformFile* file) : file_(file) { }

[gavinp, 2013/04/11 09:56:32]

No need for base::PlatformFile*. You can just use base::PlatformFile here.

[felipeg, 2013/04/11 11:25:45]

Done.

[felipeg, 2013/04/11 11:25:45]

Done.

+  ~FileAutoCloser() {
+    if (file_)

[gavinp, 2013/04/11 09:56:32]

Can this ever be null?

[felipeg, 2013/04/11 11:25:45]

Done.

+      base::ClosePlatformFile(*file_);

[gavinp, 2013/04/11 09:56:32]

No error checking?

[felipeg, 2013/04/11 11:25:45]

Nope
the only error that can happen is that the file is not opened.
And that can happen as not an error, since the destructor of this class will be
called even if we can't open the file on the LoadFromDisk function.

+  }
+ private:

[gavinp, 2013/04/11 09:56:32]

DISALLOW_COPY_AND_ASSIGN(FileAutoCloser);

+  base::PlatformFile* file_;
+};
+
 }  // namespace
 
 namespace disk_cache {
 
 SimpleIndex::SimpleIndex(
     const scoped_refptr<base::TaskRunner>& cache_thread,
+    const scoped_refptr<base::TaskRunner>& io_thread,
     const base::FilePath& path)
-    : path_(path),
-      cache_thread_(cache_thread) {
-  index_filename_ = path_.AppendASCII("index");
+    : cache_size_(0),
+      initialized_(false),
+      index_filename_(path.AppendASCII("simple-index")),
+      cache_thread_(cache_thread),
+      io_thread_(io_thread) {}
+
+void SimpleIndex::Initialize() {
+  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 SimpleIndex::Initialize() {
-  if (!OpenIndexFile())
-    return RestoreFromDisk();
+// static
+void SimpleIndex::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);
+  scoped_ptr<FileAutoCloser> auto_close_index_file(

[gavinp, 2013/04/11 09:56:32]

Why is this a scoped_ptr?

[felipeg, 2013/04/11 11:25:45]

Done.

[felipeg, 2013/04/11 11:25:45]

Done.

+      new FileAutoCloser(&index_file));
+  if (error != base::PLATFORM_FILE_OK) {
+    LOG(ERROR) << "Error opening file " << index_filename.value();
+    return RestoreFromDisk(index_filename, io_thread, merge_callback);
+  }
+
   uLong incremental_crc = crc32(0L, Z_NULL, 0);
   int64 index_file_offset = 0;
   SimpleIndexFile::Header header;
-  if (base::ReadPlatformFile(index_file_,
+  if (base::ReadPlatformFile(index_file,
                              index_file_offset,
                              reinterpret_cast<char*>(&header),
                              sizeof(header)) != sizeof(header)) {
-    return RestoreFromDisk();
+    return RestoreFromDisk(index_filename, io_thread, merge_callback);

[gavinp, 2013/04/11 09:56:32]

Should we delete the index file here, too?

[felipeg, 2013/04/11 11:25:45]

we don't need to.
RestoreFromDisk and Merge will take care of creating the new file and deleting
the old one.

   }
   index_file_offset += sizeof(header);
   incremental_crc = crc32(incremental_crc,
                           reinterpret_cast<const Bytef*>(&header),
                           implicit_cast<uInt>(sizeof(header)));
 
   if (!CheckHeader(header)) {
     LOG(ERROR) << "Invalid header on Simple Cache Index.";
-    return RestoreFromDisk();
+    return RestoreFromDisk(index_filename, io_thread, merge_callback);
   }
 
   const int entries_buffer_size =
       header.number_of_entries * SimpleIndexFile::kEntryMetadataSize;
 
   scoped_ptr<char[]> entries_buffer(new char[entries_buffer_size]);
-  if (base::ReadPlatformFile(index_file_,
+  if (base::ReadPlatformFile(index_file,
                              index_file_offset,
                              entries_buffer.get(),
                              entries_buffer_size) != entries_buffer_size) {
-    return RestoreFromDisk();
+    return RestoreFromDisk(index_filename, io_thread, merge_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_,
+  if (base::ReadPlatformFile(index_file,
                              index_file_offset,
                              reinterpret_cast<char*>(&footer),
                              sizeof(footer)) != sizeof(footer)) {
-    return RestoreFromDisk();
+    return RestoreFromDisk(index_filename, io_thread, merge_callback);
   }
   const uint32 crc_read = footer.crc;
   const uint32 crc_calculated = incremental_crc;
   if (crc_read != crc_calculated)
-    return RestoreFromDisk();
+    return RestoreFromDisk(index_filename, io_thread, merge_callback);
 
+  scoped_ptr<EntrySet> initializing_set(new EntrySet());

[gavinp, 2013/04/11 09:56:32]

This name is a bit confusing to me. How about index_file_entries ?

[felipeg, 2013/04/11 11:25:45]

Done.

[gavinp, 2013/04/11 11:35:39]

Done?

   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(entry_metadata);
+    InsertInternal(initializing_set.get(), entry_metadata);
     entries_buffer_offset += SimpleIndexFile::kEntryMetadataSize;
   }
-  DCHECK_EQ(header.number_of_entries, entries_set_.size());
-  CloseIndexFile();
-  return true;
+  DCHECK_EQ(header.number_of_entries, initializing_set->size());
+
+  io_thread->PostTask(FROM_HERE,
+                      base::Bind(merge_callback,
+                                 base::Passed(&initializing_set)));
 }
 
 void SimpleIndex::Insert(const std::string& key) {
   // 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.
-  InsertInternal(SimpleIndexFile::EntryMetadata(GetEntryHashForKey(key),
-                                                base::Time::Now(), 0));
+  // It will be updated later when the SynchronousEntryImpl finish and the
+  // UpdateEntrySize will be called.

[gavinp, 2013/04/11 09:56:32]

I liked this comment better when it was:

  // It will be updated later when the SimpleEntryImpl finishes opening or
  // creating the new entry, and then UpdateEntrySize will be called.

[felipeg, 2013/04/11 11:25:45]

Done.

+  const std::string hash_key = GetEntryHashForKey(key);
+  InsertInternal(&entries_set_, SimpleIndexFile::EntryMetadata(
+      hash_key,
+      base::Time::Now(), 0));
+  if (!initialized_)
+    removals_set_.erase(hash_key);
 }
 
 void SimpleIndex::Remove(const std::string& key) {
   UpdateEntrySize(key, 0);
-  entries_set_.erase(GetEntryHashForKey(key));
+  const std::string hash_key = GetEntryHashForKey(key);
+  entries_set_.erase(hash_key);
+
+  if (!initialized_)
+    removals_set_.insert(hash_key);
 }
 
 bool SimpleIndex::Has(const std::string& key) const {
-  return entries_set_.count(GetEntryHashForKey(key)) != 0;
+  // If not initialized, always return true, forcing it to go to the disk.
+  return !initialized_ || entries_set_.count(GetEntryHashForKey(key)) != 0;
 }
 
 bool SimpleIndex::UseIfExists(const std::string& key) {
+  // Always update the last used time, even if it is during initialization.
+  // It will be merged later.
   EntrySet::iterator it = entries_set_.find(GetEntryHashForKey(key));
   if (it == entries_set_.end())
-    return false;
+    // 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) {
   EntrySet::iterator it = entries_set_.find(GetEntryHashForKey(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;
 }
 
+// static
 void SimpleIndex::InsertInternal(

[gavinp, 2013/04/11 09:56:32]

Do we really want this? Just call insert maybe?

[felipeg, 2013/04/11 11:25:45]

I want to avoid having this long line through the code:
make_pair(entry_metadata.GetHashKey(), entry_metadata)

When we change the hash_map to be a hash_set we will probably change it here.

+    EntrySet* entry_set,
     const SimpleIndexFile::EntryMetadata& entry_metadata) {
-  entries_set_.insert(make_pair(entry_metadata.GetHashKey(), entry_metadata));
+  DCHECK(entry_set);
+  entry_set->insert(make_pair(entry_metadata.GetHashKey(), entry_metadata));
 }
 
-bool SimpleIndex::RestoreFromDisk() {
+// static
+void SimpleIndex::RestoreFromDisk(
+    const base::FilePath& index_filename,
+    const scoped_refptr<base::TaskRunner>& io_thread,
+    const MergeCallback& merge_callback) {
   using file_util::FileEnumerator;
   LOG(INFO) << "Simple Cache Index is being restored from disk.";
-  CloseIndexFile();
-  file_util::Delete(index_filename_, /* recursive = */ false);
-  entries_set_.clear();
+
+  file_util::Delete(index_filename, /* recursive = */ false);
+  scoped_ptr<EntrySet> initializing_set(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 base::FilePath::StringType file_pattern = FILE_PATH_LITERAL("*_[0-2]");
-  FileEnumerator enumerator(path_,
+  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 = hash_name.substr(0, kEntryHashKeySize);
 
     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.
     last_used_time = base::Time::FromTimeT(find_info.stat.st_atime);
 #endif
     if (last_used_time.is_null())
       last_used_time = FileEnumerator::GetLastModifiedTime(find_info);
 
     int64 file_size = FileEnumerator::GetFilesize(find_info);
-    EntrySet::iterator it = entries_set_.find(hash_key);
-    if (it == entries_set_.end()) {
-      InsertInternal(SimpleIndexFile::EntryMetadata(
+    EntrySet::iterator it = initializing_set->find(hash_key);
+    if (it == initializing_set->end()) {
+      InsertInternal(initializing_set.get(), SimpleIndexFile::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;
     }
   }
 
-  // TODO(felipeg): Detect unrecoverable problems and return false here.
-  return true;
+  io_thread->PostTask(FROM_HERE,
+                      base::Bind(merge_callback,
+                                 base::Passed(&initializing_set)));
+}
+
+void SimpleIndex::MergeInitializingSet(scoped_ptr<EntrySet> initializing_set) {
+  DCHECK(io_thread_checker_.CalledOnValidThread());
+  // First, remove the entries that are in the |removals_set_| from both sets.
+  for (base::hash_set<std::string>::const_iterator it = removals_set_.begin();
+       it != removals_set_.end(); ++it) {
+    entries_set_.erase(*it);
+    initializing_set->erase(*it);
+  }
+
+  // Recalculate the cache size while merging the two sets.
+  cache_size_ = 0;
+  for (EntrySet::const_iterator it = initializing_set->begin();
+       it != initializing_set->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()) {

[gavinp, 2013/04/11 09:56:32]

Why handle the case of inserting and merging differently? How about instead:

EntrySet::iterator current_entry =
entries_set_.insert(SimpleIndexFile::EntryMetadata()).first;
SimpleIndexFile::EntryMetadata::Merge(it->second, &(current_entry->second));
cache_size_ += current_entry->second.entry_size;

11 lines becomes 3!

[felipeg, 2013/04/11 11:25:45]

Done.

+      // 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;
+    }
+  }
+
+  initialized_ = true;
 }
 
 void SimpleIndex::Serialize(std::string* out_buffer) {
   DCHECK(out_buffer);
   SimpleIndexFile::Header header;
   SimpleIndexFile::Footer footer;
 
   header.initial_magic_number = kSimpleIndexInitialMagicNumber;
   header.version = kSimpleVersion;
   header.number_of_entries = entries_set_.size();
@@ skipping 13 matching lines @@
   }
 
   // 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::Cleanup() {
+void SimpleIndex::WriteToDisk() {

[gavinp, 2013/04/11 09:56:32]

Can you add a thread checker call on EVERY method? It's much clearer.

[felipeg, 2013/04/11 11:25:45]

Done.

[gavinp, 2013/04/11 11:35:39]

Done?

   scoped_ptr<std::string> buffer(new std::string());
   Serialize(buffer.get());
-  cache_thread_->PostTask(FROM_HERE,
-                            base::Bind(&SimpleIndex::UpdateFile,
-                                       index_filename_,
-                                       path_.AppendASCII("index_temp"),
-                                       base::Passed(&buffer)));
+  cache_thread_->PostTask(FROM_HERE, base::Bind(
+      &SimpleIndex::UpdateFile,
+      index_filename_,
+      index_filename_.DirName().AppendASCII("index_temp"),
+      base::Passed(&buffer)));
 }
 
 SimpleIndex::~SimpleIndex() {
-  CloseIndexFile();
-}
-
-bool SimpleIndex::OpenIndexFile() {
-  base::PlatformFileError error;
-  index_file_ = base::CreatePlatformFile(index_filename_,
-                                         base::PLATFORM_FILE_OPEN_ALWAYS |
-                                         base::PLATFORM_FILE_READ |
-                                         base::PLATFORM_FILE_WRITE,
-                                         NULL,
-                                         &error);
-  if (error != base::PLATFORM_FILE_OK) {
-    LOG(ERROR) << "Error opening file " << index_filename_.value();
-    return false;
-  }
-  return true;
-}
-
-bool SimpleIndex::CloseIndexFile() {
-  return base::ClosePlatformFile(index_file_);
 }
 
 // static
 void SimpleIndex::UpdateFile(const base::FilePath& index_filename,
                              const base::FilePath& temp_filename,
                              scoped_ptr<std::string> buffer) {
   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())) {
     // TODO(felipeg): Add better error handling.
     LOG(ERROR) << "Could not write Simple Cache index to temporary file: "
                << temp_filename.value();
     file_util::Delete(temp_filename, /* recursive = */ false);
     return;
   }
   // Swap temp and index_file.
   bool result = file_util::ReplaceFile(temp_filename, index_filename);
   DCHECK(result);
 }
 
 }  // namespace disk_cache