Optimize _bindAutoBindMethods

[mridgway]

We are running into some performance issues using React on the server side. In large component trees, we are finding that render time increases linearly. In our case, we are seeing render times over 80ms for a relatively simple page (112 components). During profiling we have found that the _bindAutoBindMethods is showing up at the top of our trace:

   ticks  total  nonlib   name
    245    0.6%    0.6%  LazyCompile: ~L._bindAutoBindMethods /Volumes/Projects/app/node_modules/react/dist/react-with-addons.min.js:13
    146    0.4%    0.4%  LazyCompile: ~L.mountComponent /Volumes/Projects/app/node_modules/react/dist/react-with-addons.min.js:13
     75    0.2%    0.2%  LazyCompile: Join native array.js:119
     72    0.2%    0.2%  LazyCompile: bind native v8natives.js:1578
     64    0.2%    0.2%  LazyCompile: ~i.createElement /Volumes/Projects/app/node_modules/react/dist/react-with-addons.min.js:14
     45    0.1%    0.1%  Callback: execute
     38    0.1%    0.1%  LazyCompile: hasOwnProperty native v8natives.js:249
     37    0.1%    0.1%  LazyCompile: *parse native json.js:55
     31    0.1%    0.1%  LazyCompile: *n /Volumes/Projects/app/node_modules/react/dist/react-with-addons.min.js:16

I went in and added hrtime() calls inside _bindAutoBindMethods to see how much time was contributing to the rendering, and found that over 23ms was used just within this method. This may not seem like a lot, but this has pretty dramatic effect on throughput when the server is rendering synchronously.

Is there a way to optimize this so that binding happens lazily? On the server, many of the methods may not even be called (event handlers, lifecycle events, getDOMNode, etc.).

[zpao]

cc @sebmarkbage. We were talking about changing the autobinding recently. He might have some insight here (especially in context of ES6 classes)

[sebmarkbage]

The idea is to move to an explicit binding model using ES6 classes + maybe property initializers. This lets you choses whether to bind at construction time or as they're used in render according to your optimizations.

Maybe there is a way we could change the optimization for server side render but I doubt it since in theory you'd need to bind it at least once even on the server. It's possible to call a callback in componentWillMount.

I would also suggest that you try a different strategy for benchmarking. Profiling tools tend to exaggerate small methods that gets called a lot.

If you comment out the call to _bindAutoBindMethods in React, and then profile by manually timing the whole operation, without a profiler enabled... Do you see the same results?

[mridgway]

WRT to the profiling manually: that's what the hrtime test I mentioned above is doing. I removed the profiler and just used process.hrtime around the _bindAutoBindMethods calls and it added up to 23ms.

In ReactCompositeComponent#mountComponent:

        var startTime = process.hrtime();
        this._bindAutoBindMethods();
        var diff = process.hrtime(startTime);
        bindTime += (diff[0] * 1e9 + diff[1]);

Printing out bindTime at the end of rendering gives me the total nanoseconds used in _bindAutoBindMethods.

I will try removing the method to see how it effects total rendering time. I was concerned before that the render would break if this was removed for instance with componentWillMount.

[mridgway]

Yeah, unfortunately this breaks some of our components when testing without calling _bindAutoBindMethods, so I can't test the difference.

[syranide]

@mridgway Just to be safe, have you considered the cost of calling hrtime? (i.e. move _bindAutoBindMethods outside of it and leave it empty and measure) I don't know what you're rendering, but hrtime might be an expensive external call that prevents optimizations and it could contribute significantly to the cost. Probably isn't, but you never know... 23ms just seems too high...

[sophiebits]

You could also maybe try calling _bindAutoBindMethods twice to see what that adds?

[mridgway]

You're right: hrtime was contributing quite significantly as well. I re-ran the tests by combining a few calls to the method. I was able to get the single _bindAutoBindMethods measurement down to around 6ms on average by removing some of my debugging. By adding more calls as @spicyj mentioned I was able to get the difference between calling it twice and thrice:

• 1x: ~6.0ms
• 2x: ~8.1ms
• 3x: ~10.2ms

So altogether it looks like the _bindAutoBindMethods is only taking up about 2-3ms per render. This isn't nearly as bad as I initially thought. It is still surprising how long it takes, but with this number it's probably not worth any extra effort to minimize this impact before @sebmarkbage's changes are in.

We're still trying to figure out why our render times are so high though.

[mridgway]

No matter how I measure it, _bindAutoBindMethods is always the largest contributor to our latency/performance issues. By running applications side-by-side with benchmark.js on node, with stock React vs React with _bindAutoBindMethods commented out, I see 15% performance gains (ops/sec rendering app) with it commented out.

Migrating to ES6 components helps for our custom components, but not for the built-in components which use React.createClass.

It seems like there are two ways to mitigate this issue:

1. Migrate internal components to not use React.createClass or
2. Remove _bindAutoBindMethods so that classes created with React.createClass behave similarly to ES6 classes as far as binding

[jimfb]

