React 16.6.0 performance is worse than 16.5.2

[dragonflypl]

Some time ago I did refactoring of cell renderers components to achieve performance gain (I have a huge table). I did refactoring from functional stateless components to PureComponent. E.g.:

import React from 'react';
import PropTypes from 'prop-types';

class SomeCell extends React.PureComponent {
  render() {
    const { obj } = this.props;
    return (
      <>
        {obj.name}
        <br />
        {obj.isin}
      </>
    );
  }
}

SomeCell .propTypes = {
  obj: PropTypes.object,
};

export default SomeCell ;

Now I saw that React.memo was released so I updated to [email protected] and [email protected] (from 16.5.2) and refactored from PureComponent to React.memo with the expectation that it would be even faster (no lifecycle methods called, function smaller than class in memory etc...):

import React from 'react';
import PropTypes from 'prop-types';

const SomeCell = React.memo(function({ obj }) {
  return (
    <>
      {obj.name}
      <br />
      {obj.isin}
    </>
  );
});

SomeCell .propTypes = {
  obj: PropTypes.object,
};

export default SomeCell;

And to my surprise, performance went significantly down.

Profile data in prod mode (no addons in chrome) show that there's much more scripting happening than before with PureComponent (scripting time for my case went from 0.5s to 3.8sek).

After some investigation, it seems that it is not an issue with React.memo but with the new version of React. I've reverted cell renderers to PureComponent and left new [email protected] version and a result (significantly slower performance) is still present.

Do you have any idea what could be the issue with it?

[wojtekmaj]

Hmmm, I couldn't reproduce it with a quick JSFiddle. Could you share Chrome Performance data? You can safe profiles by right clicking the list of timeline sessions.

[dragonflypl]

Thanks for help. Here're files:

profiles.zip

It is react-dom update that slows things down.

[sophiebits]

If your code is open source, can you please link it? Assuming it's not…

Can you possibly run a git bisect to help us figure out what React commit changed it? If you have a local clone of the repo, you should be able to run yarn build and then you'll have packages you can install in build/node_modules. You should be able to go from the tag v16.5.2 to v16.6.0 (using PureComponent for all, no need to use memo).

Thanks in advance for helping to investigate this. Best guess is something change with batching, but it's hard to tell.

[dragonflypl]

I'll try. Btw. is there a way to download this repo but without cmd git clone ... (I'm behind proxy that is blocking github cloning)

[noahbenham]

@dragonflypl Can you try setting your git config to work with the proxy?

git config --global http.proxy http://proxyuser:[email protected]:8080
or
git config --global http.proxy http://proxy.server.com:8080

[sophiebits]

You should be able to try cloning over either https (in which case you can also use a proxy) or ssh. Not sure if both are blocked. If that doesn't work I would clone from a different network and then copy the whole folder between computers (including the hidden .git directory). In normal cases you could download the tgz from GitHub but since you need the history here that won't work.

[dragonflypl]

• 16.6.0 8b97a9c

• 2 [b753f76] Fix failing async tests in Node 10 - bad

