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

remove if the if condition by assigning function statically #5

Closed
KrishnaPG opened this issue Nov 26, 2018 · 6 comments
Closed

remove if the if condition by assigning function statically #5

KrishnaPG opened this issue Nov 26, 2018 · 6 comments
Labels
perf Performance improvements

Comments

@KrishnaPG
Copy link

The crypto availability is being checked every time randomBytes method is called here

This if else condition checking can be done just once at the beginning, and assign the correct function to a variable, something like below:

const randomBytes = (typeof crypto !== 'undefined') ? randomCrypto : randomMath;

let bufIdx = uuid.BUFFER_SIZE;
function randomCrypto(n) {
      if ((++bufIdx + n) > uuid.BUFFER_SIZE) {
        bufIdx = 0;
        if (crypto.getRandomValues) {
          buf = new Uint8Array(uuid.BUFFER_SIZE);
          crypto.getRandomValues(bytes);
        } else if (crypto.randomBytes) {
          buf = crypto.randomBytes(uuid.BUFFER_SIZE);
        } else {
          throw new Error('Non-standard crypto library');
        }
      }
      return buf.slice(bufIdx, bufIdx + n);
}
function randomMath(n) { 
      r = [];
      for (i = 0; i < n; i++) {
        r.push(getRandomInt(0, 255));
      }
      return r;
}

This further improves the performance.

@jchook jchook added enhancement perf Performance improvements and removed enhancement labels Nov 26, 2018
@jchook
Copy link
Owner

jchook commented Nov 26, 2018

Hey thanks very much for your thoughtful suggestion.

I tried the changes and didn't see any material performance gains. However, in testing, I did find a bug! So ultimately I think it was a big win.

@KrishnaPG
Copy link
Author

The performance gains will not be visible at the code level. They will be at the energy consumption level of the underlying CPUs, as the if-else branching logic affects the Processor cache pipeline, especially when the processors have branch-prediction logic built-in. If you want to measure the performance at the code level, then try with RDTSC performance counters.

@jchook
Copy link
Owner

jchook commented Nov 27, 2018

Interesting.

What do you think of this PR? #6

@jchook jchook reopened this Nov 27, 2018
@KrishnaPG
Copy link
Author

KrishnaPG commented Nov 27, 2018

Looks fine to me, except for one small thing:

At Line 76, the else condition for randomBytesMath may need to be outside the if (typeof crypto !== 'undefined'), since it should get executed even when crypto === undefined. Currently it looks like it will not run if crypto is undefined.

@KrishnaPG
Copy link
Author

Also, It would be great if nacl is supported, if possible.
Ref: Best Practices of Security

TweetNACL

nacl.randomBytes internally picks up the right method based on the current platform.

@jchook
Copy link
Owner

jchook commented Sep 15, 2019

I got around to implementing this performance fix in #11 and also exposed uuid.randomBytes() so you can override it with TweetNACL. Thanks again for the great suggestions.

@jchook jchook closed this as completed Sep 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf Performance improvements
Projects
None yet
Development

No branches or pull requests

2 participants