-
-
Notifications
You must be signed in to change notification settings - Fork 949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: replace MersenneTwister implementation with a pure-rand based one #3288
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #3288 +/- ##
==========================================
+ Coverage 99.96% 99.97% +0.01%
==========================================
Files 2806 2806
Lines 217140 217089 -51
Branches 980 973 -7
==========================================
- Hits 217061 217034 -27
+ Misses 79 55 -24
|
Clearly ok for me 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely an approval from my side, because it looks like that no snapshot were altered and so it is a "one to one move forward"
Could there be some logic in adding pure-rand as a devDependency and adding a few tests that show that the output of our function is identical to the output from pure-rand? To guard against any possible divergence in future. |
That's not what we are aiming for here. Our goal is to have a faster implementation of mersenne. We only use pure-rand's implementation as a basis. |
This PR replaces our
MersenneTwister
implementation with one modified from https://github.com/dubzzz/pure-rand.This PR does not change the underlying algorithm/results per seed.
Reasons for the change
Improved Performance
(higher is better)
Fastest is
new (32bit)
with 4.25 times the speed of faker's old implementation.This especially improves the performance for our - current default - 53 bit variant with a speed up by 5.65 times.
One of the main reasons for the speed up is that
pure-rand
's implementation uses existingMath
functions intended to work withint32
instead of manually clamping the values to the 32bit bounds after each operation. Since the new implementation is based on that, we also get these improvements.Why not use
pure-rand
as a dependencyinitByArray
method and I was unable to find that in the official spec to verify that it is compliant. @dubzzz feel free to use our new implementation of that method for your library.initByArray
method and maybe laterinitByString
/initByAny
that depend on itprivate static member functions
are up to 75% slower when compiled with target ESNext in comparison tostandalone functions
for some reason. It is the other way round when compiled with ES3/5 (used by pure-rand).@dubzzz We already tried to contact you via e-mail to check whether you are fine with our plans to use your implementation as a base for ours. Please let us know what you think.
The code remains MIT licensed, we just wanted to make sure you aren't blindsided by this change.