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

crypto: add buffering to randomInt #35110

Closed

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Sep 8, 2020

This adds a buffer to randomInt to prevent calling randomBytes on each invocation. It also simplifies the actual calculation to always be synchronous, and doesn't duplicate the logic for asynchronous behavior. However, this change does include additional code to manage the cache.

Performance is improved significantly, by large factors, for synchronous calls and sequential asynchronous calls. For parallel calls, performance is about the same when requesting a few hundred random integers at a time.

Performance is significantly worse when requesting a large number of random integers asynchronously and in parallel. The reason is that this implementation delays all requests until the buffer has been refilled with random data, then serves as many requests as it can, then delays the remaining requests until the buffer has been refilled again, and so on. With a buffer size of 3KiB, 500 random integers can be provided without having to call randomBytes. When the benchmark requests 100,000 random integers asynchronously and in parallel, instead of processing 100,000 calls to randomBytes in parallel, the buffered implementation provides 200 batches of 500 numbers each sequentially.

  • We could add additional code to handle such cases and avoid this performance penalty, but I decided to leave the additional complexity out for now. I assume that most use cases will require a sequence of random numbers, and not necessarily a large amount of random numbers at once.
  • We could also change the buffer size, but that would require more memory, and could lead to slightly longer delays when refilling the buffer.
  • We could also disable buffering for async in general, and only use it for sync calls. That would make any single call to randomInt with a callback slow, but would make generating huge amounts of random integers at the same time faster.

