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

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 13303830dbd348b6a4d74b5ffed3ecc57f6181cd..b2fb9dc8ddbd6cf75d519aaaa4e8903de3fd1929 100644
--- a/components/safe_browsing_db/v4_store.cc
+++ b/components/safe_browsing_db/v4_store.cc
@@ -191,8 +191,8 @@ ApplyUpdateResult V4Store::ProcessPartialUpdateAndWriteToDisk(
   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;
 }
@@ -203,7 +203,8 @@ ApplyUpdateResult V4Store::ProcessFullUpdateAndWriteToDisk(
   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;
 }
@@ -638,28 +639,31 @@ StoreReadResult V4Store::ReadFromDisk() {
   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(),