• 4 [95a313e] Unfork Lazy Component Branches (Unfork Lazy Component Branches #13902) - bad

• 7 [e16cdd5] Always bail out timed out children even if they receive an update (Always bail out timed out children even if they receive an update #13901) - good

• 6 [7268d97] Centralize props memoization (Centralize props memoization #13900) - good

• 5 [36db538] Bugfix for React.lazy constructor must return result of a dynamic import #13886 (Bugfix for #13886 #13896) - good

• 3 [d9a3cc0] React.lazy constructor must return result of a dynamic import (React.lazy constructor must return result of a dynamic import #13886) - good

• 1 [c9be16f][scheduler] Rename priority levels ([scheduler] Rename priority levels #13842) - good

• 16.5.2 bec2dda

I did bisec and the output is:

$ git bisect good
95a313e is the first bad commit
commit 95a313e
Author: Sebastian Markbåge [email protected]
Date: Fri Oct 19 22:22:45 2018 -0700

I've double checked this & previous commit and I confirm that this one is the first where performance has dropped.

react-bisect-profiles.zip

[sophiebits]

@dragonflypl Thank you, this is really helpful.

[martinsb]

I can also confirm that our app performance has dropped since 95a313e

[sophiebits]

@sebmarkbage Any idea what could cause a perf regression from your "Unfork Lazy Component Branches" commit?

My instinct was incorrectly calling resolveDefaultProps leading to a missed bailout, but I verified in our test suite, only the tests that use lazy end up calling resolveDefaultProps so it doesn't seem to be that.

[sophiebits]

(cc @acdlite too)

[sophiebits]

@dragonflypl @martinsb After looking at the blamed commit I'm still not sure what this could be. Is there any chance one of you can post a reduced repro case showing the regression? It would really help us to debug if we can reproduce it locally.

We'll try to keep debugging but it will be hard to make good progress without a repro.

[gaearon]

I have a guess about what's going on here. We have a bunch of oldProps !== newProps comparisons that guard tagging with an Update effect and calling the lifecycles. I think those bailouts were broken by 95a313e lifting resolveDefaultProps() into non-Lazy code paths like here. For components with defaultProps, newProps are now always different (because we're passing the resolved props as newProps into ReactFiberClassComponent — which would always differ from the memoized props).

[gaearon]

Hmm no, I'm wrong. workInProgress.elementType === Component should protect against this.

[gaearon]

Gah. You already tried that. 😆

My instinct was incorrectly calling resolveDefaultProps leading to a missed bailout, but I verified in our test suite, only the tests that use lazy end up calling resolveDefaultProps so it doesn't seem to be that.

[sompylasar]

This may be minor but is there a reason to have two functions which call one another instead of one with all the code? From a brief review of the commit the inner function doesn't seem to be used anywhere else but I may be missing the bigger picture.

export function createFiberFromElement(
  element: ReactElement,
  mode: TypeOfMode,
  expirationTime: ExpirationTime,
): Fiber {
  const fiber = createFiberFromElementWithoutDebugInfo(
    element,
    mode,
    expirationTime,
  );
  if (__DEV__) {
    fiber._debugSource = element._source;
    fiber._debugOwner = element._owner;
  }
   return fiber;
}

[gaearon]

Google Closure Compiler (which is part of our build step) should inline it.

[localvoid]

I think that I've noticed the same issue, here is how to reproduce:

1. git clone https://github.com/localvoid/uibench-react.git
2. cd uibench-react
3. git checkout react-issue-13987
4. yarn
5. yarn build
6. http-server
7. Open http://127.0.0.1:8080/docs/16 url in the browser

Switching from uglify to terser fixed the issue:

[benwiley4000]

"Switching minifiers solves the problem" was not the conclusion I expected to find at the bottom of this thread...

[sophiebits]

OK, I investigated.

For reasons unknown, uglify has decided that now it should inline the FiberNode function into createFiber, so createFiber's implementation looks roughly like new function(t,p,k,m) { this.tag = t; ... }(t,p,k,m). VMs are much worse at optimizing this.

We should probably

1. figure out why uglify wants to do this and fix it upstream
2. maybe do something clever so that it doesn't try to inline it in React any more

[sophiebits]

Also – thank you @localvoid.

[sophiebits]

In particular, piping

(function() {
  function FiberNode(a) {
    this.tag = a;
  }
  function foo(a) {
    return new FiberNode(a);
  }
  global.foo = foo;
})();

to npx [email protected] -cm gives

!function(){global.foo=function(n){return new function(n){this.tag=n}(n)}}();

which is bad news.

[sophiebits]

@dragonflypl To confirm that what you're seeing is the same issue, can you (a) check that you're using uglify-es and (b) try checking out the same commit 95a313e from your bisect then editing packages/react-reconciler/src/ReactFiber.js to add a "FiberNode.foobar = {};" on line 273 (after the definition of FiberNode) then rebuild and see if it fixes the issue in your app?

I'm guessing you're using uglifyjs-webpack-plugin version 1. If you upgrade to version 2 (or to terser-webpack-plugin) then I think this issue will go away even on the release build of 16.6.0.

[aweary]

maybe do something clever so that it doesn't try to inline it in React any more

If you use a function expression it won't inline it

(function() {
  var FiberNode = function FiberNode(a) {
    this.tag = a;
  }
  function foo(a) {
    return new FiberNode(a);
  }
  global.foo = foo;
})();

!function(){var n=function(n){this.tag=n};global.foo=function(o){return new n(o)}}();

[sebmarkbage]

That's just not semantically equivalent. Seems like a pretty bad bug in uglify-es.

[sebmarkbage]

global.foo().constructor === global.foo().constructor

[sompylasar]

It looks like something to do with single_use of FiberNode.

uglify-es via node inspect

It does not inline if the constructor function is used more than once:

(function() {
  function FiberNode(fiberNodeArg) {
    this.tag = fiberNodeArg;
  }
  function foo(fooArg) {
    return new FiberNode(fooArg);
  }
  function bar(barArg) {
    return new FiberNode(barArg);
  }
  global.foo = foo;
  global.bar = bar;
})();

!function(){function n(n){this.tag=n}global.foo=function(o){return new n(o)},global.bar=function(o){return new n(o)}}();

[sompylasar]

uglify-es via node inspect

[sophiebits]

It’s rarely in a minifier’s best interest to inline variables/etc that are used many times, so that isn’t too surprising on its own. Note that 16.5.2 also calls the constructor only in one place. As @sebmarkbage points out though, it is incorrect to ever inline a function that “new” is applied to (unless it’s provable the instantiation runs at most once).

It seems like uglify-es is known to be buggy and is not maintained. We should probably focus our efforts on shifting people off it.

[sompylasar]

For reference: mishoo/UglifyJS#3156 (comment) uglify-es is stale

so it seems strange to abandon it. Since you haven't deprecated it on NPM, should I assume you're only taking a break from maintaining it?

uglify-es has been forked and the new project is called terser. For projects that need ES6 support, use it instead of uglify-es, since all new development will be happening over there (and things like webpack are switching over to it shortly too):
fabiosantoscode/terser

So, terser works correctly for this case, does not inline under new operator:

cat test.js; cat test.js | ./node_modules/terser/bin/uglifyjs -cm

(function() {
  function FiberNode(fiberNodeArg) {
    this.tag = fiberNodeArg;
  }
  function foo(fooArg) {
    return new FiberNode(fooArg);
  }
//  function bar(barArg) {
//    return new FiberNode(barArg);
//  }
  global.foo = foo;
//  global.bar = bar;
})();

!function(){function n(n){this.tag=n}global.foo=function(o){return new n(o)}}();

[danburzo]

Thank you for the report and the solution. I hoped transitioning from uglify-es to terser (via their respective Webpack plugins) would be as simple as switching packages, but unfortunately terser seems to be running out of memory consistently on some of our machines, whereas uglify-es does not... 😕 (this is with either parallel=true or false). We've downgraded back to React 16.5.2 / uglify-es until we figure this out.

[gaearon]

I guess we can close this then.

[benwiley4000]

Is it really fair to close this? It seems the proposed solution is to transition people from uglify to terser, which is not feasible in the very short term. Additionally as reported above by @danburzo terser has memory issues.

If it would be possible to edit the code (at least temporarily) to avoid the uglify-es bug that would be great.

[gaearon]

The solution is not to transition from uglify-js, it's to transition from uglify-es. This is a known buggy package which has already caused a bunch of bugs before. There is nothing we can do to fix this on our side — you are using a broken minifier. Even if we could patch around this particular bug (it's not clear how), you will keep bumping into these issues. It's also completely unmaintained so bugfixes won't come out.

There shouldn't be anything stopping you from reverting from uglify-es to a stable uglify-js (but that would require you to compile ES6 code beforehand). I would presume their memory characteristics are similar since uglify-js does a subset of what uglify-es (unsuccessfully) tries to do. You can also try switching to terser, and report memory issues to them.

[benwiley4000]

Ah I see. I wasn't aware of those issues but I guess it's true uglify-es is less widely used than uglify.

[robpalme]

@danburzo Would you be up for raising an issue on the Terser repo and providing a reproduction case? From what I can see, you are the first person to hit this issue.

[danburzo]

The solution is not to transition from uglify-js, it's to transition from uglify-es

Sorry, I meant uglify-es, amended my comment. We were using it via [email protected]. Seems like bumping the plugin to 2.xx is the easiest way to handle this at the moment, as it rolled back to uglify-js. (Will try it in the morning).

In regards to terser, I'll follow up in the repo's issues with anything I can find, as to not derail this thread too much.

[dragonflypl]

@sophiebits : I confirm that

• application of the FiberNode.foobar = {} fixed performance issue
• currently I am using Webpack4 that under the hood has "uglifyjs-webpack-plugin": "^1.2.4", that has "uglify-es": "^3.3.4",). Migration to latest uglifyjs-webpack-plugin or terser-webpack-plugin solved the issue.

Thanks all for support!

[martinsb]

I can also confirm that switching from Webpack 4's uglify-webpack-plugin to terser-webpack-plugin fixes the performance problem.
Thank you all!

[sophiebits]

Thank you for confieming!

[ryankshaw]

For anyone that needs a quick fix without having to upgrade to webpack 4 (because the terser-webpack-plugin only works webpack 4+) you can just turn off the reduce_funcs compression option for uglify-es (https://www.npmjs.com/package/uglify-es#compress-options). To do that in a webpack config it would look something like:

    new UglifyJsPlugin({
      uglifyOptions: {
        compress: {
          reduce_funcs: false
        }
      }
    })

to verify that it worked I did the following:
first:

cat test.js

(function() {
  function FiberNode(fiberNodeArg) {
    this.tag = fiberNodeArg;
  }
  function foo(fooArg) {
    return new FiberNode(fooArg);
  }
//  function bar(barArg) {
//    return new FiberNode(barArg);
//  }
  global.foo = foo;
//  global.bar = bar;
})();

This is how the output looks when using terser (which does not have the bug):

cat test.js | npx terser --compress --mangle

!function(){function n(n){this.tag=n}global.foo=function(o){return new n(o)}}();

this is how the output looks by default with uglify-es

cat test.js | npx uglify-es --compress --mangle

!function(){global.foo=function(n){return new function(n){this.tag=n}(n)}}();`

(has the bug)

and finally, this is how the output looks with uglify-es and the reduce_funcs option set to false

cat test.js | npx uglify-es --compress reduce_funcs=false --mangle

!function(){function n(n){this.tag=n}global.foo=function(o){return new n(o)}}();

(doesn't have the bug and is the same as what terser outputs)

[ryankshaw]

obviously, only do that ^ if you need a quick bug fix that has the least amount of change to your code. the real long-term fix is to get on a newer version of webpack which will use terser.

[uriklar]

I believe this issue should be made much more visible. I just got bitten by it. Also, because of it's nature, you might see it for the first time when pushing to production.

Maybe it should be added to the docs/release notes?

[dcworldwide]

@uriklar Is correct. We found this issue once we pushed to prod. 50k user system. Not a great DX for us.