Benchmark results based on 30 runs (3988798 compared to the master branch at b5a47ca):

                                                                                               confidence improvement accuracy (*)     (**)    (***)
 crypto/randomInt.js n=1000 max=100 min=-100 mode='async-parallel'                                             7.61 %       ±8.19%  ±10.90%  ±14.19%
 crypto/randomInt.js n=1000 max=100 min=-100 mode='async-sequential'                                  ***    113.40 %       ±9.73%  ±12.98%  ±16.98%
 crypto/randomInt.js n=1000 max=100 min=-100 mode='sync'                                              ***    151.27 %       ±4.81%   ±6.41%   ±8.36%
 crypto/randomInt.js n=1000 max=100 min=-10000 mode='async-parallel'                                           1.72 %       ±8.22%  ±10.97%  ±14.35%
 crypto/randomInt.js n=1000 max=100 min=-10000 mode='async-sequential'                                ***    110.33 %       ±9.82%  ±13.12%  ±17.17%
 crypto/randomInt.js n=1000 max=100 min=-10000 mode='sync'                                            ***    138.80 %       ±7.85%  ±10.54%  ±13.92%
 crypto/randomInt.js n=1000 max=100 min=-140737488355327 mode='async-parallel'                        ***     39.70 %       ±9.72%  ±12.99%  ±17.03%
 crypto/randomInt.js n=1000 max=100 min=-140737488355327 mode='async-sequential'                      ***    277.28 %      ±12.17%  ±16.35%  ±21.61%
 crypto/randomInt.js n=1000 max=100 min=-140737488355327 mode='sync'                                  ***    264.60 %       ±9.85%  ±13.13%  ±17.16%
 crypto/randomInt.js n=1000 max=10000 min=-100 mode='async-parallel'                                    *      6.06 %       ±5.92%   ±7.88%  ±10.26%
 crypto/randomInt.js n=1000 max=10000 min=-100 mode='async-sequential'                                ***     97.52 %      ±10.61%  ±14.20%  ±18.65%
 crypto/randomInt.js n=1000 max=10000 min=-100 mode='sync'                                            ***    138.26 %       ±8.36%  ±11.26%  ±14.94%
 crypto/randomInt.js n=1000 max=10000 min=-10000 mode='async-parallel'                                        -4.46 %       ±8.33%  ±11.16%  ±14.69%
 crypto/randomInt.js n=1000 max=10000 min=-10000 mode='async-sequential'                              ***    103.98 %      ±10.82%  ±14.46%  ±18.96%
 crypto/randomInt.js n=1000 max=10000 min=-10000 mode='sync'                                          ***    141.13 %       ±3.42%   ±4.56%   ±5.95%
 crypto/randomInt.js n=1000 max=10000 min=-140737488355327 mode='async-parallel'                      ***     40.64 %       ±4.59%   ±6.11%   ±7.95%
 crypto/randomInt.js n=1000 max=10000 min=-140737488355327 mode='async-sequential'                    ***    263.09 %      ±23.93%  ±32.22%  ±42.72%
 crypto/randomInt.js n=1000 max=10000 min=-140737488355327 mode='sync'                                ***    246.81 %       ±4.33%   ±5.83%   ±7.72%
 crypto/randomInt.js n=1000 max=140737488355328 min=-100 mode='async-parallel'                        ***     37.40 %       ±7.18%   ±9.59%  ±12.55%
 crypto/randomInt.js n=1000 max=140737488355328 min=-100 mode='async-sequential'                      ***    275.54 %      ±11.41%  ±15.31%  ±20.20%
 crypto/randomInt.js n=1000 max=140737488355328 min=-100 mode='sync'                                  ***    247.66 %       ±3.52%   ±4.71%   ±6.17%
 crypto/randomInt.js n=1000 max=140737488355328 min=-10000 mode='async-parallel'                      ***     36.03 %       ±7.56%  ±10.11%  ±13.27%
 crypto/randomInt.js n=1000 max=140737488355328 min=-10000 mode='async-sequential'                    ***    254.88 %      ±18.48%  ±24.85%  ±32.86%
 crypto/randomInt.js n=1000 max=140737488355328 min=-10000 mode='sync'                                ***    251.64 %       ±3.86%   ±5.16%   ±6.76%
 crypto/randomInt.js n=1000 max=140737488355328 min=-140737488355327 mode='async-parallel'                    -0.22 %       ±7.35%   ±9.80%  ±12.78%
 crypto/randomInt.js n=1000 max=140737488355328 min=-140737488355327 mode='async-sequential'          ***    104.97 %      ±12.95%  ±17.36%  ±22.87%
 crypto/randomInt.js n=1000 max=140737488355328 min=-140737488355327 mode='sync'                      ***    138.63 %       ±4.24%   ±5.71%   ±7.56%
 crypto/randomInt.js n=100000 max=100 min=-100 mode='async-parallel'                                  ***    -41.12 %       ±1.21%   ±1.62%   ±2.13%
 crypto/randomInt.js n=100000 max=100 min=-100 mode='async-sequential'                                ***    691.82 %       ±7.30%   ±9.81%  ±12.97%
 crypto/randomInt.js n=100000 max=100 min=-100 mode='sync'                                            ***   1310.10 %      ±36.17%  ±48.74%  ±64.70%
 crypto/randomInt.js n=100000 max=100 min=-10000 mode='async-parallel'                                ***    -40.20 %       ±1.28%   ±1.72%   ±2.26%
 crypto/randomInt.js n=100000 max=100 min=-10000 mode='async-sequential'                              ***    691.30 %       ±8.86%  ±11.91%  ±15.77%
 crypto/randomInt.js n=100000 max=100 min=-10000 mode='sync'                                          ***   1290.11 %      ±55.81%  ±75.21%  ±99.85%
 crypto/randomInt.js n=100000 max=100 min=-140737488355327 mode='async-parallel'                      ***    -38.02 %       ±0.83%   ±1.11%   ±1.47%
 crypto/randomInt.js n=100000 max=100 min=-140737488355327 mode='async-sequential'                    ***   1434.15 %      ±22.58%  ±30.43%  ±40.39%
 crypto/randomInt.js n=100000 max=100 min=-140737488355327 mode='sync'                                ***   2233.63 %      ±61.72%  ±83.18% ±110.43%
 crypto/randomInt.js n=100000 max=10000 min=-100 mode='async-parallel'                                ***    -40.04 %       ±1.48%   ±1.99%   ±2.64%
 crypto/randomInt.js n=100000 max=10000 min=-100 mode='async-sequential'                              ***    694.65 %       ±7.53%  ±10.12%  ±13.38%
 crypto/randomInt.js n=100000 max=10000 min=-100 mode='sync'                                          ***   1338.29 %      ±31.28%  ±42.15%  ±55.95%
 crypto/randomInt.js n=100000 max=10000 min=-10000 mode='async-parallel'                              ***    -39.62 %       ±1.15%   ±1.54%   ±2.04%
 crypto/randomInt.js n=100000 max=10000 min=-10000 mode='async-sequential'                            ***    685.36 %       ±8.82%  ±11.87%  ±15.71%
 crypto/randomInt.js n=100000 max=10000 min=-10000 mode='sync'                                        ***   1286.39 %      ±43.29%  ±58.33%  ±77.43%
 crypto/randomInt.js n=100000 max=10000 min=-140737488355327 mode='async-parallel'                    ***    -38.56 %       ±1.02%   ±1.37%   ±1.80%
 crypto/randomInt.js n=100000 max=10000 min=-140737488355327 mode='async-sequential'                  ***   1450.61 %      ±18.13%  ±24.43%  ±32.43%
 crypto/randomInt.js n=100000 max=10000 min=-140737488355327 mode='sync'                              ***   2137.98 %      ±87.00% ±117.25% ±155.67%
 crypto/randomInt.js n=100000 max=140737488355328 min=-100 mode='async-parallel'                      ***    -39.14 %       ±0.82%   ±1.09%   ±1.43%
 crypto/randomInt.js n=100000 max=140737488355328 min=-100 mode='async-sequential'                    ***   1453.44 %      ±16.70%  ±22.50%  ±29.87%
 crypto/randomInt.js n=100000 max=140737488355328 min=-100 mode='sync'                                ***   2161.60 %      ±72.54%  ±97.76% ±129.79%
 crypto/randomInt.js n=100000 max=140737488355328 min=-10000 mode='async-parallel'                    ***    -38.39 %       ±1.10%   ±1.47%   ±1.93%
 crypto/randomInt.js n=100000 max=140737488355328 min=-10000 mode='async-sequential'                  ***   1449.92 %      ±13.92%  ±18.74%  ±24.86%
 crypto/randomInt.js n=100000 max=140737488355328 min=-10000 mode='sync'                              ***   2169.74 %      ±67.67%  ±91.20% ±121.08%
 crypto/randomInt.js n=100000 max=140737488355328 min=-140737488355327 mode='async-parallel'          ***    -51.28 %       ±1.46%   ±1.96%   ±2.57%
 crypto/randomInt.js n=100000 max=140737488355328 min=-140737488355327 mode='async-sequential'        ***    695.29 %       ±8.29%  ±11.15%  ±14.75%
 crypto/randomInt.js n=100000 max=140737488355328 min=-140737488355327 mode='sync'                    ***   1271.56 %      ±36.90%  ±49.73%  ±66.01%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 54 comparisons, you can thus
