Improve base::RandBytes() performance by 1.75x-2.10x on POSIX.

Improve base::RandBytes() performance by 1.75x-2.10x on POSIX.

No real changes, just avoids doling out Uint64 sized chunks by
using the underlying method to hand out correctly sized blocks.

Windows is the only platform which doesn't have a byte stream
based generator, so I've moved the generic RandBytes() method
there and added native methods for other platforms which reuse
each platforms internal generator.

Performance measured by the new benchmark test over 5 runs,
each representing 10 generations of 1mb of random data:

Linux x64:
Original: 1199625.4
Modified: 686480.2
Improvement: 1.75x

On OSX (10.9.1):
Original: 1532669.8
Modified: 734808.0
Improvement: 2.10x

BUG=none
TEST=new benchmark unittest.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246486

[DaleCurtis, 2014-01-17 23:27:12]

Could be implemented on Windows by using the RtlGenRandom() method,
but that seems to require a dynamic load of advapi32.dll...

http://msdn.microsoft.com/en-us/library/aa387694.aspx

[DaleCurtis, 2014-01-17 23:31:24]

Over to thakis@ since ajwong@ is OOO for a while.

[DaleCurtis, 2014-01-17 23:45:41]

+rsleevi since RandBytes() passes through to crypto/random.{h,cc}

[Ryan Sleevi, 2014-01-18 00:01:15]

rand_s is implemented by using RtlGenRandom in MSVCRT, so you already have the
dependency on advapi32.dll.

I'm bouncing to Justin, but it seems like we really should just use RtlGenRandom
on Windows if we're using /dev/urandom on Linux. The only benefit to using the
CRT's rand is if we wanted an explicit seed, but nothing in Chrome should be
relying on that explicit seed for base::RandBytes. Justin, any objections here?

[DaleCurtis, 2014-01-18 00:25:08]

Using RtlGenRandom directly yielded a massive 44x improvement for larger buffer
sizes!  That CL is here https://codereview.chromium.org/141753009/ -- I can
merge it into this one if preferred.

[DaleCurtis, 2014-01-22 20:59:43]

jschuh, thakis: Ping?

[Nico, 2014-01-22 21:03:29]

lgtm

Nice!

https://codereview.chromium.org/140773006/diff/100001/base/rand_util_posix.cc
File base/rand_util_posix.cc (right):

https://codereview.chromium.org/140773006/diff/100001/base/rand_util_posix.cc...
base/rand_util_posix.cc:46: ReadFromFD(urandom_fd,
reinterpret_cast<char*>(&number), sizeof(number)));
nit: I liked the `bool success`, stuff with side effects in CHECKs always is a
bit hairy (someone might change it to a DCHECK and then the side effect won't
happen on official builds)

[Nico, 2014-01-22 21:04:12]

(And keeping the windows cl separate is preferable, I think.)


On Wed, Jan 22, 2014 at 1:03 PM, <thakis@chromium.org> wrote:

> lgtm
>
> Nice!
>
>
> https://codereview.chromium.org/140773006/diff/100001/
> base/rand_util_posix.cc
> File base/rand_util_posix.cc (right):
>
> https://codereview.chromium.org/140773006/diff/100001/
> base/rand_util_posix.cc#newcode46
> base/rand_util_posix.cc:46: ReadFromFD(urandom_fd,
> reinterpret_cast<char*>(&number), sizeof(number)));
> nit: I liked the `bool success`, stuff with side effects in CHECKs
> always is a bit hairy (someone might change it to a DCHECK and then the
> side effect won't happen on official builds)
>
> https://codereview.chromium.org/140773006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2014-01-22 21:12:17]

https://codereview.chromium.org/140773006/diff/100001/base/rand_util_posix.cc
File base/rand_util_posix.cc (right):

https://codereview.chromium.org/140773006/diff/100001/base/rand_util_posix.cc...
base/rand_util_posix.cc:46: ReadFromFD(urandom_fd,
reinterpret_cast<char*>(&number), sizeof(number)));
On 2014/01/22 21:03:30, Nico wrote:
> nit: I liked the `bool success`, stuff with side effects in CHECKs always is a
> bit hairy (someone might change it to a DCHECK and then the side effect won't
> happen on official builds)

Reverted. I also realized this function can simplified to RandBytes(&number,
sizeof(number)).

