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

perf(nodejs): introduce pool into default rng #513

Merged
merged 3 commits into from Aug 29, 2020

Conversation

puzpuzpuz
Copy link
Contributor

@puzpuzpuz puzpuzpuz commented Aug 27, 2020

  • Significantly improves v4 performance for the default RNG by introducing batch generation

Benchmark master (bc9d4ed):

uuidv1() x 1,868,914 ops/sec ±0.26% (92 runs sampled)
uuidv1() fill existing array x 9,711,758 ops/sec ±0.16% (94 runs sampled)
uuidv4() x 412,330 ops/sec ±0.13% (98 runs sampled)
uuidv4() fill existing array x 513,285 ops/sec ±0.16% (93 runs sampled)
uuidv3() x 321,397 ops/sec ±0.57% (91 runs sampled)
uuidv5() x 301,932 ops/sec ±1.08% (90 runs sampled)
Fastest is uuidv1() fill existing array

Benchmark this branch (pool size 64):

uuidv1() x 1,800,216 ops/sec ±0.80% (96 runs sampled)
uuidv1() fill existing array x 9,679,788 ops/sec ±0.27% (95 runs sampled)
uuidv4() x 819,473 ops/sec ±0.37% (96 runs sampled)
uuidv4() fill existing array x 1,630,070 ops/sec ±0.66% (92 runs sampled)
uuidv3() x 305,889 ops/sec ±0.99% (89 runs sampled)
uuidv5() x 299,425 ops/sec ±0.82% (89 runs sampled)
Fastest is uuidv1() fill existing array

Benchmark this branch (pool size 256) - current version:

uuidv1() x 1,874,252 ops/sec ±0.29% (97 runs sampled)
uuidv1() fill existing array x 9,641,771 ops/sec ±0.48% (94 runs sampled)
uuidv4() x 1,230,123 ops/sec ±0.63% (93 runs sampled)
uuidv4() fill existing array x 4,377,290 ops/sec ±0.71% (98 runs sampled)
uuidv3() x 317,152 ops/sec ±0.76% (93 runs sampled)
uuidv5() x 308,891 ops/sec ±0.85% (93 runs sampled)
Fastest is uuidv1() fill existing array

@broofa
Copy link
Member

broofa commented Aug 27, 2020

FWIW, results on my laptop...

Before:

uuidv1() x 1,586,023 ops/sec ±0.73% (91 runs sampled)
uuidv1() fill existing array x 8,903,577 ops/sec ±1.07% (89 runs sampled)
uuidv4() x 387,462 ops/sec ±3.11% (88 runs sampled)
uuidv4() fill existing array x 534,144 ops/sec ±2.71% (94 runs sampled)

After:

uuidv1() x 1,590,931 ops/sec ±0.55% (92 runs sampled)
uuidv1() fill existing array x 8,926,106 ops/sec ±0.56% (91 runs sampled)
uuidv4() x 745,416 ops/sec ±1.83% (82 runs sampled)
uuidv4() fill existing array x 1,685,734 ops/sec ±0.44% (96 runs sampled)

Copy link
Member

@broofa broofa left a 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)

src/rng.js Outdated Show resolved Hide resolved
src/rng.js Outdated Show resolved Hide resolved
@puzpuzpuz
Copy link
Contributor Author

@broofa I've addressed your comments and updated benchmark results in the PR description

Copy link
Member

@ctavan ctavan left a 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 🙏 !

src/rng.js Outdated Show resolved Hide resolved
@puzpuzpuz puzpuzpuz requested a review from broofa August 29, 2020 15:09
@broofa
Copy link
Member

broofa commented Aug 29, 2020

@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 npm run bundlewatch. Ended up rm -r node_modules'ing and reinstalling to get it to work.

@broofa broofa merged commit 7f1af04 into uuidjs:master Aug 29, 2020
@broofa
Copy link
Member

broofa commented Aug 29, 2020

@puzpuzpuz Thanks for the help!

@puzpuzpuz puzpuzpuz deleted the experiment/batch-rng-for-nodejs branch August 29, 2020 16:06
@puzpuzpuz
Copy link
Contributor Author

@broofa @ctavan thanks for the quick and valuable reviews!

@ctavan
Copy link
Member

ctavan commented Aug 29, 2020

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

dirkdev98 added a commit to compasjs/compas that referenced this pull request Sep 5, 2020
Utilize a pool for rng. Improves throughput approx 6 times.
This fix is brought by upstream uuidjs/uuid#513
dirkdev98 added a commit to compasjs/compas that referenced this pull request Sep 5, 2020
Utilize a pool for rng. Improves throughput approx 6 times.
This fix is brought by upstream uuidjs/uuid#513
@simlu
Copy link

simlu commented Oct 6, 2020

Please consider the regression introduced with this change.

#525

Looking forward to having a good discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants