Optimize the Math.random implementation #1171
Conversation
|
Hi @woshiccm! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
@tmikov @psaha-rbi pls help to review, thanks |
lib/VM/JSLib/Math.cpp
Outdated
| if (!storage->randomEngineInited) { | ||
| struct timeval tv; | ||
| gettimeofday(&tv, NULL); | ||
| storage->randomState_ = static_cast<uint64_t>((tv.tv_sec * SECONDS_TO_SUBTLE) + tv.tv_usec); |
There was a problem hiding this comment.
I think this is problematic because
- clock resolution maybe more coarse grained than microseconds. e.g. on Linux it is usually 10us.
- Even if there is perfect microsecond granularity, you'll start getting collisions if thousands of uuids are generated every second across many devices since many of them will get the same read of microsecond. In general, even though the "microseconds since epoch" is a large value it has very strong temporal locality in the sense that at any point in time all values will be tightly clustered.
- I also feel not so warm about rolling a custom RNG where there are so many available in the standard library specially, without running any of the standard randomness test suites on it.
I'd recommend using std::random_device()() to generate multiple 32-bit numbers and concatenating those to form a 64 bit seed or better yet, use a std::seed_seq() with enough members to initialize a Mersenne twister to get a very long period.
|
Closing since we merged 34f0b2b instead. |
Summary: Fix The Math.random implementation in Hermes is collision prone for the App use case