Skip to content
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 one #3288

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Nov 28, 2024

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
  • Avoid long license preamble
  • (Slightly improved readability)

Improved Performance

MersenneTwister Performance Compared to old (32bit)
pure-rand (32bit) 61,218,398 ops/sec ±2.47% 3.85
old (32bit) 15,896,596 ops/sec ±0.85% 1.00
old (53bit) 8,582,587 ops/sec ±1.07% 0.53
new (32bit) 67,500,658 ops/sec ±2.67% 4.25
new (53bit) 48,467,687 ops/sec ±1.59% 3.05

(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 existing Math functions intended to work with int32 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 dependency

  1. pure-rand's implementation is based on the official spec, it does not have our initByArray 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.
  2. Adding the code to our source allows us to apply some optimizations to it, that aren't back-portable to pure-rand
    • Avoid array copy during use instead of being immutable
    • Add the methods to generate float32/float53 instead of just int32
    • Add initByArray method and maybe later initByString/initByAny that depend on it
  3. I also tried vendoring the file, but TS's private static member functions are up to 75% slower when compiled with target ESNext in comparison to standalone functions for some reason. It is the other way round when compiled with ES3/5 (used by pure-rand).
  4. Lastly, we don't have any dependencies and thus it would be our first

@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.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs labels Nov 28, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Nov 28, 2024
@ST-DDT ST-DDT requested review from a team November 28, 2024 22:55
@ST-DDT ST-DDT self-assigned this Nov 28, 2024
Copy link

netlify bot commented Nov 28, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit b3a2999
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/6748f4c893daf50008b37f9b
😎 Deploy Preview https://deploy-preview-3288.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Nov 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (5d5fe30) to head (b3a2999).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3288      +/-   ##
==========================================
- Coverage   99.97%   99.96%   -0.01%     
==========================================
  Files        2806     2806              
  Lines      217140   217089      -51     
  Branches      982      967      -15     
==========================================
- Hits       217075   217005      -70     
- Misses         65       84      +19     
Files with missing lines Coverage Δ
src/internal/mersenne.ts 100.00% <100.00%> (+5.96%) ⬆️
src/utils/mersenne.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@dubzzz
Copy link

dubzzz commented Nov 28, 2024

Clearly ok for me 👍
If I can help on providing missing helpers directly from pure-rand, let me know. Not for this release but maybe later in the future

Copy link
Member

@Shinigami92 Shinigami92 left a 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"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants