Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2190233002:
Rice decode removals and additions and use them to update the store. (Closed)
|
Created:
4 years, 4 months ago by vakh (use Gerrit instead) Modified:
4 years, 4 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, woz, noé, Scott Hess - ex-Googler, palmer, francois_mozilla.com Base URL:
https://chromium.googlesource.com/chromium/src.git@01_v4_rice Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRice decode removals and additions and use them to update the store.
Also:
1. Add histograms for decoding additions/removals.
2. Update the histogram for ApplyUpdateResult to include the new failure values.
Please note that this CL does not enable RICE encoding yet since we still
telling the server that we can only process RAW updates. I'll make that tiny
change as a separate CL so that it is easier to rollback, if needed.
BUG=543161, 624567
Committed: https://crrev.com/605ab1c3447ebe4749d80c20a7d38c01a59c0887
Cr-Commit-Position: refs/heads/master@{#409844}
Patch Set 1 : TODOs: Add unit tests, remove DVLOGs #Patch Set 2 : git pull #Patch Set 3 : Remove DVLOGs. Add tests (not proud of the last test). #
Total comments: 27
Patch Set 4 : CR feedback: nparker@ #Patch Set 5 : Update comment in histograms.xml #Patch Set 6 : retry upload #Patch Set 7 : Add Process{Full,Partial}Update methods to simplify ApplyUpdate and reduce nesting #Patch Set 8 : Tiny: Move the declaration of hash_prefix_map closer to use #Patch Set 9 : Tiny: Remove a comment. Club non-static functions together in .h #
Messages
Total messages: 63 (37 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...)
git fetch and pull
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...
Also add v4_rice.h/cc to safe_browsing_db.gypi
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...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Also add v4_rice.h/cc to safe_browsing_db.gypi
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...
vakh@chromium.org changed reviewers: + nparker@chromium.org, palmer@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
vakh@chromium.org changed reviewers: - palmer@chromium.org
Gentle ping.
https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:51: UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.V4DecodeRemovalsResult", result, It'd be good to get some size/time metrics in here as well, in another CL. Size of updates, time to read updates, time to unpack/write updates, etc. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:130: std::unique_ptr<RepeatedField<int32>> rice_removals; This can just be a RepeatedField<int32> https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:135: const CompressionType compression_type = removal.compression_type(); nit: Not sure this temporary var helps much. Removal.compression_type() is pretty easy to read. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:141: rice_removals = base::MakeUnique<RepeatedField<int32>>(); This just re-inits it to what it was before, ya? https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:198: ApplyUpdateResult apply_update_result; This could go inside the for loop. And actually you don't need it at all. Since you want to return early as soon as there's an error, you could just do that at each point. That'd reduce some of the if/else nesting. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:205: PrefixSize prefix_size = addition.raw_hashes().prefix_size(); nit: I think this temporary doesn't add to the readability, esp since the member name is already descriptive. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:212: std::string raw_hashes_for_rice_encoded_hashes; nit: Since this block of code is just for Rice encoding, you could use a shorter var name (raw_hashes?). https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.h:141: // Compression type other than RAW and RICE for additions. Nit: This might be a bit more detailed error reporting than needed, since the probability of needing to differentiate these is very small. Could be rolled into a "invalid_update_proto_failre." But your choice. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.h:254: friend class V4StoreTest; Does this obviate the need for all the friends? It's a lot of friends. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:632: addition->mutable_rice_hashes()->set_num_entries(-1); [tangential commentary: It's annoying that these proto fields are signed ints. Oh well.] https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:648: "\xbf\xa8\x3f\xfb\xf\xf\x5e\x27\xe6\xc3\x1d\xc6\x38"); Add a comment describing how this was constructed, in case someone wants to make more. https://codereview.chromium.org/2190233002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2190233002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:47683: + hash prefixes. Describe when this is logged. Once per update? Once per update w/ non-empty rice encoded additions? Same below.
CR feedback: nparker@
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:51: UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.V4DecodeRemovalsResult", result, On 2016/08/02 23:43:30, Nathan Parker wrote: > It'd be good to get some size/time metrics in here as well, in another CL. Size > of updates, time to read updates, time to unpack/write updates, etc. The update size is currently being logged as: SafeBrowsing.V4UpdateResponseSizeKB Added a TODO for measuring and recording the timings. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:130: std::unique_ptr<RepeatedField<int32>> rice_removals; On 2016/08/02 23:43:30, Nathan Parker wrote: > This can just be a RepeatedField<int32> Done. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:198: ApplyUpdateResult apply_update_result; On 2016/08/02 23:43:30, Nathan Parker wrote: > This could go inside the for loop. And actually you don't need it at all. > Since you want to return early as soon as there's an error, you could just do > that at each point. That'd reduce some of the if/else nesting. Done. Need to check return values from AddUnlumpedHashes. Removed from other palces. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:205: PrefixSize prefix_size = addition.raw_hashes().prefix_size(); On 2016/08/02 23:43:30, Nathan Parker wrote: > nit: I think this temporary doesn't add to the readability, esp since the member > name is already descriptive. Done. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:212: std::string raw_hashes_for_rice_encoded_hashes; On 2016/08/02 23:43:30, Nathan Parker wrote: > nit: Since this block of code is just for Rice encoding, you could use a shorter > var name (raw_hashes?). Done. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store_unittest.cc (right): https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:632: addition->mutable_rice_hashes()->set_num_entries(-1); On 2016/08/02 23:43:31, Nathan Parker wrote: > [tangential commentary: It's annoying that these proto fields are signed ints. > Oh well.] unsigned isn't supported in protos. Sigh. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store_unittest.cc:648: "\xbf\xa8\x3f\xfb\xf\xf\x5e\x27\xe6\xc3\x1d\xc6\x38"); On 2016/08/02 23:43:31, Nathan Parker wrote: > Add a comment describing how this was constructed, in case someone wants to make > more. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Update comment in histograms.xml
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:135: const CompressionType compression_type = removal.compression_type(); On 2016/08/02 23:43:31, Nathan Parker wrote: > nit: Not sure this temporary var helps much. Removal.compression_type() is > pretty easy to read. Done. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:141: rice_removals = base::MakeUnique<RepeatedField<int32>>(); On 2016/08/02 23:43:30, Nathan Parker wrote: > This just re-inits it to what it was before, ya? Done. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.h (right): https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.h:141: // Compression type other than RAW and RICE for additions. On 2016/08/02 23:43:31, Nathan Parker wrote: > Nit: This might be a bit more detailed error reporting than needed, since the > probability of needing to differentiate these is very small. Could be rolled > into a "invalid_update_proto_failre." But your choice. I think it may be useful in the beginning as we roll out this code to verify that things are working as expected pr to identify which parts of the update are not getting processed as expected. https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.h:254: friend class V4StoreTest; On 2016/08/02 23:43:31, Nathan Parker wrote: > Does this obviate the need for all the friends? It's a lot of friends. Unfortunately not. The individual tests are not a part of this class so making this class a friend is not enough. That's why we have to use the macro: FRIEND_TEST_ALL_PREFIXES https://codereview.chromium.org/2190233002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2190233002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:47683: + hash prefixes. On 2016/08/02 23:43:31, Nathan Parker wrote: > Describe when this is logged. Once per update? Once per update w/ non-empty > rice encoded additions? > > Same below. Done.
vakh@chromium.org changed reviewers: + pkasting@chromium.org
+pkasting for histograms.
https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:198: ApplyUpdateResult apply_update_result; On 2016/08/03 01:44:48, vakh wrote: > On 2016/08/02 23:43:30, Nathan Parker wrote: > > This could go inside the for loop. And actually you don't need it at all. > > Since you want to return early as soon as there's an error, you could just do > > that at each point. That'd reduce some of the if/else nesting. > > Done. Need to check return values from AddUnlumpedHashes. > Removed from other palces. I don't see this change -- maybe I'm looking at the wrong version?
retry upload
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...
https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:198: ApplyUpdateResult apply_update_result; On 2016/08/03 21:16:37, Nathan Parker wrote: > On 2016/08/03 01:44:48, vakh wrote: > > On 2016/08/02 23:43:30, Nathan Parker wrote: > > > This could go inside the for loop. And actually you don't need it at all. > > > Since you want to return early as soon as there's an error, you could just > do > > > that at each point. That'd reduce some of the if/else nesting. > > > > Done. Need to check return values from AddUnlumpedHashes. > > Removed from other palces. > > > I don't see this change -- maybe I'm looking at the wrong version? apply_update_result can't be removed from this method. This method can't directly return a ApplyUpdateResult value on failure, since it needs to schedule a callback on the caller's thread.
vakh@chromium.org changed reviewers: + holte@chromium.org - pkasting@chromium.org
-pkasting +holte for histograms.
Add Process{Full,Partial}Update methods to simplify ApplyUpdate and reduce
nesting
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...
Tiny: Move the declaration of hash_prefix_map closer to use
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...
Tiny: Remove a comment. Club non-static functions together in .h
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...
https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:198: ApplyUpdateResult apply_update_result; On 2016/08/03 22:44:13, vakh wrote: > On 2016/08/03 21:16:37, Nathan Parker wrote: > > On 2016/08/03 01:44:48, vakh wrote: > > > On 2016/08/02 23:43:30, Nathan Parker wrote: > > > > This could go inside the for loop. And actually you don't need it at all. > > > > > Since you want to return early as soon as there's an error, you could just > > do > > > > that at each point. That'd reduce some of the if/else nesting. > > > > > > Done. Need to check return values from AddUnlumpedHashes. > > > Removed from other palces. > > > > > > I don't see this change -- maybe I'm looking at the wrong version? > > apply_update_result can't be removed from this method. > This method can't directly return a ApplyUpdateResult value on failure, since it > needs to schedule a callback on the caller's thread. I'm confused -- I don't see any callbacks in V4Store::UpdateHashPrefixMapFromAdditions().
On 2016/08/04 00:01:09, Nathan Parker wrote: > https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... > File components/safe_browsing_db/v4_store.cc (right): > > https://codereview.chromium.org/2190233002/diff/60001/components/safe_browsin... > components/safe_browsing_db/v4_store.cc:198: ApplyUpdateResult > apply_update_result; > On 2016/08/03 22:44:13, vakh wrote: > > On 2016/08/03 21:16:37, Nathan Parker wrote: > > > On 2016/08/03 01:44:48, vakh wrote: > > > > On 2016/08/02 23:43:30, Nathan Parker wrote: > > > > > This could go inside the for loop. And actually you don't need it at > all. > > > > > > > Since you want to return early as soon as there's an error, you could > just > > > do > > > > > that at each point. That'd reduce some of the if/else nesting. > > > > > > > > Done. Need to check return values from AddUnlumpedHashes. > > > > Removed from other palces. > > > > > > > > > I don't see this change -- maybe I'm looking at the wrong version? > > > > apply_update_result can't be removed from this method. > > This method can't directly return a ApplyUpdateResult value on failure, since > it > > needs to schedule a callback on the caller's thread. > > I'm confused -- I don't see any callbacks in > V4Store::UpdateHashPrefixMapFromAdditions(). Please take a look at the latest patchset (#9). I've made some changes to improve readability, modularization and nesting.
histograms lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm LG!
The CQ bit was checked by vakh@chromium.org
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.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Rice decode removals and additions and use them to update the store. Also: 1. Add histograms for decoding additions/removals. 2. Update the histogram for ApplyUpdateResult to include the new failure values. Please note that this CL does not enable RICE encoding yet since we still telling the server that we can only process RAW updates. I'll make that tiny change as a separate CL so that it is easier to rollback, if needed. BUG=543161,624567 ========== to ========== Rice decode removals and additions and use them to update the store. Also: 1. Add histograms for decoding additions/removals. 2. Update the histogram for ApplyUpdateResult to include the new failure values. Please note that this CL does not enable RICE encoding yet since we still telling the server that we can only process RAW updates. I'll make that tiny change as a separate CL so that it is easier to rollback, if needed. BUG=543161,624567 Committed: https://crrev.com/605ab1c3447ebe4749d80c20a7d38c01a59c0887 Cr-Commit-Position: refs/heads/master@{#409844} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/605ab1c3447ebe4749d80c20a7d38c01a59c0887 Cr-Commit-Position: refs/heads/master@{#409844} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
