Avoid fragmenting the heap too much while reconstructing the index.

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_file.cc b/net/disk_cache/simple/simple_index_file.cc
index 7bcea7cdfa86255360a7416d4eff5731ca24ae7c..4b035f35c00e9732ff2c316cfe70485f4999c91a 100644
--- a/net/disk_cache/simple/simple_index_file.cc
+++ b/net/disk_cache/simple/simple_index_file.cc
@@ -6,8 +6,8 @@
 
 #include <vector>
 
+#include "base/basictypes.h"
 #include "base/file_util.h"
-#include "base/files/file_enumerator.h"
 #include "base/files/memory_mapped_file.h"
 #include "base/hash.h"
 #include "base/logging.h"
@@ -25,6 +25,9 @@
 
 namespace {
 
+const int kEntryFilesHashLength = 16;
+const int kEntryFilesSuffixLength = 2;
+
 const uint64 kMaxEntiresInIndex = 100000000;
 
 uint32 CalculatePickleCRC(const Pickle& pickle) {
@@ -345,12 +348,65 @@ void SimpleIndexFile::Deserialize(const char* data, int data_len,
 }
 
 // static
+void SimpleIndexFile::ProcessEntryFile(const std::string& cache_path,
+                                       std::string* buffer,
+                                       SimpleIndex::EntrySet* entries,
+                                       const std::string& file_name) {
+  static const size_t kEntryFilesLength =
+      kEntryFilesHashLength + kEntryFilesSuffixLength;
+  if (file_name.length() != kEntryFilesLength) {
+    if (file_name != "index" && file_name != "fake_index")
+      LOG(ERROR) << "unexpected file " << file_name;
+    return;
+  }
+  buffer->assign(file_name.c_str(), kEntryFilesHashLength);
+  uint64 hash_key = 0;
+  if (!simple_util::GetEntryHashKeyFromHexString(
+          *buffer, &hash_key)) {
+    LOG(WARNING) << "Invalid Entry Hash Key filename while restoring "
+                 << "Simple Index from disk: " << *buffer;
+    // TODO(felipeg): Should we delete the invalid file here ?
+    return;
+  }
+
+  buffer->assign(cache_path);
+  buffer->append(&base::FilePath::kSeparators[0], 1);
+  buffer->append(file_name);

[gavinp, 2013/08/20 20:13:45]

I know that we've set capacity(), but I'm still scared that these are
potentially allocating, as the standard allows iterator invalidation on ANY
nonconst method on string, see ISO/IEC 14882:1998(E), 21.3(5).

[Philippe, 2013/08/21 09:37:33]

Yeah. I replaced the string with a vector.

+
+  base::FilePath file_path(*buffer);

[gavinp, 2013/08/20 20:13:45]

Does this compile on Win32?

More concerning: this is potentially allocating. base::FilePath is just a
wrapper around std::string or std::wstring. In glibc, the copy constructor is
not allocating, so as long as no '\0' character is in the string, this isn't
allocating. However, in C++11, it's difficult to imagine a std::string copy
constructor that isn't allocating, and worse, we can't pass an allocator in to
base::FilePath.

[Philippe, 2013/08/21 09:37:33]

I got rid of FilePath here as we discussed offline. I will run a windows try bot
to see if things compile there.

+  base::PlatformFileInfo file_info;
+  if (!file_util::GetFileInfo(file_path, &file_info)) {
+    LOG(INFO) << "Could not get file info for path " << file_path.value();
+    return;
+  }
+  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 = file_info.last_accessed;
+#endif
+  if (last_used_time.is_null())
+    last_used_time = file_info.last_modified;
+
+  int64 file_size = file_info.size;
+  SimpleIndex::EntrySet::iterator it = entries->find(hash_key);
+  if (it == entries->end()) {
+    SimpleIndex::InsertInEntrySet(
+        hash_key,
+        EntryMetadata(last_used_time, file_size),
+        entries);
+  } else {
+    // Summing up the total size of the entry through all the *_[0-2] files
+    it->second.SetEntrySize(it->second.GetEntrySize() + file_size);
+  }
+}
+
+// static
 void SimpleIndexFile::SyncRestoreFromDisk(
     const base::FilePath& cache_directory,
     const base::FilePath& index_file_path,
     SimpleIndexLoadResult* out_result) {
   LOG(INFO) << "Simple Cache Index is being restored from disk.";
-
   base::DeleteFile(index_file_path, /* recursive = */ false);
   out_result->Reset();
   SimpleIndex::EntrySet* entries = &out_result->entries;
@@ -359,53 +415,17 @@ void SimpleIndexFile::SyncRestoreFromDisk(
   COMPILE_ASSERT(kSimpleEntryFileCount == 3,
                  file_pattern_must_match_file_count);
 
-  const int kFileSuffixLength = sizeof("_0") - 1;
-  const base::FilePath::StringType file_pattern = FILE_PATH_LITERAL("*_[0-2]");
-  base::FileEnumerator enumerator(cache_directory,
-                                  false /* recursive */,
-                                  base::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_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: " << base_name;
-      // TODO(felipeg): Should we delete the invalid file here ?
-      continue;
-    }
-
-    base::FileEnumerator::FileInfo info = enumerator.GetInfo();
-    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(info.stat().st_atime);
-#endif
-    if (last_used_time.is_null())
-      last_used_time = info.GetLastModifiedTime();
-
-    int64 file_size = info.GetSize();
-    SimpleIndex::EntrySet::iterator it = entries->find(hash_key);
-    if (it == entries->end()) {
-      SimpleIndex::InsertInEntrySet(
-          hash_key,
-          EntryMetadata(last_used_time, file_size),
-          entries);
-    } else {
-      // Summing up the total size of the entry through all the *_[0-2] files
-      it->second.SetEntrySize(it->second.GetEntrySize() + file_size);
-    }
+  const std::string cache_path = cache_directory.value();
+  std::string buffer;
+  const bool did_succeed = TraverseCacheDirectory(
+      cache_path,
+      base::Bind(&SimpleIndexFile::ProcessEntryFile, cache_path, &buffer,
+                 entries));
+  if (!did_succeed) {
+    LOG(ERROR) << "Could not reconstruct index from disk";
+    return;
   }
-
   out_result->did_load = true;
-
   // When we restore from disk we write the merged index file to disk right
   // away, this might save us from having to restore again next time.
   out_result->flush_required = true;