Parse the hash prefix map from the V4Store on disk.

Parse the hash prefix map from the V4Store on disk.

Design doc: https://goto.google.com/design-doc-v4store

BUG=543161
Committed: https://crrev.com/beda83de4a54fabf85e773068bc4a2fe816b8831
Cr-Commit-Position: refs/heads/master@{#405654}

[vakh (use Gerrit instead), 2016-07-13 23:11:57]

Add tests for valid and invalid hash prefixes read from store file on disk

[vakh (use Gerrit instead), 2016-07-13 23:33:16]

git fetch && git pull && gclient sync

[vakh (use Gerrit instead), 2016-07-13 23:33:24]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 23:34:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2016-07-13 23:34:52]

vakh@chromium.org changed reviewers:
+ nparker@chromium.org

[commit-bot: I haz the power, 2016-07-14 02:10:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 02:10:51]

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2016-07-14 22:36:12]

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_store.cc (right):

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store.cc:45:
"SafeBrowsing.V4ApplyUpdateResultWhenReadingFromDisk", result,
Is this really an update?  It's more just a load.

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store.cc:370: ApplyUpdateResult
apply_update_result = UpdateHashPrefixMapFromAdditions(
While I think this will work, it does a lot of extra work to look in the current
hash_prefix_map_ for every single hash that's getting loaded.  It would be good
to short-circuit that since we know hash_prefix_map_ is empty and there are no
removals.  This will save a chunk of CPU at startup (where it's most at a
premium).

You might want a separate function here that would just copy the proto's
string(s) straight into the map, assuming they aren't compressed.  If they are
compressed, you could do a streaming-decompress into the string owned by
hash_prefix_map_.

Or, maybe within UpdateHashPrefixMapFromAdditions() you could have
short-circuits to do this: If removals is empty and some hash_prefix_map_
entries are empty, you can just move the corresponding update result string
directly into the hash_prefix_map_. I think that may be more complicated than a
separate function. 


OH and I found some useful proto functions that let you do zero-copy add/remove
of big strings:
  void set_allocated_foo(string* value);
  string* release_foo();

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store.cc:377: hash_prefix_map_ =
read_from_disk_map;
This does a copy of the map.

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_store_unittest.cc (right):

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store_unittest.cc:359: std::unique_ptr<V4Store>
read_store(new V4Store(task_runner_, store_path_));
could put this on the stack, rather than unique_ptr.

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store_unittest.cc:362: EXPECT_EQ(2u,
read_store->hash_prefix_map_.size());
nit: ASSERT_EQ since the later checks aren't useful and may segfault if the size
is wrong.

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store_unittest.cc:367: TEST_F(V4StoreTest,
TestReadFullResponseInWithValidHashPrefixMap) {
What does the "In" in the name here mean, compared to the above function?

nit: Name is getting pretty long.  Maybe do s/HashPrefix//?

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store_unittest.cc:382:
EXPECT_EQ(HASH_PREFIX_MAP_GENERATION_FAILURE, read_store->ReadFromDisk());
Add a comment above about what makes this fail.  I can't tell by reading it.

[vakh (use Gerrit instead), 2016-07-14 23:51:48]

CR feedback: nparker@

[vakh (use Gerrit instead), 2016-07-14 23:51:51]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2016-07-14 23:51:57]

PTAL. Thanks!

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_store.cc (right):

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store.cc:370: ApplyUpdateResult
apply_update_result = UpdateHashPrefixMapFromAdditions(
On 2016/07/14 22:36:12, Nathan Parker wrote:
> While I think this will work, it does a lot of extra work to look in the
current
> hash_prefix_map_ for every single hash that's getting loaded.

Umm, that doesn't happen. I may have misunderstood you so please correct me if
that's the case.
The UpdateHashPrefixMapFromAdditions doesn't look at the hash_prefix_map_ at
all. It is a static method.

All it does is take an instance of additions() and generate a hash_prefix_map
out of it *without merging*.
(Merge is done for partial updates by MergeUpdate() method.)

>  It would be good
> to short-circuit that since we know hash_prefix_map_ is empty and there are no
> removals.  This will save a chunk of CPU at startup (where it's most at a
> premium).
> 
> You might want a separate function here that would just copy the proto's
> string(s) straight into the map, assuming they aren't compressed.  If they are
> compressed, you could do a streaming-decompress into the string owned by
> hash_prefix_map_.
> 
> Or, maybe within UpdateHashPrefixMapFromAdditions() you could have
> short-circuits to do this: If removals is empty and some hash_prefix_map_
> entries are empty, you can just move the corresponding update result string
> directly into the hash_prefix_map_. I think that may be more complicated than
a
> separate function. 

Could you be confusing UpdateHashPrefixMapFromAdditions with ApplyUpdate() or
MergeUpdate() ?

> 
> 
> OH and I found some useful proto functions that let you do zero-copy
add/remove
> of big strings:
>   void set_allocated_foo(string* value);
>   string* release_foo();
> 
> 

Thanks. I'll investigate their use and make changes in a separate CL.

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store.cc:377: hash_prefix_map_ =
read_from_disk_map;
On 2016/07/14 22:36:12, Nathan Parker wrote:
> This does a copy of the map.

Done.

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_store_unittest.cc (right):

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store_unittest.cc:359: std::unique_ptr<V4Store>
read_store(new V4Store(task_runner_, store_path_));
On 2016/07/14 22:36:12, Nathan Parker wrote:
> could put this on the stack, rather than unique_ptr.

Done.

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store_unittest.cc:362: EXPECT_EQ(2u,
read_store->hash_prefix_map_.size());
On 2016/07/14 22:36:12, Nathan Parker wrote:
> nit: ASSERT_EQ since the later checks aren't useful and may segfault if the
size
> is wrong.

Done.

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store_unittest.cc:367: TEST_F(V4StoreTest,
TestReadFullResponseInWithValidHashPrefixMap) {
On 2016/07/14 22:36:12, Nathan Parker wrote:
> What does the "In" in the name here mean, compared to the above function?
Typo. Fixed.

> 
> nit: Name is getting pretty long.  Maybe do s/HashPrefix//?
I like it this way because:
- It uses the same terms in test and product code.
- If I change here, I won't be able to resist myself from changing it
consistently in the entire file, which isn't useful.
- If, at a later time, consistency causes other tests to become too long to fit
on a single line, I'll make the change.

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store_unittest.cc:382:
EXPECT_EQ(HASH_PREFIX_MAP_GENERATION_FAILURE, read_store->ReadFromDisk());
On 2016/07/14 22:36:12, Nathan Parker wrote:
> Add a comment above about what makes this fail.  I can't tell by reading it.

Done.

[commit-bot: I haz the power, 2016-07-14 23:52:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2016-07-15 00:01:49]

Tiniest: typo: h->H

[vakh (use Gerrit instead), 2016-07-15 00:01:53]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 00:02:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2016-07-15 00:07:30]

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_store.cc (right):

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store.cc:45:
"SafeBrowsing.V4ApplyUpdateResultWhenReadingFromDisk", result,
On 2016/07/14 22:36:12, Nathan Parker wrote:
> Is this really an update?  It's more just a load.

Yes, it depends on the naming strategy in the reader's head.
I chose this name because it tells me that the result to expect is an enum
ApplyUpdateResult for the operation WhenReadingFromDisk.

The enum should probably be named differently.

What happens when reading from disk is a subset of what happens when an update
is applied so the enum is named after the super set operation ApplyUpdate.

[Nathan Parker, 2016-07-15 00:35:31]

lgtm

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_store.cc (right):

https://codereview.chromium.org/2148673002/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_store.cc:370: ApplyUpdateResult
apply_update_result = UpdateHashPrefixMapFromAdditions(
On 2016/07/14 23:51:57, vakh wrote:
> On 2016/07/14 22:36:12, Nathan Parker wrote:
> > While I think this will work, it does a lot of extra work to look in the
> current
> > hash_prefix_map_ for every single hash that's getting loaded.
> 
> Umm, that doesn't happen. I may have misunderstood you so please correct me if
> that's the case.
> The UpdateHashPrefixMapFromAdditions doesn't look at the hash_prefix_map_ at
> all. It is a static method.
> 
> All it does is take an instance of additions() and generate a hash_prefix_map
> out of it *without merging*.
> (Merge is done for partial updates by MergeUpdate() method.)
> 
> >  It would be good
> > to short-circuit that since we know hash_prefix_map_ is empty and there are
no
> > removals.  This will save a chunk of CPU at startup (where it's most at a
> > premium).
> > 
> > You might want a separate function here that would just copy the proto's
> > string(s) straight into the map, assuming they aren't compressed.  If they
are
> > compressed, you could do a streaming-decompress into the string owned by
> > hash_prefix_map_.
> > 
> > Or, maybe within UpdateHashPrefixMapFromAdditions() you could have
> > short-circuits to do this: If removals is empty and some hash_prefix_map_
> > entries are empty, you can just move the corresponding update result string
> > directly into the hash_prefix_map_. I think that may be more complicated
than
> a
> > separate function. 
> 
> Could you be confusing UpdateHashPrefixMapFromAdditions with ApplyUpdate() or
> MergeUpdate() ?

Blarg. Indeed, that's exactly how I was reading it. Sorry for the confusion,
ignore that comment.
> 
> > 
> > 
> > OH and I found some useful proto functions that let you do zero-copy
> add/remove
> > of big strings:
> >   void set_allocated_foo(string* value);
> >   string* release_foo();
> > 
> > 
> 
> Thanks. I'll investigate their use and make changes in a separate CL.

[vakh (use Gerrit instead), 2016-07-15 00:39:29]

The CQ bit was checked by vakh@chromium.org

[commit-bot: I haz the power, 2016-07-15 00:39:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 00:57:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-15 00:59:26]

Description was changed from

==========
Parse the hash prefix map from the V4Store on disk.

Design doc: https://goto.google.com/design-doc-v4store

BUG=543161
==========

to

==========
Parse the hash prefix map from the V4Store on disk.

Design doc: https://goto.google.com/design-doc-v4store

BUG=543161

Committed: https://crrev.com/beda83de4a54fabf85e773068bc4a2fe816b8831
Cr-Commit-Position: refs/heads/master@{#405654}
==========

[commit-bot: I haz the power, 2016-07-15 00:59:27]

Patchset 5 (id:??) landed as
https://crrev.com/beda83de4a54fabf85e773068bc4a2fe816b8831
Cr-Commit-Position: refs/heads/master@{#405654}