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

components/safe_browsing_db/v4_store.cc

 // Copyright 2016 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 "base/base64.h"
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
@@ skipping 170 matching lines @@
   state_ = "";
 }
 
 ApplyUpdateResult V4Store::ProcessPartialUpdateAndWriteToDisk(
     const HashPrefixMap& hash_prefix_map_old,
     std::unique_ptr<ListUpdateResponse> response) {
   DCHECK(response->has_response_type());
   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();
     response.reset();
     RecordStoreWriteResult(WriteToDisk(checksum));
   }
   return result;
 }
 
 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();
     response.reset();
     RecordStoreWriteResult(WriteToDisk(checksum));
   }
   return result;
 }
 
 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
   // lexographical order to store it, but we do need to do that for calculating
   // 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();
   DCHECK_LE(removals_size, 1u);
   if (removals_size == 1) {
     const ThreatEntrySet& removal = response->removals().Get(0);
     const CompressionType compression_type = removal.compression_type();
     if (compression_type == RAW) {
       raw_removals = &removal.raw_indices().indices();
     } else if (compression_type == RICE) {
@@ skipping 24 matching lines @@
   if (apply_update_result != APPLY_UPDATE_SUCCESS) {
     return apply_update_result;
   }
 
   std::string expected_checksum;
   if (response->has_checksum() && response->checksum().has_sha256()) {
     expected_checksum = response->checksum().sha256();
   }
 
   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;
   }
   RecordMergeUpdateTime(TimeTicks::Now() - before, store_path_);
 
   state_ = response->new_client_state();
   return APPLY_UPDATE_SUCCESS;
 }
 
 void V4Store::ApplyUpdate(
@@ skipping 158 matching lines @@
     size_t existing_capacity = existing_prefixes.capacity();
 
     (*prefix_map_to_update)[prefix_size].reserve(existing_capacity +
                                                  prefix_length_to_add);
   }
 }
 
 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());
 
   bool calculate_checksum = !expected_checksum.empty();
   if (calculate_checksum &&
       (expected_checksum.size() != crypto::kSHA256Length)) {
     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,
     // reset the store.
     expected_checksum_ = expected_checksum;
 
     return APPLY_UPDATE_SUCCESS;
   }
 
   hash_prefix_map_.clear();
@@ skipping 147 matching lines @@
   if (file_format.version_number() != kFileVersion) {
     return FILE_VERSION_INCOMPATIBLE_FAILURE;
   }
 
   if (!file_format.has_list_update_response()) {
     return HASH_PREFIX_INFO_MISSING_FAILURE;
   }
 
   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();
     return HASH_PREFIX_MAP_GENERATION_FAILURE;
   }
   RecordReadFromDiskTime(TimeTicks::Now() - before, store_path_);
 
   return READ_SUCCESS;
 }
 
@@ skipping 118 matching lines @@
                << "; expected: " << expected_checksum_b64
                << "; store: " << *this;
 #endif
       return false;
     }
   }
   return true;
 }
 
 }  // namespace safe_browsing