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

Slow require() of v1 #189

Closed
chgibb opened this issue Apr 21, 2017 · 5 comments
Closed

Slow require() of v1 #189

chgibb opened this issue Apr 21, 2017 · 5 comments

Comments

@chgibb
Copy link

chgibb commented Apr 21, 2017

Using require("uuid/v1") under Electron, I've noticed anywhere from 2x to 4x increase in window load time. I've tracked this to the unwrapped call to rng() which in turn synchronously calls crypto.randomBytes(). From the NodeJS docs:

The crypto.randomBytes() method will block until there is sufficient entropy. This should normally never take longer than a few milliseconds. The only time when generating the random bytes may conceivably block for a longer period of time is right after boot, when the whole system is still low on entropy.

This is unfortunately the behvaviour I'm seeing. The loading of the entire window is being held up by the call to crypto.randomBytes. Changing my use of uuid/v1 to uuid/v4 fixes the problem.

If there is sufficient interest in modifying uuid/v1 to use the asynchronous version of crypto.randomBytes or otherwise wrap the synchronous version so it is not called on require time I would be happy to open a PR.

@broofa
Copy link
Member

broofa commented Apr 21, 2017

2x to 4x increase in window load time

What kind of time are we talking about here? msecs? secs?

Changing my use of uuid/v1 to uuid/v4 fixes the problem

v4 uses rng() as well

The only time when generating the random bytes may conceivably block for a longer period of time is right after boot, when the whole system is still low on entropy.

That's for node.js though, not the browser, right? How does that affect window load time in your clients? I would expect the entropy of a node.js server to have been established by the time it's ready to start handling requests.

interest in modifying uuid/v1

I think you mean rng.js, as that's where this issue should be addressed.

I'm open to fixing this, but adding/converting to an async api is a non-trivial change. I'd like to make sure it's actually solving a real problem. That your problem goes away when you switch to v4() has me suspicious... 😕

(I could be way off base on all of this, btw... I won't pretend to know with certainty what crypto.randomBytes() is doing under the hood.)

@chgibb
Copy link
Author

chgibb commented Apr 22, 2017

Thanks for the quick response @broofa. In terms of slowdown, we're talking the difference between 20ms and 300ms.

Electron is a framework for building desktop apps with web technologies. Each window is a separate process, including a separate instance of NodeJS. Therefore each window's requires are evaluated each time a window is loaded. The difference between v1 and v4 in my case is the unwrapped usage of crypto.randomBytes.

From the v1 source;

// Unique ID creation requires a high quality random # generator.  We feature
// detect to determine the best RNG source, normalizing to a function that
// returns 128-bits of randomness, since that's what's usually required
var rng = require('./lib/rng');
var bytesToUuid = require('./lib/bytesToUuid');

// **`v1()` - Generate time-based UUID**
//
// Inspired by https://github.com/LiosK/UUID.js
// and http://docs.python.org/library/uuid.html

// random #'s we need to init node and clockseq
var _seedBytes = rng();

Each time uuid/v1 is required (in my case on startup of each window, when entropy is low), cyrpto.randomBytes is invoked.

Compared to v4

var rng = require('./lib/rng');
var bytesToUuid = require('./lib/bytesToUuid');

function v4(options, buf, offset) {
  var i = buf && offset || 0;

  if (typeof(options) == 'string') {
    buf = options == 'binary' ? new Array(16) : null;
    options = null;
  }
  options = options || {};

  var rnds = options.random || (options.rng || rng)();

  // Per 4.4, set bits for version and `clock_seq_hi_and_reserved`
  rnds[6] = (rnds[6] & 0x0f) | 0x40;
  rnds[8] = (rnds[8] & 0x3f) | 0x80;

  // Copy bytes to buffer, if provided
  if (buf) {
    for (var ii = 0; ii < 16; ++ii) {
      buf[i + ii] = rnds[ii];
    }
  }

  return buf || bytesToUuid(rnds);
}

module.exports = v4;

Where the only thing happening at require time is the the further requiring of .lib/rng as well as ./lib/bytesToUuid and the creation and exporting of the v4 function. In v4, crypto.randomBytes is not invoked until the v4 method is.

This is where the performance difference is coming from. The difference in time in the process' lifecycle when crypto.randomBytes is being called. At startup (when entropy is low) in v1, versus at some point in the future, during runtime when entropy is (presumably) higher in v4.

@niik
Copy link

niik commented Apr 27, 2017

We're seeing this behavior as well albeit with a much higher penalty (around 1 second for the initial call)

image

image

We're using webpack and typescript and import the v4 version using an import statement, this causes the v1 version to get loaded as well.

image

It seems like the initial call to crypto.randomBytes can be extraordinarily expensive when running under electron. I suspect this is something we can improve in electron and I'll open an issue there to discuss it.

For now we've worked around the issue by creating our own wrapper around uuid that sources entropy using window.crypto instead of the Node.JS crypto module and the initial load time is now trivial. I threw up my workaround in a gist if you're interested @chgibb.

@niik
Copy link

niik commented Apr 27, 2017

Update: there's already an issue tracking this in electron, see electron/electron#2073

@chgibb
Copy link
Author

chgibb commented Apr 27, 2017

Thanks @niik !!

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

No branches or pull requests

3 participants