[jschuh, 2014-01-22 21:15:20]

D'oh, sorry. This was on my list of things to fix, so thanks for taking care of
it. lgtm

[DaleCurtis, 2014-01-22 21:38:19]

Looks like the NaCl interface is actually size_t, so the chunking code can be
removed:

https://code.google.com/p/chromium/codesearch#chromium/src/native_client/src/...

+bradnelson from NaCl team to confirm since he reviewed the original change.

[bradn, 2014-01-22 21:56:12]

+wtc

lgtm on the nacl part.

I had a question on windows.
I've adde wtc as his name came up when looking into RtlGenRandom vs
CryptGenRandom.

If you'd like to worry about windows separately, that's ok too.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_win.cc
File base/rand_util_win.cc (right):

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_win.cc#n...
base/rand_util_win.cc:32: uint64 random_int;
Any reason not to use:
CryptGenRandom ?
http://msdn.microsoft.com/en-us/library/aa379942.aspx

We appear to use it already in a few places:
  crypto/symmetric_key_win.cc
  third_party/tlslite/tlslite/utils/entropy.c

[DaleCurtis, 2014-01-22 21:59:00]

Lets move the Windows discussion to this CL:
https://codereview.chromium.org/141753009/

I've added you and wtc there.

[DaleCurtis, 2014-01-22 22:03:13]

CQ'ing this one.  Thanks for the reviews everyone.