expect the following amount of false-positive results:
  2.70 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.54 false positives, when considering a   1% risk acceptance (**, ***),
  0.05 false positives, when considering a 0.1% risk acceptance (***)
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Sep 8, 2020
@tniessen tniessen added benchmark Issues and PRs related to the benchmark subsystem. performance Issues and PRs related to the performance of Node.js. labels Sep 8, 2020
@bnoordhuis
Copy link
Member

I'm decidedly 'meh' on this. No one's been complaining about its speed so far.

@tniessen tniessen added the review wanted PRs that need reviews. label Sep 14, 2020
@tniessen
Copy link
Member Author

@bnoordhuis How about adding this for the sync case only? That's a relatively small change with a big payoff, and I think the performance of the sync case is more critical anyway. The downside is that this approach results in the existing code duplication between sync and async behavior, which essentially implements the entire thing twice.

@aduh95
Copy link
Contributor

aduh95 commented Nov 8, 2020

@nodejs/crypto can you review this please? Those benchmark results look very good!

@tniessen tniessen mentioned this pull request Jan 3, 2021
4 tasks
@ChALkeR ChALkeR self-requested a review January 3, 2021 14:35
lib/internal/crypto/random.js Outdated Show resolved Hide resolved
lib/internal/crypto/random.js Outdated Show resolved Hide resolved
const randomCache = new FastBuffer(3072);
let randomCacheOffset = randomCache.length;
let asyncCacheFillInProgress = false;
const asyncCachePendingTasks = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using an array for this is usually not performing as expected for high numbers. Using a linked list solves this problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have fixed-queue which also might be appropriate?

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to introduce a random buffer internal utility that can be used in multiple places (here, the randomuuid fn, etc). That can be done later tho

@tniessen
Copy link
Member Author

@mcollina I doubled the cache size, but a more efficient way to improve performance might be to reduce the number of bytes that is used for each random integer. In most cases, we don't actually need 6 bytes, probably ony 2 to 4 bytes. (It depends on the use case, of course.)

@puzpuzpuz
Copy link
Member

@tniessen could you resolve conflicts on this one?

@tniessen tniessen force-pushed the crypto-add-randomint-buffering branch from efd2b91 to 499bed1 Compare January 18, 2021 13:17
@tniessen
Copy link
Member Author

tniessen commented Jan 18, 2021

Benchmark results still look good:

Node js Core Benchmark Visualization

