Index initialization improvements for Simple Cache.

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 caec19f6068fd9fa2e1571558419ad2ca3096b71..78ce87ed71d6c9006f3b67cdfc5575d3bc9439b3 100644
--- a/net/disk_cache/simple/simple_index.cc
+++ b/net/disk_cache/simple/simple_index.cc
@@ -359,18 +359,24 @@ void SimpleIndex::MergeInitializingSet(
     index_file_entries->erase(*it);
   }
 
-  // Recalculate the cache size while merging the two sets.
-  for (EntrySet::const_iterator it = index_file_entries->begin();
+  for (EntrySet::const_iterator it = entries_set_.begin();
+       it != entries_set_.end(); ++it) {
+    const uint64 entry_hash = it->first;
+    std::pair<EntrySet::iterator, bool> insert_result =
+        index_file_entries->insert(EntrySet::value_type(entry_hash,
+                                                        EntryMetadata()));
+    EntrySet::iterator& possibly_inserted_entry = insert_result.first;
+    possibly_inserted_entry->second = it->second;

[Randy Smith (Not in Mondays), 2013/08/12 17:02:20]

Suggestion: Why not test insert_result.second to decide whether to insert?  I
sorta assume it's in cache anyway since we're touching .first, and it'd save a
24 byte copy.  I don't see a reason to believe that that case would be
particularly common or uncommon.

(I believe your judgement will be better than mine as to whether this is a good
idea, thus "suggestion".)

[gavinp, 2013/08/13 11:11:05]

I think it's just a copy of EntrySet::data_type you're talking about saving, not
EntrySet::value_type, so it's 16 bytes, not 24. But that's a minor detail.

I originally coded it this way you mentioned. The constructor EntryMetadata() on
line 368 had to be replaced with |it->second|, and then recapitulated in the
case that insert_result.second is false; that repetition seemed a bit confusing,
so I went with the less conditional path.

I'm pretty sure, but I didn't check by compiling, that data flow analysis should
be able to toss out the redundant call to EntryMetadata(), making this just as
efficient. The constructor to EntryMetadata is inline able in this context. The
STL authors are actually fairly careful about this, because this code is the
same as:

  index_file_entries[entry_hash] = it->second;

And now that I think about it, perhaps we should replace lihnes 368-370 with
just that. I'll upload that CL later today.

+  }
+
+  uint64 merged_cache_size = 0;
+  for (EntrySet::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);
-    // When Merging, existing data in the |current_entry| will prevail.
-    if (current_entry == entries_set_.end()) {
-      InsertInEntrySet(it->first, it->second, &entries_set_);
-      cache_size_ += it->second.GetEntrySize();
-    }
+    merged_cache_size += it->second.GetEntrySize();
   }
+
+  entries_set_.swap(*index_file_entries);
+  cache_size_ = merged_cache_size;
   initialized_ = true;
   removed_entries_.clear();