Bug 170045 – [JSC] Use WeakRandom for SamplingProfiler interval fluctuation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Use WeakRandom for SamplingProfiler interval fluctuation

[Yusuke Suzuki]

Created attachment 305264 [details]
Patch

[Mark Lam]

Comment on attachment 305264 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305264&action=review

r=me with comment.

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:279
> +    , m_weakRandom(static_cast<unsigned>(randomNumber() * (std::numeric_limits<unsigned>::max() + 1.0)))

Why the + 1.0?  I don't think it is needed.  I'm guessing you're trying to ensure a non-zero seed, but:
1. this does guarantee a non-zero seed because std::numeric_limits<unsigned>::max() + 1 is still 0, and
2. WeakRandom's setSeed() already guards against a 0 seed.

[Yusuke Suzuki]

Comment on attachment 305264 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305264&action=review

Thanks!

>> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:279
>> +    , m_weakRandom(static_cast<unsigned>(randomNumber() * (std::numeric_limits<unsigned>::max() + 1.0)))
> 
> Why the + 1.0?  I don't think it is needed.  I'm guessing you're trying to ensure a non-zero seed, but:
> 1. this does guarantee a non-zero seed because std::numeric_limits<unsigned>::max() + 1 is still 0, and
> 2. WeakRandom's setSeed() already guards against a 0 seed.

Originally, randomNumber() is created by dividing the result (uint32_t) of cryptographicallyRandomNumber() by `std::numeric_limits<unsigned>::max() + 1.0`.
Thus, to recover it to uint32_t range, we need to multiply it by `std::numeric_limits<unsigned>::max() + 1.0`.
But, maybe, using cryptographicallyRandomNumber() would be good.

Why we divide cryptographicallyRandomNumber() with `std::numeric_limits<unsigned>::max() + 1.0` in `randomNumber()` is to make the double value uniformly distributed in [0, 1.0).
It does not include 1.0, thus, we should not divide it with `std::numeric_limits<unsigned>::max()`. If cryptographicallyRandomNumber() produces UINT32_MAX, it produces 1.0.

I'll change this code to use cryptographicallyRandomNumber() directly.

[Yusuke Suzuki]

Comment on attachment 305264 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305264&action=review

>>> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:279
>>> +    , m_weakRandom(static_cast<unsigned>(randomNumber() * (std::numeric_limits<unsigned>::max() + 1.0)))
>> 
>> Why the + 1.0?  I don't think it is needed.  I'm guessing you're trying to ensure a non-zero seed, but:
>> 1. this does guarantee a non-zero seed because std::numeric_limits<unsigned>::max() + 1 is still 0, and
>> 2. WeakRandom's setSeed() already guards against a 0 seed.
> 
> Originally, randomNumber() is created by dividing the result (uint32_t) of cryptographicallyRandomNumber() by `std::numeric_limits<unsigned>::max() + 1.0`.
> Thus, to recover it to uint32_t range, we need to multiply it by `std::numeric_limits<unsigned>::max() + 1.0`.
> But, maybe, using cryptographicallyRandomNumber() would be good.
> 
> Why we divide cryptographicallyRandomNumber() with `std::numeric_limits<unsigned>::max() + 1.0` in `randomNumber()` is to make the double value uniformly distributed in [0, 1.0).
> It does not include 1.0, thus, we should not divide it with `std::numeric_limits<unsigned>::max()`. If cryptographicallyRandomNumber() produces UINT32_MAX, it produces 1.0.
> 
> I'll change this code to use cryptographicallyRandomNumber() directly.

Ah, instead, we just construct in a form `m_weakRandom()`. It will automatically use cryptographicallyRandomNumber() as its seed.

[Yusuke Suzuki]

Committed r214336: <http://trac.webkit.org/changeset/214336>