-
-
Notifications
You must be signed in to change notification settings - Fork 910
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
perf(nodejs): introduce pool into default rng #513
perf(nodejs): introduce pool into default rng #513
Conversation
FWIW, results on my laptop... Before:
After:
|
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.
Nice improvement. It looks like we can further significantly improve perf with the attached suggestions. (Mostly a result of increasing pool size from 64 -> 256. Other tweaks to slightly reduce code size / complexity.)
With the attached, I get the following benchmark results:
uuidv1() x 1,604,852 ops/sec ±0.55% (95 runs sampled)
uuidv1() fill existing array x 8,987,992 ops/sec ±0.22% (97 runs sampled)
uuidv4() x 1,126,995 ops/sec ±0.33% (93 runs sampled)
uuidv4() fill existing array x 4,448,781 ops/sec ±1.50% (93 runs sampled)
@broofa I've addressed your comments and updated benchmark results in the PR description |
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.
LGTM, thank you @puzpuzpuz 🙏 !
@ctavan 'Not sure why bundlewatch test is hanging here. I was able to get it to pass on my laptop so I'm merging, but I ran into some sort of "call stack exceeded" error initially when doing |
@puzpuzpuz Thanks for the help! |
@broofa bundlewatch needs a token which is not available in CI runs from forks, only from branches within this repo... so PRs from forks will never report bundlewatch results. I haven’t found a workaround so far. |
Utilize a pool for rng. Improves throughput approx 6 times. This fix is brought by upstream uuidjs/uuid#513
Utilize a pool for rng. Improves throughput approx 6 times. This fix is brought by upstream uuidjs/uuid#513
Please consider the regression introduced with this change. Looking forward to having a good discussion |
Benchmark
master
(bc9d4ed):Benchmark this branch (pool size 64):
Benchmark this branch (pool size 256) - current version: