Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2403913004:
Small: Serialize the store's hash_prefix_map_ to file in WriteToDisk() (Closed)
|
Created:
4 years, 2 months ago by vakh (use Gerrit instead) Modified:
4 years, 2 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSerialize the store's hash_prefix_map_ to file in WriteToDisk()
Associating with bug 651911 since I am writing RAW hash prefixes on disk in this CL to improve startup time.
BUG=543161, 651911
Committed: https://crrev.com/9f1266e1e3cdb43dd9f5031f1dd3f5e7262a0d13
Cr-Commit-Position: refs/heads/master@{#425233}
Patch Set 1 : Serialize the store's hash_prefix_map_ to file in WriteToDisk() #
Total comments: 10
Patch Set 2 : nparker@ review #Patch Set 3 : git rebase #
Total comments: 2
Patch Set 4 : rebase. and temporary scope to make sure that |contents| get destroyed as soon as we're done using … #
Messages
Total messages: 33 (19 generated)
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Serialize the store's hash_prefix_map_ to file in WriteToDisk()
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vakh@chromium.org changed reviewers: + nparker@chromium.org, shess@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm.
Description was changed from ========== Serialize the store's hash_prefix_map_ to file in WriteToDisk() BUG=543161 ========== to ========== Serialize the store's hash_prefix_map_ to file in WriteToDisk() Associating with bug 651911 since I am writing RAW hash prefixes on disk in this CL to improve startup time. BUG=543161, 651911 ==========
https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:646: *(lur->mutable_checksum()) = checksum; nit: lur->set_checksum(checksum)? https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:659: additions->mutable_raw_hashes()->set_raw_hashes(map_iter.second); 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. https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.h:354: // Writes the FULL_UPDATE |response| to disk as a V4StoreFileFormat proto. There's no longer a |response| arg https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:542: TEST_F(V4StoreTest, TestReadFullResponseWithValidHashPrefixMap) { How about a test for when a write fails (like, permission denied or some such)? And is there any test to verify that a ApplyUpdate() (full/partial) calls WriteToDisk correctly? https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:553: ASSERT_EQ(2u, read_store.hash_prefix_map_.size()); This should verify the store id too, ya?
nparker@ review
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Thanks for the review. PTAL. https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:646: *(lur->mutable_checksum()) = checksum; On 2016/10/11 20:29:41, Nathan Parker wrote: > nit: lur->set_checksum(checksum)? set_ is only available for primitive types. See: https://cs.chromium.org/chromium/src/out/Debug/gen/components/safe_browsing_d... https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:659: additions->mutable_raw_hashes()->set_raw_hashes(map_iter.second); On 2016/10/11 20:29:41, Nathan Parker wrote: > 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. Ack. > > 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. > Done for both to be consistent. > 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. Now clearing |contents| as soon as the proto has been parsed. https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.h:354: // Writes the FULL_UPDATE |response| to disk as a V4StoreFileFormat proto. On 2016/10/11 20:29:42, Nathan Parker wrote: > There's no longer a |response| arg Done. https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:542: TEST_F(V4StoreTest, TestReadFullResponseWithValidHashPrefixMap) { On 2016/10/11 20:29:42, Nathan Parker wrote: > How about a test for when a write fails (like, permission denied or some such)? > And is there any test to verify that a ApplyUpdate() (full/partial) calls > WriteToDisk correctly? Done. https://codereview.chromium.org/2403913004/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:553: ASSERT_EQ(2u, read_store.hash_prefix_map_.size()); On 2016/10/11 20:29:42, Nathan Parker wrote: > This should verify the store id too, ya? I realized that there's no need to set the list_id since we never use it. The store file is read as a set of hash prefixes. The store is never aware of which list it represents. The DB is aware of that, but it manages that by using unique filenames for each store.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
git rebase
lgtm https://codereview.chromium.org/2403913004/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2403913004/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:652: contents.clear(); I think this doesn't actually free memory. You probably want to call shrink_to_fit() after, or else declare the string as a unique_ptr so you can reset it.
https://codereview.chromium.org/2403913004/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2403913004/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:652: contents.clear(); On 2016/10/14 00:10:05, Nathan Parker wrote: > I think this doesn't actually free memory. You probably want to call > shrink_to_fit() after, or else declare the string as a unique_ptr so you can > reset it. The canonical way to release string memory is something like std::string().swap(contents), though something horrible like ignore_result(std::move(contents)) would probably work, too. Of course you could just scope it appropriately, albeit at some cost to rearrange your variable declarations. Also IMHO, don't unique_ptr it. It works, but it's not much work to avoid the extra heap allocation.
rebase. and temporary scope to make sure that |contents| get destroyed as soon as we're done using it.
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by vakh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2403913004/#ps80001 (title: "rebase. and temporary scope to make sure that |contents| get destroyed as soon as we're done using …")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Serialize the store's hash_prefix_map_ to file in WriteToDisk() Associating with bug 651911 since I am writing RAW hash prefixes on disk in this CL to improve startup time. BUG=543161, 651911 ========== to ========== Serialize the store's hash_prefix_map_ to file in WriteToDisk() Associating with bug 651911 since I am writing RAW hash prefixes on disk in this CL to improve startup time. BUG=543161, 651911 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Serialize the store's hash_prefix_map_ to file in WriteToDisk() Associating with bug 651911 since I am writing RAW hash prefixes on disk in this CL to improve startup time. BUG=543161, 651911 ========== to ========== Serialize the store's hash_prefix_map_ to file in WriteToDisk() Associating with bug 651911 since I am writing RAW hash prefixes on disk in this CL to improve startup time. BUG=543161, 651911 Committed: https://crrev.com/9f1266e1e3cdb43dd9f5031f1dd3f5e7262a0d13 Cr-Commit-Position: refs/heads/master@{#425233} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9f1266e1e3cdb43dd9f5031f1dd3f5e7262a0d13 Cr-Commit-Position: refs/heads/master@{#425233}
Message was sent while issue was closed.
On 2016/10/14 00:16:18, Scott Hess wrote: > https://codereview.chromium.org/2403913004/diff/60001/components/safe_browsin... > File components/safe_browsing_db/v4_store.cc (right): > > https://codereview.chromium.org/2403913004/diff/60001/components/safe_browsin... > components/safe_browsing_db/v4_store.cc:652: contents.clear(); > On 2016/10/14 00:10:05, Nathan Parker wrote: > > I think this doesn't actually free memory. You probably want to call > > shrink_to_fit() after, or else declare the string as a unique_ptr so you can > > reset it. > > The canonical way to release string memory is something like > std::string().swap(contents), though something horrible like > ignore_result(std::move(contents)) would probably work, too. Of course you > could just scope it appropriately, albeit at some cost to rearrange your > variable declarations. > > Also IMHO, don't unique_ptr it. It works, but it's not much work to avoid the > extra heap allocation. Done with temporary scope. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
