Small: Serialize the store's hash_prefix_map_ to file in WriteToDisk()

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 173 matching lines @@
 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);
   if (result == APPLY_UPDATE_SUCCESS) {
     RecordProcessPartialUpdateTime(TimeTicks::Now() - before, store_path_);
-    // TODO(vakh): Create a ListUpdateResponse containing RICE encoded
-    // hash prefixes and response_type as FULL_UPDATE, and write that to disk.
+    RecordStoreWriteResult(
+        WriteToDisk(ListIdentifier(*response), response->checksum()));
   }
   return result;
 }
 
 ApplyUpdateResult V4Store::ProcessFullUpdateAndWriteToDisk(
     std::unique_ptr<ListUpdateResponse> response) {
   TimeTicks before = TimeTicks::Now();
   ApplyUpdateResult result = ProcessFullUpdate(response);
   if (result == APPLY_UPDATE_SUCCESS) {
     RecordProcessFullUpdateTime(TimeTicks::Now() - before, store_path_);
-    RecordStoreWriteResult(WriteToDisk(std::move(response)));
+    RecordStoreWriteResult(
+        WriteToDisk(ListIdentifier(*response), response->checksum()));
   }
   return result;
 }
 
 ApplyUpdateResult V4Store::ProcessFullUpdate(
     const std::unique_ptr<ListUpdateResponse>& response) {
   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
@@ skipping 414 matching lines @@
   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;
 }
 
-StoreWriteResult V4Store::WriteToDisk(
-    std::unique_ptr<ListUpdateResponse> response) const {
-  // Do not write partial updates to the disk.
-  // After merging the updates, the ListUpdateResponse passed to this method
-  // should be a FULL_UPDATE.
-  if (!response->has_response_type() ||
-      response->response_type() != ListUpdateResponse::FULL_UPDATE) {
-    DVLOG(1) << "Failure: response->has_response_type(): "
-             << response->has_response_type()
-             << " : response->response_type(): " << response->response_type();
-    return INVALID_RESPONSE_TYPE_FAILURE;
+StoreWriteResult V4Store::WriteToDisk(const ListIdentifier& list_id,
+                                      const Checksum& checksum) const {
+  V4StoreFileFormat file_format;
+  ListUpdateResponse* lur = file_format.mutable_list_update_response();
+  *(lur->mutable_checksum()) = checksum;

[Nathan Parker, 2016/10/11 20:29:41]

nit: lur->set_checksum(checksum)?

[vakh (use Gerrit instead), 2016/10/11 21:50:39]

set_ is only available for primitive types.
See:
https://cs.chromium.org/chromium/src/out/Debug/gen/components/safe_browsing_d...

+  lur->set_new_client_state(state_);
+  lur->set_platform_type(list_id.platform_type());
+  lur->set_response_type(ListUpdateResponse::FULL_UPDATE);
+  lur->set_threat_entry_type(list_id.threat_entry_type());
+  lur->set_threat_type(list_id.threat_type());
+  for (auto map_iter : hash_prefix_map_) {
+    ThreatEntrySet* additions = lur->add_additions();
+    // TODO(vakh): Write RICE encoded hash prefixes on disk. Not doing so
+    // currently since it takes a long time to decode them on startup, which
+    // blocks resource load.
+    additions->set_compression_type(RAW);
+    additions->mutable_raw_hashes()->set_prefix_size(map_iter.first);
+    additions->mutable_raw_hashes()->set_raw_hashes(map_iter.second);

[Nathan Parker, 2016/10/11 20:29:41]

Note: This creates another full copy of the whole list. Then there's another
with the SerializeToString(). It's not super critical, but I'd say it's higher
priority than the TODO for Rice encoding on disk since this'll increase the peak
RAM usage by at least a few MB.

Actually, you could free up some memory by destroying the response in
Process*UpdateAndWriteToDisk() _before_ calling this function. That'd mostly
just help in a the full-update case, since partials will be smallish.

Also, in ReadFromDisk(), you might want to make |contents| a scoped_ptr so you
can destroy it before doing merge/updates since that will also use more RAM.
That minimizes the number of big allocated blocks that overlap in time.

[vakh (use Gerrit instead), 2016/10/11 21:50:39]

Ack.
Done for both to be consistent.
Now clearing |contents| as soon as the proto has been parsed.

   }
 
   // Attempt writing to a temporary file first and at the end, swap the files.
   const base::FilePath new_filename = TemporaryFileForFilename(store_path_);
 
-  V4StoreFileFormat file_format;
   file_format.set_magic_number(kFileMagic);
   file_format.set_version_number(kFileVersion);
-  ListUpdateResponse* response_to_write =
-      file_format.mutable_list_update_response();
-  response_to_write->Swap(response.get());
   std::string file_format_string;
   file_format.SerializeToString(&file_format_string);
   size_t written = base::WriteFile(new_filename, file_format_string.data(),
                                    file_format_string.size());
   DCHECK_EQ(file_format_string.size(), written);
 
   if (!base::Move(new_filename, store_path_)) {
     return UNABLE_TO_RENAME_FAILURE;
   }
 
@@ skipping 87 matching lines @@
                << "; expected: " << expected_checksum_b64
                << "; store: " << *this;
 #endif
       return false;
     }
   }
   return true;
 }
 
 }  // namespace safe_browsing