Migrating to ES6, where possible, seems better. It's hard to justify optimizing a feature that is "not the future". We should increase the priority of migrating the built-in components to ES6 if we're really going to get a 15% perf boost on SSR.

[sophiebits]

@JSFB Not too hard to justify it when everyone uses it… but I don't know how to make autobinding faster.

[mridgway]

With NODE_ENV=production I'm seeing 20-25% improvement. This is in node.js using renderToStaticMarkup. This is the script I'm running to test: https://github.com/mridgway/flux-example/blob/bench/bench-chat.js. I am using React 0.13.1 from NPM and applied the patch from #3615 to make contexts work with multiple React's in same process.

[zpao]

Yea, not really hard to justify at all.

We're still talking about server-side here right? Perhaps there's something we can do so we don't actually create all the bound functions upfront? Maybe have placeholder functions which lookup the real function and use call?

[sophiebits]

@mridgway Can you try requiring 'react/dist/react.min.js' instead of the main React module as you're currently doing and compare?

[sophiebits]

(Wondering if this is #812.)

[mridgway]

I don't think it is related to #812, since both tests are hitting the process.env. I will see if I can get the test working with the min.js although patching it is kind of difficult for me since I have to add the context changes as well.

[sophiebits]

Here's a build from #3615: https://s3.amazonaws.com/uploads.hipchat.com/6574/26709/Ojl4jYZgCmwTdo1/react.min.js

And one with the call to bindAutoBindMethods removed:
https://s3.amazonaws.com/uploads.hipchat.com/6574/26709/iVzAHdzNHgEGOMh/react-nobind.min.js

[mridgway]

Ran the test with the above files and still seeing about 25% difference. The changes that I made for the benchmark are here: mridgway/flux-example@0d6a228

Ran it a few times to confirm, but here's the smallest margin of difference that I found:

$ node bench-chat.js
React 558 ops/sec ±5.06%
ReactOpt 698 ops/sec ±1.87%

[sophiebits]

Okay. I'm having trouble getting that running locally – it complains it can't find fluxible-router and I haven't figured out the right incantation of install/link/etc to make it work.

[mridgway]

Sorry, let me pull this out and simplify.

[mridgway]

Ok, you should have better luck with this repo: https://github.com/mridgway/react-perf

Note: the minified builds seem to break if I don't use NODE_ENV=production for some reason.

[sophiebits]

Still failing to require fluxible-router here: https://github.com/mridgway/react-perf/blob/e8e0048d5153b3c2acd834b44ff9fb88c17b0cfd/chat/components/ThreadSection.jsx#L23

[mridgway]

Ugh, so sorry. This is what happens when you globally link something. I pushed changes that removes that dependency.

[sophiebits]

Got it. Not seeing 25%, but definitely some difference – like 10% with NODE_ENV=production:

balpert@balpert-mbp:/t/react-perf master$ node bench-chat.js
React x 348 ops/sec ±3.63% (70 runs sampled)
ReactOpt x 379 ops/sec ±3.52% (70 runs sampled)
Fastest is ReactOpt
balpert@balpert-mbp:/t/react-perf master$ node bench-chat.js
React x 346 ops/sec ±4.07% (71 runs sampled)
ReactOpt x 356 ops/sec ±3.63% (73 runs sampled)
Fastest is ReactOpt,React
balpert@balpert-mbp:/t/react-perf master$ NODE_ENV=production node bench-chat.js
React x 996 ops/sec ±2.73% (83 runs sampled)
ReactOpt x 1,141 ops/sec ±2.62% (80 runs sampled)
Fastest is ReactOpt
balpert@balpert-mbp:/t/react-perf master$ NODE_ENV=production node bench-chat.js
React x 1,002 ops/sec ±3.05% (76 runs sampled)
ReactOpt x 1,096 ops/sec ±2.66% (78 runs sampled)
Fastest is ReactOpt

[syranide]

I think it's important to note that even if you drop _bindAutoBindMethods you'll end up taking the hit through manually calling .bind() instead. It may end up being less, but I don't think you can ever reasonably get rid of the entire cost with the way JS currently is, other than not binding them server-side, but that's not 100% safe.

[mridgway]

The typical use case for needing the autobinding is for event handlers which usually only get fired on the client. It would be more optimal to bind the methods on componentDidMount in that case.

[sophiebits]

Looks like 0.13 started autobinding getDOMNode where we didn't previously, which is almost certainly unintentional on our part. :(

[jordwalke]

Whoa, I noticed this in some of my benchmarks as well but thought it was an outlier. Have you tried with Chrome's high resolution timers? Sometimes frequently called functions get an unfair share of the blame.

[sophiebits]

@jordwalke This isn't with a profiler; this is with normal React and with autobinding commented out.

[mridgway]

With #3801 merged in, would you be open to a PR to migrate internal components to ES6 classes instead of React.createClass to avoid autobind overhead?

[sophiebits]

I think so. Let's do the plain ES3 "class" syntax for now until we figure out whether we want to use loose mode, etc. in babel.

[mridgway]

Closing as internals have removed usage of createClass and userland can avoid it.