Table
n max min mode confidence improvement accuracy (*) accuracy (**) accuracy (***)
100000 10000 -10000 'async-parallel' *** 41.05 % ±4.60% ±6.17% ±8.12%
100000 10000 -10000 'async-sequential' *** 1931.62 % ±106.63% ±143.69% ±190.75%
100000 10000 -10000 'sync' *** 1976.14 % ±114.57% ±154.41% ±204.98%
100000 10000 -100 'async-parallel' *** 43.43 % ±5.20% ±6.95% ±9.10%
100000 10000 -100 'async-sequential' *** 1888.73 % ±99.87% ±134.60% ±178.69%
100000 10000 -100 'sync' *** 1895.70 % ±99.80% ±134.49% ±178.54%
100000 10000 -140737488355327 'async-parallel' *** 44.39 % ±4.65% ±6.23% ±8.22%
100000 10000 -140737488355327 'async-sequential' *** 3455.63 % ±230.82% ±311.08% ±412.99%
100000 10000 -140737488355327 'sync' *** 3056.68 % ±158.66% ±213.82% ±283.87%
100000 100 -10000 'async-parallel' *** 39.66 % ±5.26% ±7.05% ±9.27%
100000 100 -10000 'async-sequential' *** 1936.95 % ±97.77% ±131.76% ±174.89%
100000 100 -10000 'sync' *** 1885.94 % ±75.29% ±101.47% ±134.70%
100000 100 -100 'async-parallel' *** 39.74 % ±5.70% ±7.64% ±10.06%
100000 100 -100 'async-sequential' *** 1878.43 % ±88.73% ±119.57% ±158.72%
100000 100 -100 'sync' *** 1892.60 % ±96.45% ±129.98% ±172.55%
100000 100 -140737488355327 'async-parallel' *** 40.54 % ±4.49% ±6.03% ±7.96%
100000 100 -140737488355327 'async-sequential' *** 3335.10 % ±234.14% ±315.55% ±418.91%
100000 100 -140737488355327 'sync' *** 3140.61 % ±170.90% ±230.32% ±305.77%
100000 140737488355328 -10000 'async-parallel' *** 47.40 % ±4.92% ±6.60% ±8.71%
100000 140737488355328 -10000 'async-sequential' *** 3300.80 % ±182.10% ±245.41% ±325.80%
100000 140737488355328 -10000 'sync' *** 2980.74 % ±144.38% ±194.58% ±258.31%
100000 140737488355328 -100 'async-parallel' *** 48.80 % ±5.10% ±6.86% ±9.06%
100000 140737488355328 -100 'async-sequential' *** 3385.05 % ±175.80% ±236.92% ±314.53%
100000 140737488355328 -100 'sync' *** 3041.96 % ±176.29% ±237.58% ±315.41%
100000 140737488355328 -140737488355327 'async-parallel' *** 21.46 % ±4.38% ±5.86% ±7.71%
100000 140737488355328 -140737488355327 'async-sequential' *** 2011.00 % ±146.63% ±197.62% ±262.35%
100000 140737488355328 -140737488355327 'sync' *** 1863.61 % ±92.30% ±124.39% ±165.14%
1000 10000 -10000 'async-parallel' *** 52.00 % ±11.51% ±15.44% ±20.35%
1000 10000 -10000 'async-sequential' *** 159.33 % ±10.13% ±13.58% ±17.88%
1000 10000 -10000 'sync' *** 545.42 % ±38.66% ±52.05% ±69.01%
1000 10000 -100 'async-parallel' *** 38.86 % ±6.89% ±9.18% ±11.99%
1000 10000 -100 'async-sequential' *** 156.91 % ±13.86% ±18.61% ±24.56%
1000 10000 -100 'sync' *** 605.82 % ±54.76% ±73.78% ±97.89%
1000 10000 -140737488355327 'async-parallel' *** 78.15 % ±5.72% ±7.61% ±9.90%
1000 10000 -140737488355327 'async-sequential' *** 331.03 % ±20.68% ±27.83% ±36.86%
1000 10000 -140737488355327 'sync' *** 516.57 % ±42.54% ±57.30% ±76.01%
1000 100 -10000 'async-parallel' *** 36.48 % ±8.30% ±11.06% ±14.44%
1000 100 -10000 'async-sequential' *** 152.74 % ±10.06% ±13.46% ±17.65%
1000 100 -10000 'sync' *** 566.73 % ±35.87% ±48.30% ±64.04%
1000 100 -100 'async-parallel' *** 31.18 % ±6.98% ±9.30% ±12.11%
1000 100 -100 'async-sequential' *** 161.54 % ±11.41% ±15.28% ±20.10%
1000 100 -100 'sync' *** 543.65 % ±36.10% ±48.61% ±64.46%
1000 100 -140737488355327 'async-parallel' *** 97.30 % ±10.66% ±14.31% ±18.89%
1000 100 -140737488355327 'async-sequential' *** 353.64 % ±29.03% ±39.09% ±51.84%
1000 100 -140737488355327 'sync' *** 494.78 % ±20.94% ±28.15% ±37.24%
1000 140737488355328 -10000 'async-parallel' *** 82.55 % ±10.74% ±14.38% ±18.90%
1000 140737488355328 -10000 'async-sequential' *** 325.50 % ±17.69% ±23.80% ±31.51%
1000 140737488355328 -10000 'sync' *** 525.51 % ±45.32% ±61.05% ±80.97%
1000 140737488355328 -100 'async-parallel' *** 80.98 % ±11.91% ±15.94% ±20.93%
1000 140737488355328 -100 'async-sequential' *** 325.43 % ±15.49% ±20.82% ±27.53%
1000 140737488355328 -100 'sync' *** 483.61 % ±36.11% ±48.61% ±64.44%
1000 140737488355328 -140737488355327 'async-parallel' *** 47.12 % ±7.40% ±9.88% ±12.92%
1000 140737488355328 -140737488355327 'async-sequential' *** 164.94 % ±12.99% ±17.42% ±22.93%
1000 140737488355328 -140737488355327 'sync' *** 601.63 % ±45.89% ±61.83% ±82.04%
Unformatted output
                                                                                               confidence improvement accuracy (*)     (**)    (***)
 crypto/randomInt.js n=100000 max=10000 min=-10000 mode='async-parallel'                              ***     41.05 %       ±4.60%   ±6.17%   ±8.12%
 crypto/randomInt.js n=100000 max=10000 min=-10000 mode='async-sequential'                            ***   1931.62 %     ±106.63% ±143.69% ±190.75%
 crypto/randomInt.js n=100000 max=10000 min=-10000 mode='sync'                                        ***   1976.14 %     ±114.57% ±154.41% ±204.98%
 crypto/randomInt.js n=100000 max=10000 min=-100 mode='async-parallel'                                ***     43.43 %       ±5.20%   ±6.95%   ±9.10%
 crypto/randomInt.js n=100000 max=10000 min=-100 mode='async-sequential'                              ***   1888.73 %      ±99.87% ±134.60% ±178.69%
 crypto/randomInt.js n=100000 max=10000 min=-100 mode='sync'                                          ***   1895.70 %      ±99.80% ±134.49% ±178.54%
 crypto/randomInt.js n=100000 max=10000 min=-140737488355327 mode='async-parallel'                    ***     44.39 %       ±4.65%   ±6.23%   ±8.22%
 crypto/randomInt.js n=100000 max=10000 min=-140737488355327 mode='async-sequential'                  ***   3455.63 %     ±230.82% ±311.08% ±412.99%
 crypto/randomInt.js n=100000 max=10000 min=-140737488355327 mode='sync'                              ***   3056.68 %     ±158.66% ±213.82% ±283.87%
 crypto/randomInt.js n=100000 max=100 min=-10000 mode='async-parallel'                                ***     39.66 %       ±5.26%   ±7.05%   ±9.27%
 crypto/randomInt.js n=100000 max=100 min=-10000 mode='async-sequential'                              ***   1936.95 %      ±97.77% ±131.76% ±174.89%
 crypto/randomInt.js n=100000 max=100 min=-10000 mode='sync'                                          ***   1885.94 %      ±75.29% ±101.47% ±134.70%
 crypto/randomInt.js n=100000 max=100 min=-100 mode='async-parallel'                                  ***     39.74 %       ±5.70%   ±7.64%  ±10.06%
 crypto/randomInt.js n=100000 max=100 min=-100 mode='async-sequential'                                ***   1878.43 %      ±88.73% ±119.57% ±158.72%
 crypto/randomInt.js n=100000 max=100 min=-100 mode='sync'                                            ***   1892.60 %      ±96.45% ±129.98% ±172.55%
 crypto/randomInt.js n=100000 max=100 min=-140737488355327 mode='async-parallel'                      ***     40.54 %       ±4.49%   ±6.03%   ±7.96%
 crypto/randomInt.js n=100000 max=100 min=-140737488355327 mode='async-sequential'                    ***   3335.10 %     ±234.14% ±315.55% ±418.91%
 crypto/randomInt.js n=100000 max=100 min=-140737488355327 mode='sync'                                ***   3140.61 %     ±170.90% ±230.32% ±305.77%
 crypto/randomInt.js n=100000 max=140737488355328 min=-10000 mode='async-parallel'                    ***     47.40 %       ±4.92%   ±6.60%   ±8.71%
 crypto/randomInt.js n=100000 max=140737488355328 min=-10000 mode='async-sequential'                  ***   3300.80 %     ±182.10% ±245.41% ±325.80%
 crypto/randomInt.js n=100000 max=140737488355328 min=-10000 mode='sync'                              ***   2980.74 %     ±144.38% ±194.58% ±258.31%
 crypto/randomInt.js n=100000 max=140737488355328 min=-100 mode='async-parallel'                      ***     48.80 %       ±5.10%   ±6.86%   ±9.06%
 crypto/randomInt.js n=100000 max=140737488355328 min=-100 mode='async-sequential'                    ***   3385.05 %     ±175.80% ±236.92% ±314.53%
 crypto/randomInt.js n=100000 max=140737488355328 min=-100 mode='sync'                                ***   3041.96 %     ±176.29% ±237.58% ±315.41%
 crypto/randomInt.js n=100000 max=140737488355328 min=-140737488355327 mode='async-parallel'          ***     21.46 %       ±4.38%   ±5.86%   ±7.71%
 crypto/randomInt.js n=100000 max=140737488355328 min=-140737488355327 mode='async-sequential'        ***   2011.00 %     ±146.63% ±197.62% ±262.35%
 crypto/randomInt.js n=100000 max=140737488355328 min=-140737488355327 mode='sync'                    ***   1863.61 %      ±92.30% ±124.39% ±165.14%
 crypto/randomInt.js n=1000 max=10000 min=-10000 mode='async-parallel'                                ***     52.00 %      ±11.51%  ±15.44%  ±20.35%
 crypto/randomInt.js n=1000 max=10000 min=-10000 mode='async-sequential'                              ***    159.33 %      ±10.13%  ±13.58%  ±17.88%
 crypto/randomInt.js n=1000 max=10000 min=-10000 mode='sync'                                          ***    545.42 %      ±38.66%  ±52.05%  ±69.01%
 crypto/randomInt.js n=1000 max=10000 min=-100 mode='async-parallel'                                  ***     38.86 %       ±6.89%   ±9.18%  ±11.99%
 crypto/randomInt.js n=1000 max=10000 min=-100 mode='async-sequential'                                ***    156.91 %      ±13.86%  ±18.61%  ±24.56%
 crypto/randomInt.js n=1000 max=10000 min=-100 mode='sync'                                            ***    605.82 %      ±54.76%  ±73.78%  ±97.89%
 crypto/randomInt.js n=1000 max=10000 min=-140737488355327 mode='async-parallel'                      ***     78.15 %       ±5.72%   ±7.61%   ±9.90%
 crypto/randomInt.js n=1000 max=10000 min=-140737488355327 mode='async-sequential'                    ***    331.03 %      ±20.68%  ±27.83%  ±36.86%
 crypto/randomInt.js n=1000 max=10000 min=-140737488355327 mode='sync'                                ***    516.57 %      ±42.54%  ±57.30%  ±76.01%
 crypto/randomInt.js n=1000 max=100 min=-10000 mode='async-parallel'                                  ***     36.48 %       ±8.30%  ±11.06%  ±14.44%
 crypto/randomInt.js n=1000 max=100 min=-10000 mode='async-sequential'                                ***    152.74 %      ±10.06%  ±13.46%  ±17.65%
 crypto/randomInt.js n=1000 max=100 min=-10000 mode='sync'                                            ***    566.73 %      ±35.87%  ±48.30%  ±64.04%
 crypto/randomInt.js n=1000 max=100 min=-100 mode='async-parallel'                                    ***     31.18 %       ±6.98%   ±9.30%  ±12.11%
 crypto/randomInt.js n=1000 max=100 min=-100 mode='async-sequential'                                  ***    161.54 %      ±11.41%  ±15.28%  ±20.10%
 crypto/randomInt.js n=1000 max=100 min=-100 mode='sync'                                              ***    543.65 %      ±36.10%  ±48.61%  ±64.46%
 crypto/randomInt.js n=1000 max=100 min=-140737488355327 mode='async-parallel'                        ***     97.30 %      ±10.66%  ±14.31%  ±18.89%
 crypto/randomInt.js n=1000 max=100 min=-140737488355327 mode='async-sequential'                      ***    353.64 %      ±29.03%  ±39.09%  ±51.84%
 crypto/randomInt.js n=1000 max=100 min=-140737488355327 mode='sync'                                  ***    494.78 %      ±20.94%  ±28.15%  ±37.24%
 crypto/randomInt.js n=1000 max=140737488355328 min=-10000 mode='async-parallel'                      ***     82.55 %      ±10.74%  ±14.38%  ±18.90%
 crypto/randomInt.js n=1000 max=140737488355328 min=-10000 mode='async-sequential'                    ***    325.50 %      ±17.69%  ±23.80%  ±31.51%
 crypto/randomInt.js n=1000 max=140737488355328 min=-10000 mode='sync'                                ***    525.51 %      ±45.32%  ±61.05%  ±80.97%
 crypto/randomInt.js n=1000 max=140737488355328 min=-100 mode='async-parallel'                        ***     80.98 %      ±11.91%  ±15.94%  ±20.93%
 crypto/randomInt.js n=1000 max=140737488355328 min=-100 mode='async-sequential'                      ***    325.43 %      ±15.49%  ±20.82%  ±27.53%
 crypto/randomInt.js n=1000 max=140737488355328 min=-100 mode='sync'                                  ***    483.61 %      ±36.11%  ±48.61%  ±64.44%
 crypto/randomInt.js n=1000 max=140737488355328 min=-140737488355327 mode='async-parallel'            ***     47.12 %       ±7.40%   ±9.88%  ±12.92%
 crypto/randomInt.js n=1000 max=140737488355328 min=-140737488355327 mode='async-sequential'          ***    164.94 %      ±12.99%  ±17.42%  ±22.93%
 crypto/randomInt.js n=1000 max=140737488355328 min=-140737488355327 mode='sync'                      ***    601.63 %      ±45.89%  ±61.83%  ±82.04%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 54 comparisons, you can thus
