Small: Delay checksum only on ReadFromDisk, not in FullUpdate cases

components/safe_browsing_db/v4_store.cc

Index: components/safe_browsing_db/v4_store.cc
diff --git a/components/safe_browsing_db/v4_store.cc b/components/safe_browsing_db/v4_store.cc
index 5bfad46dfc9e44dfdad4ac6f3db0fefd78026e9b..69bc79744ebff2fdc361184afdc0dcee93b2f56e 100644
--- a/components/safe_browsing_db/v4_store.cc
+++ b/components/safe_browsing_db/v4_store.cc
@@ -188,7 +188,8 @@ ApplyUpdateResult V4Store::ProcessPartialUpdateAndWriteToDisk(
   DCHECK_EQ(ListUpdateResponse::PARTIAL_UPDATE, response->response_type());
 
   TimeTicks before = TimeTicks::Now();
-  ApplyUpdateResult result = ProcessUpdate(hash_prefix_map_old, response);
+  ApplyUpdateResult result =
+      ProcessUpdate(hash_prefix_map_old, response, false);

[Nathan Parker, 2016/10/14 23:49:38]

nit: false /* delay_checksum check */ )

   if (result == APPLY_UPDATE_SUCCESS) {
     RecordProcessPartialUpdateTime(TimeTicks::Now() - before, store_path_);
     Checksum checksum = response->checksum();
@@ -201,7 +202,7 @@ ApplyUpdateResult V4Store::ProcessPartialUpdateAndWriteToDisk(
 ApplyUpdateResult V4Store::ProcessFullUpdateAndWriteToDisk(
     std::unique_ptr<ListUpdateResponse> response) {
   TimeTicks before = TimeTicks::Now();
-  ApplyUpdateResult result = ProcessFullUpdate(response);
+  ApplyUpdateResult result = ProcessFullUpdate(response, false);
   if (result == APPLY_UPDATE_SUCCESS) {
     RecordProcessFullUpdateTime(TimeTicks::Now() - before, store_path_);
     Checksum checksum = response->checksum();
@@ -212,7 +213,8 @@ ApplyUpdateResult V4Store::ProcessFullUpdateAndWriteToDisk(
 }
 
 ApplyUpdateResult V4Store::ProcessFullUpdate(
-    const std::unique_ptr<ListUpdateResponse>& response) {
+    const std::unique_ptr<ListUpdateResponse>& response,
+    bool delay_checksum_check) {
   DCHECK(response->has_response_type());
   DCHECK_EQ(ListUpdateResponse::FULL_UPDATE, response->response_type());
   // TODO(vakh): For a full update, we don't need to process the update in
@@ -220,12 +222,13 @@ ApplyUpdateResult V4Store::ProcessFullUpdate(
   // checksum. It might save some CPU cycles to store the full update as-is and
   // walk the list of hash prefixes in lexographical order only for checksum
   // calculation.
-  return ProcessUpdate(HashPrefixMap(), response);
+  return ProcessUpdate(HashPrefixMap(), response, delay_checksum_check);
 }
 
 ApplyUpdateResult V4Store::ProcessUpdate(
     const HashPrefixMap& hash_prefix_map_old,
-    const std::unique_ptr<ListUpdateResponse>& response) {
+    const std::unique_ptr<ListUpdateResponse>& response,
+    bool delay_checksum_check) {
   const RepeatedField<int32>* raw_removals = nullptr;
   RepeatedField<int32> rice_removals;
   size_t removals_size = response->removals_size();
@@ -270,8 +273,9 @@ ApplyUpdateResult V4Store::ProcessUpdate(
   }
 
   TimeTicks before = TimeTicks::Now();
-  apply_update_result = MergeUpdate(hash_prefix_map_old, hash_prefix_map,
-                                    raw_removals, expected_checksum);
+  apply_update_result =
+      MergeUpdate(hash_prefix_map_old, hash_prefix_map, raw_removals,
+                  expected_checksum, delay_checksum_check);
   if (apply_update_result != APPLY_UPDATE_SUCCESS) {
     return apply_update_result;
   }
@@ -450,7 +454,8 @@ void V4Store::ReserveSpaceInPrefixMap(const HashPrefixMap& other_prefixes_map,
 ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map,
                                        const HashPrefixMap& additions_map,
                                        const RepeatedField<int32>* raw_removals,
-                                       const std::string& expected_checksum) {
+                                       const std::string& expected_checksum,
+                                       bool delay_checksum_check) {
   DCHECK(task_runner_->RunsTasksOnCurrentThread());
   DCHECK(hash_prefix_map_.empty());
 
@@ -460,10 +465,12 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map,
     return CHECKSUM_MISMATCH_FAILURE;
   }
 
-  if (old_prefixes_map.empty()) {
-    // If the old map is empty, which it is at startup, then just copy over the
-    // additions map.
+  if (delay_checksum_check) {

[Nathan Parker, 2016/10/14 23:49:38]

A thought: I kinda feel like this block could be removed from mergeUpdate and
put directly into ProcessUpdate (or another func), since it really isn't merging
an update.

+    DCHECK(old_prefixes_map.empty());
     DCHECK(!raw_removals);
+    // We delay the checksum check at startup to be able to load the DB
+    // quickly. In this case, the old_prefixes_map should be empty, so just
+    // copy over the additions map.
     hash_prefix_map_ = additions_map;
 
     // Calculate the checksum asynchronously later and if it doesn't match,
@@ -631,7 +638,7 @@ StoreReadResult V4Store::ReadFromDisk() {
 
   std::unique_ptr<ListUpdateResponse> response(new ListUpdateResponse);
   response->Swap(file_format.mutable_list_update_response());
-  ApplyUpdateResult apply_update_result = ProcessFullUpdate(response);
+  ApplyUpdateResult apply_update_result = ProcessFullUpdate(response, true);

[Nathan Parker, 2016/10/14 23:49:38]

true /* delay_checksum_check */

   RecordApplyUpdateResultWhenReadingFromDisk(apply_update_result);
   if (apply_update_result != APPLY_UPDATE_SUCCESS) {
     hash_prefix_map_.clear();