Skip to content

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

Merged
broofa merged 3 commits intouuidjs:masterfrom
puzpuzpuz:experiment/batch-rng-for-nodejs
Aug 29, 2020
Merged

perf(nodejs): introduce pool into default rng#513
broofa merged 3 commits intouuidjs:masterfrom
puzpuzpuz:experiment/batch-rng-for-nodejs

Conversation

@puzpuzpuz
Copy link
Copy Markdown
Contributor

@puzpuzpuz puzpuzpuz commented Aug 27, 2020 •
edited
Loading

  • 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
Copy Markdown
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)

broofa
broofa requested changes Aug 27, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@broofa broofa left a comment •
edited
Loading

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)

@puzpuzpuz
Copy link
Copy Markdown
Contributor Author

puzpuzpuz commented Aug 28, 2020

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

ctavan
ctavan approved these changes Aug 28, 2020
View reviewed changes
Copy link
Copy Markdown
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 🙏 !

broofa
broofa requested changes Aug 29, 2020
View reviewed changes
@puzpuzpuz puzpuzpuz requested a review from broofa August 29, 2020 15:09
@broofa
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

puzpuzpuz commented Aug 29, 2020

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

@ctavan
Copy link
Copy Markdown
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
@dirkdev98
stdlib: improve perf of uuid
f219767 
Utilize a pool for rng. Improves throughput approx 6 times.
This fix is brought by upstream uuidjs/uuid#513
@dirkdev98 dirkdev98 mentioned this pull request Sep 5, 2020
Merged
dirkdev98 added a commit to compasjs/compas that referenced this pull request Sep 5, 2020
@dirkdev98
stdlib: improve perf of uuid (#242)
ef62575 
Utilize a pool for rng. Improves throughput approx 6 times.
This fix is brought by upstream uuidjs/uuid#513
@simlu simlu mentioned this pull request Oct 6, 2020
Closed
@simlu
Copy link
Copy Markdown

simlu commented Oct 6, 2020

Please consider the regression introduced with this change.

#525

Looking forward to having a good discussion

@puzpuzpuz puzpuzpuz mentioned this pull request May 14, 2021
Merged
@broofa broofa mentioned this pull request Jan 9, 2023
Closed
@dhensby dhensby mentioned this pull request Jul 25, 2024
Merged
This was referenced Oct 27, 2024
Merged
Merged
Closed
Merged
@realityking realityking mentioned this pull request Aug 3, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@broofa broofa broofa approved these changes

@ctavan ctavan ctavan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@puzpuzpuz @broofa @ctavan @simlu