Performance regression in relay 0.9.x (for server rendering)

[rodrigopr]

Hi, we recently upgraded from 0.7.3 to 0.9.2 and noticed that SSR latency was almost twice slower than before.

Investigating using v8-profiler I've found that this happened due to a lot of module require being called inline.

Most of then seem to be from require('fbjs/lib/invariant').

I've changed inlineRequire to false in scripts/getBabelOptions.js and it fixed for us:

writeRelayQueryPayload runs about 6 times faster after the change.

---

Is there any problem in disabling inlineRequire?

[josephsavona]

Thanks for letting us know about this. For some context, we measure every substantive change internally on real devices, so we know that the core logic of e.g. writeRelayQueryPayload hasn't regressed. However, we don't have an equivalent set up to make sure performance doesn't regress as a result of OSS build config - this is definitely something we should address as we want to ensure OSS users get the same performance we see internally. @rodrigopr, thank you again for your bringing this to our attention!

The inlineRequires settings causes require calls to be moved to the point where a module is referenced, instead of at the top of the file. You've probably noticed this while debugging, for others looking at this issue, the effect is as follows:

// Before:
var foo = require('foo');
function bar() {
  return foo();
}

// After:
function bar() {
  return require('foo')();
}

On mobile devices we have found that this can improve performance by delaying the work of requiring modules until they are needed (at the cost of some overhead on future invocations). On the server, the inverse (not doing inline requires) is clearly better, as indicated by your findings.

This might be a good argument for having two different builds: the default one optimized for clients, the other for use on the server (we could also disable the query/fragment cache on this one). Thoughts?

Also ccing @zpao about the OSS inline requires perf.

[cpojer]

Which require implementation are you using @rodrigopr? The one we use at Facebook is incredibly optimized (though not entirely CommonJS compatible) and it is possible that whichever one you are using isn't optimized enough. At Facebook we found that the overhead of requiring the same module over and over again is actually not a big deal.

[rodrigopr]

Thanks for the context, it makes sense.

A very naive alternative for lazy module loading that would improve the perf for us is something like:

var _foo;
var foo = () => {
  if (!_foo) { _foo = require('fbjs/lib/invariant'); }
  return _foo;
}

function bar() {
  return foo()();
}

(via the inline-requiresbabel plugin)
But I guess Facebook optimized require implementation does lot of others stuff (optimize amount of loaded modules for mobile maybe?)

---

@josephsavona, the two build setup would work for us.
But it might add complexity for new users as I assume will require some change on the bundler.

@cpojer, we are using node (5.12.0) default implementation.
Is Facebook implementation opensource? I'd love to see how it works.

[zpao]

The FB implementation is client-side, it's not really relevant to the node case. Basically because there's not filesystem involved, every module ends up with a unique name and once it's loaded once, the cache is primed and it a quick lookup. Something like this:

var exportsCache = {};
function require(module) {
  if (exportsCache.module) {
    return exportsCache.module;
  }
  // resolve and add to cache
}

Inline requires make less sense for node. Node is doing caching of require paths too but it still does a bunch of work to make sure the cached thing is correct. You can follow the code along here: https://github.com/nodejs/node/blob/cc189370dd4a124bf3988c1ab8aca2d274ecae48/lib/module.js#L465

We could probably have a smarter inline-requires transform that instead of just inlining requires, we generate a fn that does the caching first and fwds on to regular require. It would probably break things like browserify & webpack though as requires can no longer be statically resolved without special knowledge of the transform semantics.

Alternatively, we just stop building here with inline requires. Initial parse time goes up server-size (but should be constant client-side). Startup time would theoretically go up in both cases though as it'll have to run modules.

[voideanvalue]

@rodrigopr: You can see our implementation for React Native here. As @zpao mentioned though, you probably won't be able to drop in and replace node's implementation (all modules must be define-ed before they can be require-ed, relative paths are not supported since we rewrite relative paths to canonical ids).

The naive alternative that you suggested is actually what Nuclide does for converting import statements to requires. I don't think it's too farfetched to make the inline-requires babel-plugin do something similar (as an optional feature that can be turned on by modifying babelOptions).

[rodrigopr]

Thanks @zpao, @voideanvalue.

As @voideanvalue said, we can add an option to inline-requires behave like implemented on Nuclide, seems this might work well for both client and node.
Do you all agree? If yes, I can send a PR to fbjs and update usage here.

[zpao]

@voideanvalue How much value do we think inline requires actually provides for the final builds? I understand it's a divergence from our internal pattern so there's a possibility of subtle differences, but I think the likelihood is low (we saw issues going the other way but I don't expect the same problems this way).

[voideanvalue]

Do you all agree? If yes, I can send a PR to fbjs and update usage here.

@rodrigopr: I'd be fine with that as long as we're not changing the default option. What do you think, @josephsavona @zpao @cpojer?

@zpao: Apart from a potentially slowed down initialization, circular dependencies might become unsafe without inline requires. We could probably write a test to guard against that though...

[josephsavona]

@rodrigopr were you able to resolve this? If so, what solution did you end up with?

[rodrigopr]

@josephsavona we're using a custom build of relay 0.9.2 with inlineRequire disabled, it solved the problem. Didn't have an opportunity to contribute with a definitive solution (PR) for it yet.

[AndrewIngram]

So, I've just had to look into this same change, and i'm seeing the same approximate 2x render speedup on the server. I'm also seeing a comparable degradation on the client.

So it seems like I need access to both variants on the build in a single project. One option would be for Relay to ship a separate libServer folder in the distributions, then leave it up to us to use some alias trickery during our builds to decide which one to use.

The slow response times we're seeing caused by this issue not only impacts our users pretty heavily, but also practically doubles the cost of running our website.

[denvned]

In my projects I solved this problem by webpack-ing the node_modules into the app's server-side bundle (i.e. I have thrown away traditional externals config). And you probably need to do it anyway because of react/react#812 (with DefinePlugin and UglifyJS).

Another, much more hacky option is monkey patching node's require to use your own cache:

const modulePrototype = module.constructor.prototype;
const originalRequire = modulePrototype.require;

modulePrototype.require = function (path) {
  let module;

  if (!this.myModuleCache) {
    this.myModuleCache = new Map();
  } else {
    module = this.myModuleCache.get(path);
  }

  if (!module) {
    module = originalRequire.call(this, path);
    this.myModuleCache.set(path, module);
  }

  return module;
};

And a similarly hacky solution for the process.env problem: react/react#812 (comment)

[wincent]

(Spring cleaning.) Thanks for the discussion everybody. We have a couple of workarounds (such as custom builds) in the thread above, so I am going to close this. If anybody wants to take a stab at implementing a PR that would make toggling this easier, we'd be happy to take a look at that too.