Asynchronous initialization in Simple Index.

net/disk_cache/simple/simple_index.cc

Index: net/disk_cache/simple/simple_index.cc
diff --git a/net/disk_cache/simple/simple_index.cc b/net/disk_cache/simple/simple_index.cc
index cff8b011c6e3bce20f0b7b756fcb3d32b1312939..86977ff4444c37b59464a8fde5f08dc1780f4819 100644
--- a/net/disk_cache/simple/simple_index.cc
+++ b/net/disk_cache/simple/simple_index.cc
@@ -37,29 +37,71 @@ bool CheckHeader(disk_cache::SimpleIndexFile::Header header) {
       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,
@@ -68,18 +110,18 @@ bool SimpleIndex::Initialize() {
 
   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,
@@ -87,51 +129,66 @@ bool SimpleIndex::Initialize() {
                           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;
 }
@@ -149,27 +206,33 @@ bool SimpleIndex::UpdateEntrySize(const std::string& key, uint64 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();
@@ -190,9 +253,9 @@ bool SimpleIndex::RestoreFromDisk() {
       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
@@ -200,8 +263,39 @@ bool SimpleIndex::RestoreFromDisk() {
     }
   }
 
-  // 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) {
@@ -235,37 +329,17 @@ void SimpleIndex::Serialize(std::string* out_buffer) {
   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