Introduce a RandonNumberGenerator class. Refactor the random/private_random uses in Isolate/Context.

Introduce a RandonNumberGenerator class. Refactor the random/private_random uses in Isolate/Context.

The RandomNumberGenerator is a pseudorandom number generator
with 48-bit state. It is properly seeded using either

(1) the --random-seed if specified, or
(2) the entropy_source function if configured, or
(3) /dev/urandom if available, or
(4) falls back to Time and TimeTicks based seeding.

Each Isolate now contains a RandomNumberGenerator, which replaces
the previous private_random_seed.

Every native context still has its own random_seed. But this random
seed is now properly initialized during bootstrapping,
instead of on-demand initialization. This will allow us to cleanup
and speedup the HRandom implementation quite a lot (this is delayed
for a followup CL)!

Also stop messing with the system rand()/random(), which should
not be done from a library anyway! We probably re-seeded the
libc rand()/random() after the application (i.e. Chrome) already
seeded it (with better entropy than what we used).

Another followup CL will replace the use of the per-isolate
random number generator for the address randomization and
thereby get rid of the Isolate::UncheckedCurrent() usage in
the platform code.

TEST=cctest/test-random-number-generator,cctest/test-random
R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16612

[Benedikt Meurer, 2013-09-09 14:16:09]

Hey Michi,
Here's the random cleanup. PTAL
-- Benedikt

[Michael Starzinger, 2013-09-09 17:11:25]

https://codereview.chromium.org/23548024/diff/4001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/23548024/diff/4001/src/bootstrapper.cc#newcod...
src/bootstrapper.cc:1320: Handle<ByteArray>
random_seed(factory->NewByteArray(kRandomStateSize));
nit: Use assignment instead of copy constructor.

https://codereview.chromium.org/23548024/diff/4001/src/bootstrapper.cc#newcod...
src/bootstrapper.cc:2410: // using the per-isolate random number generator.
It seems "ConfigureGlobalObjects" is a bad name for this initialization, can we
either move it into Genesis::Genesis directly (i.e. after experimental natives
are initialized)?

https://codereview.chromium.org/23548024/diff/4001/src/flags.h
File src/flags.h (right):

https://codereview.chromium.org/23548024/diff/4001/src/flags.h#newcode31
src/flags.h:31: #include "atomicops.h"
I am not sure why this header needs the atomicops.h include. Is it just to break
some weird circular dependency?

https://codereview.chromium.org/23548024/diff/4001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/23548024/diff/4001/src/heap.cc#newcode6933
src/heap.cc:6933: isolate()->random_number_generator()->NextInt() &
nit: Indentation is off.

https://codereview.chromium.org/23548024/diff/4001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/23548024/diff/4001/src/isolate.h#newcode1129
src/isolate.h:1129: inline RandomNumberGenerator* random_number_generator()
V8_WARN_UNUSED_RESULT;
I don't think it is vitally important that the result of this simple getter is
actually used. The world is not going to end if someone calls the getter without
using the result. Can we please keep this line readable and drop the
V8_WARN_UNUSED_RESULT? Unless I am missing a whole class of bugs that will
actually be caught by this warning.

https://codereview.chromium.org/23548024/diff/4001/src/platform/time.h
File src/platform/time.h (right):

https://codereview.chromium.org/23548024/diff/4001/src/platform/time.h#newcod...
src/platform/time.h:227: static Time FromInternalValue(int64_t value)
V8_WARN_UNUSED_RESULT {
Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

https://codereview.chromium.org/23548024/diff/4001/src/platform/time.h#newcod...
src/platform/time.h:230: int64_t ToInternalValue() const V8_WARN_UNUSED_RESULT {
Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

https://codereview.chromium.org/23548024/diff/4001/src/platform/time.h#newcod...
src/platform/time.h:343: static TimeTicks FromInternalValue(int64_t value)
V8_WARN_UNUSED_RESULT {
Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

https://codereview.chromium.org/23548024/diff/4001/src/platform/time.h#newcod...
src/platform/time.h:346: int64_t ToInternalValue() const V8_WARN_UNUSED_RESULT {
Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

https://codereview.chromium.org/23548024/diff/4001/src/v8.cc
File src/v8.cc (right):

https://codereview.chromium.org/23548024/diff/4001/src/v8.cc#newcode118
src/v8.cc:118: // When we get, the RNG must have been initialized, see
nit: s/get/get here/

[Benedikt Meurer, 2013-09-10 06:08:07]

PTAL

https://codereview.chromium.org/23548024/diff/4001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/23548024/diff/4001/src/bootstrapper.cc#newcod...
src/bootstrapper.cc:1320: Handle<ByteArray>
random_seed(factory->NewByteArray(kRandomStateSize));
On 2013/09/09 17:11:25, Michael Starzinger wrote:
> nit: Use assignment instead of copy constructor.

Done.

https://codereview.chromium.org/23548024/diff/4001/src/bootstrapper.cc#newcod...
src/bootstrapper.cc:2410: // using the per-isolate random number generator.
On 2013/09/09 17:11:25, Michael Starzinger wrote:
> It seems "ConfigureGlobalObjects" is a bad name for this initialization, can
we
> either move it into Genesis::Genesis directly (i.e. after experimental natives
> are initialized)?

Just this piece of code, or the whole ConfigureGlobalObjects?

https://codereview.chromium.org/23548024/diff/4001/src/flags.h
File src/flags.h (right):

https://codereview.chromium.org/23548024/diff/4001/src/flags.h#newcode31
src/flags.h:31: #include "atomicops.h"
On 2013/09/09 17:11:25, Michael Starzinger wrote:
> I am not sure why this header needs the atomicops.h include. Is it just to
break
> some weird circular dependency?

Because flag-definitions.h uses AtomicWord.

https://codereview.chromium.org/23548024/diff/4001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/23548024/diff/4001/src/heap.cc#newcode6933
src/heap.cc:6933: isolate()->random_number_generator()->NextInt() &
On 2013/09/09 17:11:25, Michael Starzinger wrote:
> nit: Indentation is off.

Done.

https://codereview.chromium.org/23548024/diff/4001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/23548024/diff/4001/src/isolate.h#newcode1129
src/isolate.h:1129: inline RandomNumberGenerator* random_number_generator()
V8_WARN_UNUSED_RESULT;
On 2013/09/09 17:11:25, Michael Starzinger wrote:
> I don't think it is vitally important that the result of this simple getter is
> actually used. The world is not going to end if someone calls the getter
without
> using the result. Can we please keep this line readable and drop the
> V8_WARN_UNUSED_RESULT? Unless I am missing a whole class of bugs that will
> actually be caught by this warning.

Done.

https://codereview.chromium.org/23548024/diff/4001/src/platform/time.h
File src/platform/time.h (right):

https://codereview.chromium.org/23548024/diff/4001/src/platform/time.h#newcod...
src/platform/time.h:227: static Time FromInternalValue(int64_t value)
V8_WARN_UNUSED_RESULT {
On 2013/09/09 17:11:25, Michael Starzinger wrote:
> Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

Done.

https://codereview.chromium.org/23548024/diff/4001/src/platform/time.h#newcod...
src/platform/time.h:230: int64_t ToInternalValue() const V8_WARN_UNUSED_RESULT {
On 2013/09/09 17:11:25, Michael Starzinger wrote:
> Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

Done.

https://codereview.chromium.org/23548024/diff/4001/src/platform/time.h#newcod...
src/platform/time.h:343: static TimeTicks FromInternalValue(int64_t value)
V8_WARN_UNUSED_RESULT {
On 2013/09/09 17:11:25, Michael Starzinger wrote:
> Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

Done.

https://codereview.chromium.org/23548024/diff/4001/src/platform/time.h#newcod...
src/platform/time.h:346: int64_t ToInternalValue() const V8_WARN_UNUSED_RESULT {
On 2013/09/09 17:11:25, Michael Starzinger wrote:
> Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

Done.

https://codereview.chromium.org/23548024/diff/4001/src/v8.cc
File src/v8.cc (right):

https://codereview.chromium.org/23548024/diff/4001/src/v8.cc#newcode118
src/v8.cc:118: // When we get, the RNG must have been initialized, see
On 2013/09/09 17:11:25, Michael Starzinger wrote:
> nit: s/get/get here/

Done.

[Michael Starzinger, 2013-09-10 08:43:31]

LGTM (if comment in bootstrapper.cc is addressed).

https://codereview.chromium.org/23548024/diff/4001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/23548024/diff/4001/src/bootstrapper.cc#newcod...
src/bootstrapper.cc:2410: // using the per-isolate random number generator.
On 2013/09/10 06:08:07, Benedikt Meurer wrote:
> On 2013/09/09 17:11:25, Michael Starzinger wrote:
> > It seems "ConfigureGlobalObjects" is a bad name for this initialization, can
> we
> > either move it into Genesis::Genesis directly (i.e. after experimental
natives
> > are initialized)?
> 
> Just this piece of code, or the whole ConfigureGlobalObjects?

Just this new piece of initialization.

https://codereview.chromium.org/23548024/diff/4001/src/flags.h
File src/flags.h (right):

https://codereview.chromium.org/23548024/diff/4001/src/flags.h#newcode31
src/flags.h:31: #include "atomicops.h"
On 2013/09/10 06:08:07, Benedikt Meurer wrote:
> On 2013/09/09 17:11:25, Michael Starzinger wrote:
> > I am not sure why this header needs the atomicops.h include. Is it just to
> break
> > some weird circular dependency?
> 
> Because flag-definitions.h uses AtomicWord.

I see, didn't know that.

https://codereview.chromium.org/23548024/diff/15001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/23548024/diff/15001/src/api.cc#newcode67
src/api.cc:67: #include "utils/random-number-generator.h"
nit: Can we alpha-sort the includes?

https://codereview.chromium.org/23548024/diff/15001/src/utils/random-number-g...
File src/utils/random-number-generator.h (right):

https://codereview.chromium.org/23548024/diff/15001/src/utils/random-number-g...
src/utils/random-number-generator.h:62: V8_INLINE(int NextInt())
V8_WARN_UNUSED_RESULT {
nit: Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

https://codereview.chromium.org/23548024/diff/15001/src/utils/random-number-g...
src/utils/random-number-generator.h:72: int NextInt(int max)
V8_WARN_UNUSED_RESULT;
nit: Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

https://codereview.chromium.org/23548024/diff/15001/src/utils/random-number-g...
src/utils/random-number-generator.h:79: V8_INLINE(bool NextBool())
V8_WARN_UNUSED_RESULT {
nit: Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

https://codereview.chromium.org/23548024/diff/15001/src/utils/random-number-g...
src/utils/random-number-generator.h:88: double NextDouble()
V8_WARN_UNUSED_RESULT;
nit: Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

[Benedikt Meurer, 2013-09-10 10:52:13]

https://codereview.chromium.org/23548024/diff/4001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/23548024/diff/4001/src/bootstrapper.cc#newcod...
src/bootstrapper.cc:2410: // using the per-isolate random number generator.
On 2013/09/10 08:43:31, Michael Starzinger wrote:
> On 2013/09/10 06:08:07, Benedikt Meurer wrote:
> > On 2013/09/09 17:11:25, Michael Starzinger wrote:
> > > It seems "ConfigureGlobalObjects" is a bad name for this initialization,
can
> > we
> > > either move it into Genesis::Genesis directly (i.e. after experimental
> natives
> > > are initialized)?
> > 
> > Just this piece of code, or the whole ConfigureGlobalObjects?
> 
> Just this new piece of initialization.

Done.

https://codereview.chromium.org/23548024/diff/15001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/23548024/diff/15001/src/api.cc#newcode67
src/api.cc:67: #include "utils/random-number-generator.h"
On 2013/09/10 08:43:31, Michael Starzinger wrote:
> nit: Can we alpha-sort the includes?

Done.

https://codereview.chromium.org/23548024/diff/15001/src/utils/random-number-g...
File src/utils/random-number-generator.h (right):

https://codereview.chromium.org/23548024/diff/15001/src/utils/random-number-g...
src/utils/random-number-generator.h:62: V8_INLINE(int NextInt())
V8_WARN_UNUSED_RESULT {
On 2013/09/10 08:43:31, Michael Starzinger wrote:
> nit: Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

As per offline discussion: This is a valid use.

https://codereview.chromium.org/23548024/diff/15001/src/utils/random-number-g...
src/utils/random-number-generator.h:72: int NextInt(int max)
V8_WARN_UNUSED_RESULT;
On 2013/09/10 08:43:31, Michael Starzinger wrote:
> nit: Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

As per offline discussion: This is a valid use.

https://codereview.chromium.org/23548024/diff/15001/src/utils/random-number-g...
src/utils/random-number-generator.h:79: V8_INLINE(bool NextBool())
V8_WARN_UNUSED_RESULT {
On 2013/09/10 08:43:31, Michael Starzinger wrote:
> nit: Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

As per offline discussion: This is a valid use.

https://codereview.chromium.org/23548024/diff/15001/src/utils/random-number-g...
src/utils/random-number-generator.h:88: double NextDouble()
V8_WARN_UNUSED_RESULT;
On 2013/09/10 08:43:31, Michael Starzinger wrote:
> nit: Ceterum censeo V8_WARN_UNUSED_RESULT esse delendam.

As per offline discussion: This is a valid use.

[Benedikt Meurer, 2013-09-10 11:14:28]

Committed patchset #6 manually as r16612 (presubmit successful).