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

Server rendering is slower with npm react #812

Closed
plievone opened this Issue Jan 4, 2014 · 59 comments

Comments

Projects
None yet
@plievone
Contributor

plievone commented Jan 4, 2014

I ran a few benchmarks on the server (modifying https://github.com/paulshen/react-bench to not use jsdom). The results were surprising, as the browserified react.js was about 30% faster than the npm version, even with NODE_ENV=production.

The performance ranking (test run time) was react.min.js < react.js < NODE_ENV=production node react < node react.

I suspect process.env is not a regular js object but perhaps a getter and thus carries a penalty when you test for "production" !== process.env.NODE_ENV everywhere.

Also the minified version might still perform best of all, as at least some time ago V8 used function source length (including comments) as a heuristic for function complexity / compilation time and thus affecting chances for optimization in some cases, but the effect might be negligible.

@plievone

This comment has been minimized.

Contributor

plievone commented Jan 5, 2014

process.env is indeed not a regular object, but defined from the native side, so that may be the culprit for server rendering regression:
https://github.com/joyent/node/blob/b922b5e/src/node.cc#L2326-L2335 and https://github.com/joyent/node/blob/b922b5e/src/node.cc#L2030-L2055
So this introduced a getter in quite time-sensitive places (around invariants etc) in your otherwise clean codebase. Can you reproduce the issue? /cc @petehunt @benjamn

@plievone

This comment has been minimized.

Contributor

plievone commented Apr 6, 2014

Lately I have been simply using a wrapped module

var React;
if (process.env.NODE_ENV !== 'production') {
  React = require('./react-with-addons-0.10.0.js');
} else {
  React = require('./react-with-addons-0.10.0.min.js');
}
module.exports = React;

Works great. You could consider switching npm package to some such scheme if there are no downsides.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Apr 7, 2014

My first attempt was to just add var __DEV__ = process.env.NODE_ENV !== 'production'; to the top of every file, but unfortunately uglify isn't smart enough to do dead code removal then. I wonder if we should have vendor/constants.js essentially make two copies of the code -- one for dev, one for prod, so

function x() { return __DEV__; }
module.exports = x();

transforms into

if (process.env.NODE_ENV !== 'production') {
  var x = function x() { return true; };
  module.exports = x();
} else {
  var x = function x() { return false; };
  module.exports = x();
}

so we only pay the getter cost once (per module) at require time.

@benjamn Is doing this easy with recast? It's not obvious to me if this.replace interacts well with cloning the AST.

@mridgway

This comment has been minimized.

Contributor

mridgway commented May 16, 2014

I would love to see this get some attention. 30% improvement out of the box would be awesome. Is there a lot of work to do here?

@petehunt

This comment has been minimized.

Contributor

petehunt commented May 16, 2014

I think eventually we will want people to do a build step for server rendering, i.e. webpack's --target node mode. Especially if people start expressing static asset dependencies as require() statements.

@syranide

This comment has been minimized.

Contributor

syranide commented May 17, 2014

I'm using webpack and new webpack.DefinePlugin({'process.env.NODE_ENV': '"production"'}) was enough to solve it for me. Is there no such thing in browserify?

@andreypopp

This comment has been minimized.

Contributor

andreypopp commented May 17, 2014

For browserify there's envify transform which automatically activates for react package. You just need to set NODE_ENV before bundling:

NODE_ENV=production browserify app-which-requires-react.js

Thus browserify also can be used for bundling for server rendering.

sophiebits added a commit to sophiebits/react that referenced this issue May 20, 2014

When packaging, copy the code and inline `__DEV__`
Fixes facebook#812.

Previously, this code

    module.exports = moo();
    function moo() { return __DEV__; }

would be transformed to

    module.exports = moo();
    function moo() { return "production" !== process.env.NODE_ENV; }

Now, it's transformed to:

    if ("production" !== process.env.NODE_ENV) {
      var moo = function() { return true; };
      module.exports = moo();
    } else {
      var moo = function() { return false; };
      module.exports = moo();
    }

which reduces the getter cost to one test at require time instead of inline for every `__DEV__` check, warning, and invariant.

The unminified build is about twice as large now (but it's about the same after gzipping) and the minified build is the same size.
@karlmikko

This comment has been minimized.

Contributor

karlmikko commented Aug 6, 2014

Call me all crazy, just had a left field idea. Could the transformer that builds the npm module create 2 complete copies of react? one for dev and one for production?

In the the normal version of react var React = require('react') could you there look at the NODE_ENV and swap which module is returned? This way on server you only get the performance hit once;

@zpao

This comment has been minimized.

Member

zpao commented Aug 6, 2014

That's not too crazy. It's a bit annoying from a build process though. I'd love to know what's happening at a larger scale in the node community. It seems like we shouldn't be the first people to encounter this in a larger scale.

A couple other ideas…

What if we set global.__DEV__ = process.env.NODE_ENV in our main module and leave __DEV__ in our code everywhere else. There's some obvious bad behavior there (setting globals). So we could do variants. Only set it when it's not already set. Change __DEV__ to something more unique at build time (__REACT__DEV__) and then set that. If we switch to webpack for building the browser bundles, then the DefinePlugin solution is super easy, and I'm sure there's a way to do this in browserify too.

Another idea: Have a RuntimeEnvironment modules which memoizes process.env.NODE_ENV. This is sounding like something node should do itself, but the code @plievone linked to uses ObjectTemplate (which if I'm understand a bit of V8 right doesn't do any caching because values can change). There would still be a lookup cost, but it would just be a property access, not a getter and not a call out to get current environment variables (getenv). This one is trickier because it would mean more code and we won't do this internally. I'd prefer a different solution, even if it's more hacky.

If all you're using is React and nothing from react/lib/*, then you could do something like @plievone mentioned above. We started shipping the browerified bundles in the npm package, so require('react/dist/react') and require('react/dist/react.min') will end up giving you the bundles which don't access process.env.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Apr 1, 2015

I don't think we're going to do anything specifically for server rendering other than bundling the builds in dist like @zpao mentioned, but we might change the npm package eventually to be prebuilt using closure or similar.

@stryju

This comment has been minimized.

stryju commented May 11, 2015

just curious why

if (process.env.NODE_ENV !== 'production') {
  var x = function x() { return true; };
  module.exports = x();
} else {
  var x = function x() { return false; };
  module.exports = x();
}

and not

module.exports = (process.env.NODE_ENV !== 'production');

?

@sophiebits

This comment has been minimized.

Member

sophiebits commented May 11, 2015

It was just an example. If that was the actual code I'd do the direct assignment like you suggested.

@stryju

This comment has been minimized.

stryju commented May 11, 2015

ah, ok - thanks :)

@basketofsoftkittens

This comment has been minimized.

basketofsoftkittens commented May 18, 2015

any movement on this?

@sophiebits

This comment has been minimized.

Member

sophiebits commented May 19, 2015

No movement, though you can already require the builds from react/dist as @zpao mentioned above.

pascience added a commit to pascience/invariant that referenced this issue May 28, 2015

browser.js: potential minification checks process.env.NODE_ENV
Before this, my errors didn't get minified in production.

I'm not using `envify` but webpack's `DefinePlugin` to replace `process.env.NODE_ENV` by its string value in all the modules I use.

I don't care much about facebook/react#812 for now. The reason for this commit is not performance ; I didn't integrate uglify into my build workflow yet, so dead code elimination doesn't happen anyway.
@jiyinyiyong

This comment has been minimized.

jiyinyiyong commented Jun 9, 2015

@syranide since you mentioned Webpack, do you use Webpack for server side code?

@mhart

This comment has been minimized.

mhart commented Jan 19, 2016

@STRML oh for sure it is – I completely agree with that – I don't think it should be cached by Node either, just the current implementation has (or had) room for improvement.

@STRML

This comment has been minimized.

Contributor

STRML commented Jan 19, 2016

As I mention in that ticket, this is the main problem:

// This would work if it were a const
var NODE_ENV = process.env.NODE_ENV;
//...
if(NODE_ENV !== 'production') { // this will not be eliminated
  // debug code
}

A /** @const */ annotation in UglifyJS will fix this.

@mhart

This comment has been minimized.

mhart commented Jan 19, 2016

That seems like a decent solution to me.

I'm personally not a fan of the use of a global __DEV__ everywhere in the React code – again, it feels really build-tool specific. I'd prefer it be imported from a config file or declared in each module or something similar. Principle of least surprise and all that.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jan 19, 2016

@mhart I would be happy to do that, but it is important that we have a way to eliminate dev-only code in prod builds and that users of React from npm have a way to do the same. Obviously we can use whatever tools we want for our stack easily but for npm currently we suggest envify/webpack.DefinePlugin. Not inherently opposed to changing that but we'd need a good proposal and reason.

@mhart

This comment has been minimized.

mhart commented Jan 19, 2016

@spicyj yeah, I hear ya – there's a lot of code now in React that depends on that behaviour.

Not sure I fully understand your comment "for npm currently we suggest envify/webpack.DefinePlugin" – what do you mean "for npm"? React code is compiled before it's published to npm, right?

My general feeling is that it would be great if there were no FB/React-specific idioms in the code – so that it's treated as though, if v8 (or other engines) had all the ES2015/2016 features needed, then you could run the code on Node.js without even needing a compiler. Currently that's not really the case. (I'm sure you could do some sort of global.__DEV__ = false trick or something before requiring any React modules, but still, that would be a polluting hack)

If there were a way such that __DEV__ was required, or declared, per module – and code elimination still worked as it does now for the dist builds – would you be amenable to a PR along those lines?

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jan 19, 2016

I mean: if you use webpack or browserify in conjunction with react from npm, you should be able to eliminate React dev-only code from your prod builds. envify lets us do that easily in browserify as it copies the NODE_ENV from when you make your build, and webpack.DefinePlugin lets you configure your build to replace production.env.NODE_ENV with a constant which then can get constant-folded and minified out. This use case is important.

If there were a way such that __DEV__ was required, or declared, per module – and code elimination still worked as it does now for the dist builds – would you be amenable to a PR along those lines?

Yes, if it works for the case of React devs using browserify/webpack too, not just our premade builds.

@mhart

This comment has been minimized.

mhart commented Jan 19, 2016

@spicyj as an aside, just looking through the code now, and aside from the global __DEV__, I haven't seen anything that's not supported out of the box in Node.js right now – is there a reason, aside from __DEV__, that the non-dist code is even compiled and transformed into a different structure before publishing to npm?

@STRML

This comment has been minimized.

Contributor

STRML commented Jan 19, 2016

Well, it does set up the team to move to ES6 for src/, which will be important in the near future.

I've submitted a PR to UglifyJS2 as you can see above. If it's accepted this will solve the problem.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jan 19, 2016

@STRML We'd still need everyone using React to upgrade to that version of UglifyJS so I can't promise that we'll take it…

@STRML

This comment has been minimized.

Contributor

STRML commented Jan 19, 2016

Just everyone using React in combination with webpack or browserify and not using the dist/ build.

Alternatively they could use the DefinePlugin to set __DEV__ to false to get the dead code elimination.

@mhart

This comment has been minimized.

mhart commented Jan 19, 2016

@spicyj just to clarify, do you mean "everyone using React who also currently uses UglifyJS with it"?

@mhart

This comment has been minimized.

mhart commented Jan 19, 2016

Just so I'm clear, there are a few users here that I can think of. Those, on the browser:

  1. Using require('react/dist/...') everywhere already, or as a global <script>
  2. Using browserify/webpack + envify/DefinePlugin
  3. Using browserify/webpack + envify/DefinePlugin + UglifyJS
  4. Using browserify/webpack + plugin to transform require('react') to require('react/dist/...') or window.React

So 1 and 4 are taken care of by the compilation in dist. What's the main reason for even supporting 2 and 3? Is it because of things like require('react/lib/...')?

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jan 19, 2016

2 and 3 are the most natural for many people. If you are using browserify then envify gets used automatically because of our config in package.json. Almost everyone minifies their code in prod.

It is true that we could recommend #4 instead for many cases, but it does fall apart in the case of requiring submodules. We don't support this for external users because we consider the modules private but the addons packages use this pattern. Various third-party projects (unsupported by us) also make use of this.

@mhart

This comment has been minimized.

mhart commented Jan 19, 2016

I think the only reason 2 and 3 are "most natural" is just because process.env is used everywhere so you have to use something just to get it to even work out of the box. It would be equally easy to setup the config in package.json to use literalify or browserify-shim or whatever.

I use 4 whenever I can and it's no more complicated than 2 or 3 for normal usage – I guess it's just a pity that users are encouraged to "reach in" to react/lib/ for various addons.

Thanks for the clarification.

So it's those doing 3 who are expecting their current UglifyJS setup to eliminate any code using __DEV__ that you're concerned about?

@sophiebits

This comment has been minimized.

Member

sophiebits commented Jan 19, 2016

I guess it's just a pity that users are encouraged to "reach in" to react/lib/ for various addons.

To clarify: we recommend users require react-addons-transition-group or similar, which (currently) reaches into react but that's an implementation detail.

So it's those doing 3 who are expecting their current UglifyJS setup to eliminate any code using __DEV__ that you're concerned about?

Yes.

@mhart

This comment has been minimized.

mhart commented Jan 19, 2016

we recommend users require react-addons-transition-group or similar

Ah cool, good to know.

mrtnbroder added a commit to mrtnbroder/universal-react-webpack-boilerplate that referenced this issue Jan 26, 2016

uglify server build
this gets rid of some runtime checks against the process.env property, which slows down node A LOT, see facebook/react#812
@santiagoaguiar

This comment has been minimized.

santiagoaguiar commented Jul 29, 2016

We have been bitten by this one too. Would be great if some note about this behavior was present on https://facebook.github.io/react/docs/environments.html , it's a large performance impact, and is hard to find this issue unless you have already done all the investigation work.

@pmoleri

This comment has been minimized.

pmoleri commented Jul 29, 2016

I would like to add that this issue also affects Electron applications.

Now the work arounds seem to be:

  • Include the minified version of React globally via <script> tag, it would only work in Electron and it isn't the node way, where one should be able to track every used object just seeing the requires. It also makes it difficult to switch between dev/prod.
  • Use the proposed hack to replace process.env with a regular object. Not good, as we are forced to overwrite node api just because we want to use React.
  • If using babel: transform-inline-environment-variables... by default babel doesn't transpile node_modules, this can be configured but doesn't look like a work around.

Note that replace every require() to react/dist/react.min.js isn't a solution because react-dom automatically requires from react root, not from react/dist.

Why is this issue closed?
It seems to me that React should be able to initialize its configuration using NODE_ENV, then rely on its own objects, and optionally offer a React.setProduction(true/false) function to change the value later.
Is there any reason why this wouldn't work?

@STRML

This comment has been minimized.

Contributor

STRML commented Jul 29, 2016

Technically this could be fixed now as UglifyJS supports /* @const */ annotations, and it may be easier still as the team moves the build to Rollup in #7178.

In combination with Rollup, it would be simple to export a __DEV__ var from a constants file and have it effectively tagged by Uglify for dead code elimination in production mode.

@jknight12882

This comment has been minimized.

jknight12882 commented Dec 23, 2016

Adding my two cents here, I was able to work around this by adding the following to the top of my server.js

if (process.env.NODE_ENV === 'production') {
    require('react/dist/react.min.js');
    require.cache[require.resolve('react')] = require.cache[require.resolve('react/dist/react.min.js')];

    require('react-dom/dist/react-dom-server.min.js');
    require.cache[require.resolve('react-dom/server')] = require.cache[require.resolve('react-dom/dist/react-dom-server.min.js')];
}

I had previously wrapped React using local modules file://... as was suggested above, but this caused a lot of issues with npm shrinkwrap. This seems to get the job done without too much overhead or complexity.

@rtsao

This comment has been minimized.

rtsao commented Feb 10, 2017

For future reference, the above snippet is broken per: #8788

@STRML

This comment has been minimized.

Contributor

STRML commented Mar 8, 2017

Referencing facebook/fbjs#86 which will fix this now that UglifyJS can properly eliminate dev code (even if it references a var that never changes) with its new reduce_vars option, which is a default.

See facebook/fbjs#86 (comment) for more context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment