Supervised user whitelists optimization: Store raw sha1 hashes rather than strings

Supervised user whitelists optimization: Store raw sha1 hashes rather than strings

BUG=

Committed: https://crrev.com/b452a96aff5dbe94bd93f399e91d9af398d946a4
Cr-Commit-Position: refs/heads/master@{#362971}

[Marc Treib, 2015-11-23 12:04:12]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2015-11-23 12:04:13]

As discussed on https://codereview.chromium.org/1443033004/

This needs some rather yucky low-level stuff, and TBH I'm not convinced it's
worth it - WDYT?

[Marc Treib, 2015-11-26 08:47:22]

On 2015/11/23 12:04:13, Marc Treib wrote:
> As discussed on https://codereview.chromium.org/1443033004/
> 
> This needs some rather yucky low-level stuff, and TBH I'm not convinced it's
> worth it - WDYT?

Right now, there's a discussion on cxx@chromium.org about allowing std::array.
Might be worth waiting for that.

[Bernhard Bauer, 2015-12-01 12:42:32]

On 2015/11/26 08:47:22, Marc Treib wrote:
> On 2015/11/23 12:04:13, Marc Treib wrote:
> > As discussed on https://codereview.chromium.org/1443033004/
> > 
> > This needs some rather yucky low-level stuff, and TBH I'm not convinced it's
> > worth it - WDYT?
> 
> Right now, there's a discussion on mailto:cxx@chromium.org about allowing
std::array.
> Might be worth waiting for that.

Looks like there are no objections to using std::array, but TBH I'd also be okay
with using a std::vector<uint8> instead.

https://codereview.chromium.org/1469813002/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_site_list.h (right):

https://codereview.chromium.org/1469813002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_site_list.h:49: uint8
bytes_[base::kSHA1Length];
DISALLOW_COPY_AND_ASSIGN? Or add a comment that copying and assignment are
allowed, so that it doesn't look like an oversight.

https://codereview.chromium.org/1469813002/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_url_filter.cc (right):

https://codereview.chromium.org/1469813002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_url_filter.cc:38: struct
HashHostnameHash {
Should we move this into HostnameHash? Then we don't need to expose the data()
method anymore.

https://codereview.chromium.org/1469813002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_url_filter.cc:40: return
*reinterpret_cast<const size_t*>(value.data());
This will effectively return the first 4 or 8 bytes (which are already randomly
distributed because hash), right? Maybe add a comment to explain that?

[Marc Treib, 2015-12-03 11:21:11]

IMO if we go to the trouble of storing the hashes properly, then we should also
avoid the 16-byte overhead for pointer+size, and the extra indirection. So
std::array it is, PTAL!

https://codereview.chromium.org/1469813002/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_site_list.h (right):

https://codereview.chromium.org/1469813002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_site_list.h:49: uint8
bytes_[base::kSHA1Length];
On 2015/12/01 12:42:32, Bernhard Bauer wrote:
> DISALLOW_COPY_AND_ASSIGN? Or add a comment that copying and assignment are
> allowed, so that it doesn't look like an oversight.

Yeah, I meant to allow copying here. Comment added.

https://codereview.chromium.org/1469813002/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_url_filter.cc (right):

https://codereview.chromium.org/1469813002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_url_filter.cc:38: struct
HashHostnameHash {
On 2015/12/01 12:42:32, Bernhard Bauer wrote:
> Should we move this into HostnameHash? Then we don't need to expose the data()
> method anymore.

Done. (We still need the functor struct, but now it just calls .hash())

https://codereview.chromium.org/1469813002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_url_filter.cc:40: return
*reinterpret_cast<const size_t*>(value.data());
On 2015/12/01 12:42:32, Bernhard Bauer wrote:
> This will effectively return the first 4 or 8 bytes (which are already
randomly
> distributed because hash), right? Maybe add a comment to explain that?

Done.

[Bernhard Bauer, 2015-12-03 11:43:21]

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_site_list.cc (right):

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_site_list.cc:52:
DCHECK_GE(bytes.size(), base::kSHA1Length);
Would you mind making this a CHECK just to be a bit safer about accessing memory
out of bounds in a release build?

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_site_list.cc:53:
std::copy_n(bytes.begin(), base::kSHA1Length, bytes_.begin());
Actually, is copy_n already allowed? I can't find it (positive or negative) on
https://chromium-cpp.appspot.com/.

Or maybe just use std::copy with begin() and end()?

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_site_list.cc:108:
DCHECK(hash_bytes.size() == base::kSHA1Length);
DCHECK_EQ?

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_url_filter.cc (right):

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_url_filter.cc:38: struct
HashHostnameHash {
Move this into an anonymous namespace.

(I think it will have to stay before the definition of
SupervisedUserURLFilter::Contents, but otherwise it should be possible to
reference a type from an anonymous namespace in it, right?)

[Marc Treib, 2015-12-03 12:24:39]

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

[commit-bot: I haz the power, 2015-12-03 12:25:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1469813002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1469813002/40001

[Marc Treib, 2015-12-03 12:25:11]

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_site_list.cc (right):

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_site_list.cc:52:
DCHECK_GE(bytes.size(), base::kSHA1Length);
On 2015/12/03 11:43:21, Bernhard Bauer wrote:
> Would you mind making this a CHECK just to be a bit safer about accessing
memory
> out of bounds in a release build?

Hm... sure, I guess. Done.

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_site_list.cc:53:
std::copy_n(bytes.begin(), base::kSHA1Length, bytes_.begin());
On 2015/12/03 11:43:21, Bernhard Bauer wrote:
> Actually, is copy_n already allowed? I can't find it (positive or negative) on
> https://chromium-cpp.appspot.com/.
> 
> Or maybe just use std::copy with begin() and end()?

Hm, <algorithms> is still under "to be discussed". TBH I didn't notice that
copy_n was only added in C++11.
Anyway, regular copy will do just as well.

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_site_list.cc:108:
DCHECK(hash_bytes.size() == base::kSHA1Length);
On 2015/12/03 11:43:21, Bernhard Bauer wrote:
> DCHECK_EQ?

Done.

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_url_filter.cc (right):

https://codereview.chromium.org/1469813002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_url_filter.cc:38: struct
HashHostnameHash {
On 2015/12/03 11:43:21, Bernhard Bauer wrote:
> Move this into an anonymous namespace.
> 
> (I think it will have to stay before the definition of
> SupervisedUserURLFilter::Contents, but otherwise it should be possible to
> reference a type from an anonymous namespace in it, right?)

Done.

[Bernhard Bauer, 2015-12-03 12:27:10]

LGTM, thanks!

[commit-bot: I haz the power, 2015-12-03 13:15:29]

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

[commit-bot: I haz the power, 2015-12-03 13:15:29]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2015-12-03 13:19:45]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-12-03 13:20:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1469813002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1469813002/40001

[commit-bot: I haz the power, 2015-12-03 13:25:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-03 13:26:17]

Description was changed from

==========
Supervised user whitelists optimization: Store raw sha1 hashes rather than
strings

BUG=
==========

to

==========
Supervised user whitelists optimization: Store raw sha1 hashes rather than
strings

BUG=

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

[commit-bot: I haz the power, 2015-12-03 13:26:18]

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