expect the following amount of false-positive results:
  2.70 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.54 false positives, when considering a   1% risk acceptance (**, ***),
  0.05 false positives, when considering a 0.1% risk acceptance (***)

let randomCacheOffset = randomCache.length;
let asyncCacheFillInProgress = false;
const asyncCachePendingTasks = [];

// Generates an integer in [min, max) range where min is inclusive and max is
// exclusive.
function randomInt(min, max, callback) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An idea for potential enhancement: consider adding options arg with disableEntropyCache setting to allow disabling the random cache. This way will make the API consistent with randomUUID (see #36729).

@@ -179,6 +179,13 @@ function randomFill(buf, offset, size, callback) {
// e.g.: Buffer.from("ff".repeat(6), "hex").readUIntBE(0, 6);
const RAND_MAX = 0xFFFF_FFFF_FFFF;

// Cache random data to use in randomInt. The cache size must be evenly
// divisible by 6 because each attempt to obtain a random int uses 6 bytes.
const randomCache = new FastBuffer(6 * 1024);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using secureBuffer here to integrate with secure heap (see randomUUID's random cache and #36779).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. We could do that, but UUIDs and random ints always result in insecure memory allocations anyway that mostly destroy any security guarantees. JavaScript is terrible for secure memory management.

@jasnell
Copy link
Member

jasnell commented Feb 22, 2021

@mcollina ... please take another look

lib/internal/crypto/random.js Outdated Show resolved Hide resolved
lib/internal/crypto/random.js Outdated Show resolved Hide resolved
lib/internal/crypto/random.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@tniessen tniessen force-pushed the crypto-add-randomint-buffering branch from fe80c8e to 2cdb569 Compare April 2, 2021 13:25
@tniessen
Copy link
Member Author

tniessen commented Apr 2, 2021

I believe this was technically ready to land a while ago, but I just addressed @aduh95's comments regarding primordials.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member Author

tniessen commented Apr 3, 2021

Benchmark results still look good:

Screenshot_2021-04-03 Node js Core Benchmark Visualization

Unformatted output
15:15:01                                                                                               confidence improvement accuracy (*)     (**)    (***)
15:15:01  crypto/randomInt.jsn=100000 max=10000 min=-10000 mode='async-parallel'                              ***     24.11 %       ±3.79%   ±5.04%   ±6.57%
15:15:01  crypto/randomInt.jsn=100000 max=10000 min=-10000 mode='async-sequential'                            ***   1854.25 %     ±100.81% ±135.85% ±180.34%
15:15:01  crypto/randomInt.jsn=100000 max=10000 min=-10000 mode='sync'                                        ***   1692.53 %      ±86.05% ±115.97% ±153.96%
15:15:01  crypto/randomInt.jsn=100000 max=10000 min=-100 mode='async-parallel'                                ***     26.16 %       ±4.40%   ±5.86%   ±7.66%
15:15:01  crypto/randomInt.jsn=100000 max=10000 min=-100 mode='async-sequential'                              ***   1908.77 %     ±126.68% ±170.72% ±226.62%
15:15:01  crypto/randomInt.jsn=100000 max=10000 min=-100 mode='sync'                                          ***   1681.47 %      ±83.93% ±113.11% ±150.16%
15:15:01  crypto/randomInt.jsn=100000 max=10000 min=-140737488355327 mode='async-parallel'                    ***     20.07 %       ±5.60%   ±7.53%   ±9.95%
15:15:01  crypto/randomInt.jsn=100000 max=10000 min=-140737488355327 mode='async-sequential'                  ***   3343.76 %     ±182.99% ±246.62% ±327.41%
15:15:01  crypto/randomInt.jsn=100000 max=10000 min=-140737488355327 mode='sync'                              ***   2995.21 %     ±184.03% ±248.02% ±329.26%
15:15:01  crypto/randomInt.jsn=100000 max=100 min=-10000 mode='async-parallel'                                ***     28.82 %       ±4.26%   ±5.68%   ±7.42%
15:15:01  crypto/randomInt.jsn=100000 max=100 min=-10000 mode='async-sequential'                              ***   1854.69 %      ±91.38% ±123.14% ±163.47%
15:15:01  crypto/randomInt.jsn=100000 max=100 min=-10000 mode='sync'                                          ***   1725.99 %      ±72.64%  ±97.89% ±129.94%
15:15:01  crypto/randomInt.jsn=100000 max=100 min=-100 mode='async-parallel'                                  ***     20.69 %       ±3.74%   ±4.98%   ±6.50%
15:15:01  crypto/randomInt.jsn=100000 max=100 min=-100 mode='async-sequential'                                ***   1849.93 %      ±89.96% ±121.23% ±160.92%
15:15:01  crypto/randomInt.jsn=100000 max=100 min=-100 mode='sync'                                            ***   1624.21 %      ±42.71%  ±57.56%  ±76.40%
15:15:01  crypto/randomInt.jsn=100000 max=100 min=-140737488355327 mode='async-parallel'                      ***     21.91 %       ±5.37%   ±7.21%   ±9.54%
15:15:01  crypto/randomInt.jsn=100000 max=100 min=-140737488355327 mode='async-sequential'                    ***   3501.94 %     ±202.95% ±273.51% ±363.11%
15:15:01  crypto/randomInt.jsn=100000 max=100 min=-140737488355327 mode='sync'                                ***   3044.37 %     ±186.10% ±250.80% ±332.97%
15:15:01  crypto/randomInt.jsn=100000 max=140737488355328 min=-10000 mode='async-parallel'                    ***     22.47 %       ±6.02%   ±8.10%  ±10.71%
15:15:01  crypto/randomInt.jsn=100000 max=140737488355328 min=-10000 mode='async-sequential'                  ***   3494.36 %     ±215.78% ±290.80% ±386.06%
15:15:01  crypto/randomInt.jsn=100000 max=140737488355328 min=-10000 mode='sync'                              ***   3017.06 %     ±166.74% ±224.71% ±298.33%
15:15:01  crypto/randomInt.jsn=100000 max=140737488355328 min=-100 mode='async-parallel'                      ***     29.59 %       ±4.95%   ±6.63%   ±8.74%
15:15:01  crypto/randomInt.jsn=100000 max=140737488355328 min=-100 mode='async-sequential'                    ***   3281.68 %     ±179.42% ±241.80% ±320.99%
15:15:01  crypto/randomInt.jsn=100000 max=140737488355328 min=-100 mode='sync'                                ***   2985.85 %     ±242.80% ±327.22% ±434.41%
15:15:01  crypto/randomInt.jsn=100000 max=140737488355328 min=-140737488355327 mode='async-parallel'          ***     -6.84 %       ±3.26%   ±4.34%   ±5.65%
15:15:01  crypto/randomInt.jsn=100000 max=140737488355328 min=-140737488355327 mode='async-sequential'        ***   1861.71 %      ±83.21% ±112.12% ±148.83%
15:15:01  crypto/randomInt.jsn=100000 max=140737488355328 min=-140737488355327 mode='sync'                    ***   1652.79 %     ±101.12% ±136.28% ±180.92%
15:15:01  crypto/randomInt.jsn=1000 max=10000 min=-10000 mode='async-parallel'                                ***     55.56 %       ±6.94%   ±9.29%  ±12.21%
15:15:01  crypto/randomInt.jsn=1000 max=10000 min=-10000 mode='async-sequential'                              ***    184.54 %      ±14.83%  ±19.90%  ±26.24%
15:15:01  crypto/randomInt.jsn=1000 max=10000 min=-10000 mode='sync'                                          ***    491.12 %      ±24.91%  ±33.46%  ±44.18%
15:15:01  crypto/randomInt.jsn=1000 max=10000 min=-100 mode='async-parallel'                                  ***     39.02 %       ±6.75%   ±8.98%  ±11.70%
15:15:01  crypto/randomInt.jsn=1000 max=10000 min=-100 mode='async-sequential'                                ***    198.34 %      ±14.60%  ±19.62%  ±25.95%
15:15:01  crypto/randomInt.jsn=1000 max=10000 min=-100 mode='sync'                                            ***    500.96 %      ±26.60%  ±35.77%  ±47.35%
15:15:01  crypto/randomInt.jsn=1000 max=10000 min=-140737488355327 mode='async-parallel'                      ***     85.51 %      ±10.84%  ±14.54%  ±19.18%
15:15:01  crypto/randomInt.jsn=1000 max=10000 min=-140737488355327 mode='async-sequential'                    ***    363.90 %      ±16.79%  ±22.56%  ±29.80%
15:15:01  crypto/randomInt.jsn=1000 max=10000 min=-140737488355327 mode='sync'                                ***    704.50 %      ±57.53%  ±77.49% ±102.80%
15:15:01  crypto/randomInt.jsn=1000 max=100 min=-10000 mode='async-parallel'                                  ***     51.90 %      ±12.02%  ±16.13%  ±21.27%
15:15:01  crypto/randomInt.jsn=1000 max=100 min=-10000 mode='async-sequential'                                ***    200.13 %      ±17.37%  ±23.32%  ±30.80%
15:15:01  crypto/randomInt.jsn=1000 max=100 min=-10000 mode='sync'                                            ***    499.05 %      ±30.34%  ±40.84%  ±54.13%
15:15:01  crypto/randomInt.jsn=1000 max=100 min=-100 mode='async-parallel'                                    ***     36.25 %       ±9.41%  ±12.52%  ±16.30%
15:15:01  crypto/randomInt.jsn=1000 max=100 min=-100 mode='async-sequential'                                  ***    191.84 %      ±16.23%  ±21.81%  ±28.84%
15:15:01  crypto/randomInt.jsn=1000 max=100 min=-100 mode='sync'                                              ***    465.86 %      ±19.74%  ±26.53%  ±35.09%
15:15:01  crypto/randomInt.jsn=1000 max=100 min=-140737488355327 mode='async-parallel'                        ***     93.88 %      ±12.16%  ±16.32%  ±21.52%
15:15:01  crypto/randomInt.jsn=1000 max=100 min=-140737488355327 mode='async-sequential'                      ***    388.58 %      ±27.68%  ±37.28%  ±49.46%
15:15:01  crypto/randomInt.jsn=1000 max=100 min=-140737488355327 mode='sync'                                  ***    772.60 %      ±59.93%  ±80.74% ±107.14%
15:15:01  crypto/randomInt.jsn=1000 max=140737488355328 min=-10000 mode='async-parallel'                      ***     76.59 %       ±8.42%  ±11.20%  ±14.58%
15:15:01  crypto/randomInt.jsn=1000 max=140737488355328 min=-10000 mode='async-sequential'                    ***    384.21 %      ±29.08%  ±39.17%  ±51.95%
15:15:01  crypto/randomInt.jsn=1000 max=140737488355328 min=-10000 mode='sync'                                ***    662.43 %      ±42.43%  ±57.11%  ±75.66%
15:15:01  crypto/randomInt.jsn=1000 max=140737488355328 min=-100 mode='async-parallel'                        ***     72.87 %       ±8.36%  ±11.13%  ±14.50%
15:15:01  crypto/randomInt.jsn=1000 max=140737488355328 min=-100 mode='async-sequential'                      ***    384.58 %      ±30.60%  ±41.19%  ±54.58%
15:15:01  crypto/randomInt.jsn=1000 max=140737488355328 min=-100 mode='sync'                                  ***    658.07 %      ±39.90%  ±53.70%  ±71.14%
15:15:01  crypto/randomInt.jsn=1000 max=140737488355328 min=-140737488355327 mode='async-parallel'            ***     40.61 %       ±6.91%   ±9.19%  ±11.96%
15:15:01  crypto/randomInt.jsn=1000 max=140737488355328 min=-140737488355327 mode='async-sequential'          ***    203.08 %      ±20.27%  ±27.28%  ±36.14%
15:15:01  crypto/randomInt.jsn=1000 max=140737488355328 min=-140737488355327 mode='sync'                      ***    521.38 %      ±40.28%  ±54.24%  ±71.94%
15:15:01  
15:15:01  Be aware that when doing many comparisons the risk of a false-positive
15:15:01  result increases. In this case, there are 54 comparisons, you can thus
15:15:01  expect the following amount of false-positive results:
15:15:01    2.70 false positives, when considering a   5% risk acceptance (*, **, ***),
15:15:01    0.54 false positives, when considering a   1% risk acceptance (**, ***),
15:15:01    0.05 false positives, when considering a 0.1% risk acceptance (***)

tniessen added a commit that referenced this pull request Apr 3, 2021
PR-URL: #35110
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@tniessen
Copy link
Member Author

tniessen commented Apr 3, 2021

Landed in 5dae7d6, thanks for reviewing.

@tniessen tniessen closed this Apr 3, 2021
MylesBorins pushed a commit that referenced this pull request Apr 4, 2021
PR-URL: #35110
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 4, 2021
MylesBorins pushed a commit that referenced this pull request Apr 5, 2021
PR-URL: #35110
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. crypto Issues and PRs related to the crypto subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants