[webkit-reviews] review denied: [Bug 111533] Replace Mersenne Twister RNG with a simple but fast RNG : [Attachment 191672] Patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied  review:
Bug 111533: Replace Mersenne Twister RNG with a simple but fast RNG
https://bugs.webkit.org/show_bug.cgi?id=111533

Attachment 191672: Patch
https://bugs.webkit.org/attachment.cgi?id=191672&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=191672&action=review


Don't use the g_ prefix on the global (we tend to use s_ in general, but also
it's not exported from RandomNumber.cpp so you probably don't need a prefix
anyway.

That said i think this whole thing should be in just the cpp file, there's no
need to have any initializer see my comments inline.

> Source/WTF/wtf/RandomNumber.cpp:49
> +namespace Internal {
> +
> +static uint64_t g_state;
> +
> +void initializeRandomNumber(uint64_t seed)
> +{
> +    g_state = seed;
>  }

Remove these changes, and remove g_state

> Source/WTF/wtf/RandomNumber.cpp:62
> +uint32_t randomNumber()
> +{
> +    g_state += ((g_state * g_state) | 5);
> +    return static_cast<uint32_t>(g_state >> 32);
> +}

uint32_t randomNumber()
{
    // Don't use rand() as your seed here, it's been demonstrated that you can
rollback to find the initial seed
    // I'd suggest a combo of time() + ASLR sourced entropy to try and make
things less deterministic, a la:
    static uint64_t state = (reinterpret_cast<uintptr_t>(&state) >> 5) ^
time(0) ^ (reinterpret_cast<uintptr_t>(&state) >> 13);
    state += ((state * state) | 5);
    return static_cast<uint32_t>(state >> 32);
}

> Source/WTF/wtf/RandomNumber.h:35
> +#if !USE(OS_RANDOMNESS)
> +namespace Internal {
> +void initializeRandomNumber(uint64_t);
> +}
> +#endif

remove this, and so reset RendomNumber.h you don't need changes.

> Source/WTF/wtf/RandomNumberSeed.h:65
> -#if USE(MERSENNE_TWISTER_19937)
> -    // use rand() to initialize the Mersenne Twister random number
generator.
> -    unsigned long initializationBuffer[4];
> -    initializationBuffer[0] = (rand() << 16) | rand();
> -    initializationBuffer[1] = (rand() << 16) | rand();
> -    initializationBuffer[2] = (rand() << 16) | rand();
> -    initializationBuffer[3] = (rand() << 16) | rand();
> -    init_by_array(initializationBuffer, 4);
> +#if !USE(OS_RANDOMNESS)
> +    uint64_t seed = static_cast<uint64_t>(rand()) << 32 |
static_cast<uint64_t>(rand());
> +    Internal::initializeRandomNumber(seed);
>  #endif

You can remove all this explicit initialization