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

Performance improvement: ReactDOMServer `resolve` class method #15173

Open

Conversation

Projects
None yet
5 participants
@overlookmotel
Copy link
Contributor

commented Mar 20, 2019

At present the server-side renderer has a hot path through a function called resolve() which contains a lengthy closure processChild().

This PR converts resolve() and processChild() to prototype methods of ReactDOMServerRenderer class, to remove this closure.

Using a fairly naive benchmark, I am seeing the following performance improvements:

  • Node 11: ~4%
  • Node 10: ~4%
  • Node 8: ~4%
  • Node 6: ~15%

If there is an official benchmark I can use to test this on, please let me know.

@sizebot

This comment has been minimized.

Copy link

commented Mar 20, 2019

ReactDOM: size: 0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: c05b4b8...3f0c3b3

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.production.min.js 0.0% 0.0% 105.1 KB 105.1 KB 33.94 KB 33.94 KB UMD_PROD
react-dom.development.js 0.0% -0.0% 791.02 KB 791.02 KB 179.19 KB 179.19 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 105.07 KB 105.07 KB 33.46 KB 33.46 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js 0.0% -0.0% 108.09 KB 108.09 KB 34.63 KB 34.63 KB UMD_PROFILING
ReactFire-profiling.js 0.0% -0.0% 315.82 KB 315.82 KB 58.14 KB 58.14 KB FB_WWW_PROFILING
react-dom-test-utils.production.min.js 0.0% 0.0% 9.73 KB 9.73 KB 3.58 KB 3.59 KB NODE_PROD
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 10.42 KB 10.42 KB 3.57 KB 3.56 KB NODE_PROD
react-dom-server.browser.development.js +0.2% 0.0% 133.58 KB 133.83 KB 35.32 KB 35.34 KB UMD_DEV
react-dom-server.browser.production.min.js 🔺+0.7% 🔺+0.4% 18.93 KB 19.06 KB 7.14 KB 7.17 KB UMD_PROD
react-dom-server.browser.development.js +0.2% 0.0% 129.71 KB 129.97 KB 34.39 KB 34.4 KB NODE_DEV
react-dom-server.browser.production.min.js 🔺+0.7% 🔺+0.3% 18.86 KB 18.99 KB 7.14 KB 7.16 KB NODE_PROD
ReactDOMServer-dev.js +0.2% 0.0% 131.94 KB 132.21 KB 34.08 KB 34.1 KB FB_WWW_DEV
ReactDOMServer-prod.js 🔺+0.9% 🔺+0.1% 45.67 KB 46.06 KB 10.56 KB 10.57 KB FB_WWW_PROD
react-dom-server.node.development.js +0.2% +0.1% 131.65 KB 131.91 KB 34.93 KB 34.95 KB NODE_DEV
react-dom-server.node.production.min.js 🔺+0.7% 🔺+0.4% 19.72 KB 19.85 KB 7.44 KB 7.47 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.66 KB 3.66 KB 1.45 KB 1.45 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.1% 1.21 KB 1.21 KB 706 B 705 B UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.49 KB 3.49 KB 1.41 KB 1.41 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.2% 1.05 KB 1.05 KB 637 B 636 B NODE_PROD
react-dom-unstable-fizz.node.production.min.js 0.0% -0.1% 1.1 KB 1.1 KB 667 B 666 B NODE_PROD

Generated by 🚫 dangerJS

({child, context} = this.resolveNext(element, context, Component));
}

return {child, context};

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 21, 2019

Member

Can we just return the thing returned from resolveNext here? Since we already have it.

@jacobp100

This comment has been minimized.

Copy link

commented Mar 21, 2019

Could this be made simpler by just un-nesting processChild and passing in threadID as an argument? Then we could avoid the prototype lookup and the closure.

@gaearon

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Yeah I'd like to avoid dynamic dispatch if possible.

@overlookmotel

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Is there any official (or more official than mine) benchmark I can run to determine what's fastest?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.