[commit-bot: I haz the power, 2014-01-22 22:03:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/140773006/210001

[wtc, 2014-01-22 22:29:08]

Patch set 5 LGTM. Please fix the coding style and comment nits before you commit
this.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_nacl.cc
File base/rand_util_nacl.cc (right):

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_nacl.cc#...
base/rand_util_nacl.cc:46: // NOTE: This function must be cryptographically
secure. http://crbug.com/140076

This comment actually contradicts the comment for RandBytes in base/rand_util.h:

  // Fills |output_length| bytes of |output| with random data.
  //
  // WARNING:
  // Do not use for security-sensitive purposes.
  // See crypto/ for cryptographically secure random number generation APIs.
  BASE_EXPORT void RandBytes(void* output, size_t output_length);

Please move this comment to the front of RandUint64().

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_nacl.cc#...
base/rand_util_nacl.cc:47: void RandBytes(void* output, size_t output_length) {

RandBytes should be defined after RandUint64 to match the declaration order in
the .h file. This is recommended by the Style Guide.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_posix.cc
File base/rand_util_posix.cc (right):

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_posix.cc...
base/rand_util_posix.cc:41: // NOTE: This function must be cryptographically
secure. http://crbug.com/140076

This comment should be in front of RandUint64(). I think this comment doesn't
make sense, but you should avoid dealing with this issue in this CL.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_posix.cc...
base/rand_util_posix.cc:42: void RandBytes(void* output, size_t output_length) {

Define RandBytes after RandUint64, as the Style Guide recommends.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_unittest.cc
File base/rand_util_unittest.cc (right):

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_unittest...
base/rand_util_unittest.cc:127: TEST(RandUtilTest, DISABLED_RandBytesPerf) {

We should document why this test is DISABLED.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_win.cc
File base/rand_util_win.cc (right):

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_win.cc#n...
base/rand_util_win.cc:32: uint64 random_int;

On 2014/01/22 21:56:12, bradn wrote:
> Any reason not to use:
> CryptGenRandom ?
> http://msdn.microsoft.com/en-us/library/aa379942.aspx

There is a better Windows function: RtlGenRandom. Using RtlGenRandom will be
more efficient. But it is fine to use the current code as a starting point.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_win.cc#n...
base/rand_util_win.cc:36: size_t copy_count = std::min(output_length - i,
random_int_size);

|copy_count| can also be 'const'. (I don't do this myself, but since I noticed
you like to use 'const', I thought I should mention it.)

[DaleCurtis, 2014-01-22 22:36:26]

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_nacl.cc
File base/rand_util_nacl.cc (right):

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_nacl.cc#...
base/rand_util_nacl.cc:46: // NOTE: This function must be cryptographically
secure. http://crbug.com/140076
On 2014/01/22 22:29:09, wtc wrote:
> 
> This comment actually contradicts the comment for RandBytes in
base/rand_util.h:
> 
>   // Fills |output_length| bytes of |output| with random data.
>   //
>   // WARNING:
>   // Do not use for security-sensitive purposes.
>   // See crypto/ for cryptographically secure random number generation APIs.
>   BASE_EXPORT void RandBytes(void* output, size_t output_length);
> 
> Please move this comment to the front of RandUint64().

Are you okay with moving this comment into the rand_util.h file for
RandUint64(), RandBytes(), and RandBytesAsString()? Would you like it duplicated
on each method for safety's sake?

[wtc, 2014-01-22 22:47:11]

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_nacl.cc
File base/rand_util_nacl.cc (right):

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_nacl.cc#...
base/rand_util_nacl.cc:46: // NOTE: This function must be cryptographically
secure. http://crbug.com/140076

On 2014/01/22 22:36:26, DaleCurtis wrote:
>
> Are you okay with moving this comment into the rand_util.h file for
> RandUint64(), RandBytes(), and RandBytesAsString()? Would you like it
duplicated
> on each method for safety's sake?

I think this comment doesn't make sense. The purpose of your CL is to improve
performance. Your CL should stay focused on that. So I suggest you not touch the
comment. Since this comment wasn't in this file before, I think you do not need
to add it.

It is a good idea to clean up the comments, but it's better to do that in a
separate CL.

[DaleCurtis, 2014-01-22 22:54:14]

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_nacl.cc
File base/rand_util_nacl.cc (right):

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_nacl.cc#...
base/rand_util_nacl.cc:46: // NOTE: This function must be cryptographically
secure. http://crbug.com/140076
On 2014/01/22 22:47:12, wtc wrote:
> 
> On 2014/01/22 22:36:26, DaleCurtis wrote:
> >
> > Are you okay with moving this comment into the rand_util.h file for
> > RandUint64(), RandBytes(), and RandBytesAsString()? Would you like it
> duplicated
> > on each method for safety's sake?
> 
> I think this comment doesn't make sense. The purpose of your CL is to improve
> performance. Your CL should stay focused on that. So I suggest you not touch
the
> comment. Since this comment wasn't in this file before, I think you do not
need
> to add it.
> 
> It is a good idea to clean up the comments, but it's better to do that in a
> separate CL.

Heh, fair enough. All changes to it reverted.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_nacl.cc#...
base/rand_util_nacl.cc:47: void RandBytes(void* output, size_t output_length) {
On 2014/01/22 22:29:09, wtc wrote:
> 
> RandBytes should be defined after RandUint64 to match the declaration order in
> the .h file. This is recommended by the Style Guide.

Done.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_posix.cc
File base/rand_util_posix.cc (right):

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_posix.cc...
base/rand_util_posix.cc:41: // NOTE: This function must be cryptographically
secure. http://crbug.com/140076
On 2014/01/22 22:29:09, wtc wrote:
> 
> This comment should be in front of RandUint64(). I think this comment doesn't
> make sense, but you should avoid dealing with this issue in this CL.

Done.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_posix.cc...
base/rand_util_posix.cc:42: void RandBytes(void* output, size_t output_length) {
On 2014/01/22 22:29:09, wtc wrote:
> 
> Define RandBytes after RandUint64, as the Style Guide recommends.

Done.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_unittest.cc
File base/rand_util_unittest.cc (right):

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_unittest...
base/rand_util_unittest.cc:127: TEST(RandUtilTest, DISABLED_RandBytesPerf) {
On 2014/01/22 22:29:09, wtc wrote:
> 
> We should document why this test is DISABLED.

Done.

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_win.cc
File base/rand_util_win.cc (right):

https://codereview.chromium.org/140773006/diff/210001/base/rand_util_win.cc#n...
base/rand_util_win.cc:36: size_t copy_count = std::min(output_length - i,
random_int_size);
On 2014/01/22 22:29:09, wtc wrote:
> 
> |copy_count| can also be 'const'. (I don't do this myself, but since I noticed
> you like to use 'const', I thought I should mention it.)

Done.

[wtc, 2014-01-22 23:01:43]

Patch set 6 LGTM. Thanks.

[commit-bot: I haz the power, 2014-01-22 23:04:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/140773006/470001

[commit-bot: I haz the power, 2014-01-23 02:14:30]

Change committed as 246486