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

Use benchmark.js #7

Merged
merged 6 commits into from
Jan 16, 2017
Merged

Use benchmark.js #7

merged 6 commits into from
Jan 16, 2017

Conversation

imsobear
Copy link
Collaborator

  • use benchmark.js replace nodejs child_process
  • code optimization

@imsobear
Copy link
Collaborator Author

imsobear commented Jan 15, 2017

// mock/list length: 50
➜  server-side-rendering-comparison git:(use-benchmark.js) NODE_ENV=production node ./benchmarks/renderToString.js
Rax#renderToString x 678 ops/sec ±3.61% (75 runs sampled)
React#renderToString x 420 ops/sec ±5.36% (81 runs sampled)
Vue#renderToString x 405 ops/sec ±12.81% (57 runs sampled)
Fastest is Rax#renderToString
// mock/list length: 500
➜  server-side-rendering-comparison git:(use-benchmark.js) ✗ NODE_ENV=production node ./benchmarks/renderToString.js
Rax#renderToString x 37.53 ops/sec ±13.83% (38 runs sampled)
React#renderToString x 32.81 ops/sec ±12.13% (42 runs sampled)
Vue#renderToString x 105 ops/sec ±10.56% (54 runs sampled)
Fastest is Vue#renderToString
// mock/list length: 5000
➜  server-side-rendering-comparison git:(use-benchmark.js) ✗ NODE_ENV=production node ./benchmarks/renderToString.js
Rax#renderToString x 6.36 ops/sec ±8.72% (20 runs sampled)
React#renderToString x 4.13 ops/sec ±6.35% (15 runs sampled)
Vue#renderToString x 14.56 ops/sec ±4.88% (68 runs sampled)
Fastest is Vue#renderToString
// mock/list length: 50000
➜  server-side-rendering-comparison git:(use-benchmark.js) ✗ NODE_ENV=production node ./benchmarks/renderToString.js
Rax#renderToString x 0.64 ops/sec ±7.50% (6 runs sampled)
React#renderToString x 0.36 ops/sec ±18.14% (5 runs sampled)
Vue#renderToString x 0.99 ops/sec ±47.18% (11 runs sampled)
Fastest is Vue#renderToString

@imsobear
Copy link
Collaborator Author

cc @yyx990803 @aickin Is this benchmark ok? 😺

@aickin
Copy link

aickin commented Jan 15, 2017

This is an awesome improvement; nice job! I really like how you are using benchmark.js and how you tested with multiple workload sizes.

I'm not sure if you intended this PR to optimize the React code by compiling out process.env.NODE_ENV or not; it seems that the change to client webpack config is attempting to do that. Unfortunately, since none of the test functions or server controllers are run through webpack, that optimization never happens.

So, my feedback is: great job if you are just intending to address some of the benchmark issues! If you also want to fix React prod mode in this PR, it needs some more work, though.

If you just want to merge this to get the benchmark fixes, I'd be happy to rewrite my previous perf PR on top of this PR. Your call.

@yyx990803
Copy link
Contributor

@imsobear - looks great with different data sizes now. Although as @aickin pointed out, his PR (#5) is compiling out the access to process.env.NODE_ENV in server side code as well, because process in Node is not a "simple object", and accessing properties on it is more expensive than it seems. It improves React perf by quite a bit.

@aickin
Copy link

aickin commented Jan 15, 2017

his PR (#5) is compiling out the access to process.env.NODE_ENV in server side code as well

And to be clear, this PR doesn't compile out process.env.NODE_ENV in the renderToString benchmark, either.

const Rax = require('rax');
const raxRenderToString = require('rax-server-renderer').renderToString;
const React = require('react');
const ReactDOMServer = require('react-dom/server');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aickin I don't know how to require ReactDomServer from dist, I try this const ReactDOMServer = require('react-dom/dist/react-dom-server.min');, but get the error:

➜  server-side-rendering-comparison git:(use-benchmark.js) ✗ NODE_ENV=production node benchmarks/renderToString.js
/Users/sobear/work/ssr/server-side-rendering-comparison/node_modules/.15.4.2@react-dom/dist/react-dom.min.js:15
o(n,r);case"topSelectionChange":if(f)break;case"topKeyDown":case"topKeyUp":return o(n,r)}return null},didPutListener:function(e,t,n){"onSelect"===t&&(_=!0)}};t.exports=C},{111:111,123:123,132:132,141:141,19:19,33:33,56:56,80:80}],75:[function(e,t,n){"use strict";function r(e){return"."+e._rootNodeID}function o(e){return"button"===e||"input"===e||"select"===e||"textarea"===e}var i=e(113),a=e(122),s=e(19),u=e(33),l=e(76),c=e(77),p=e(80),d=e(81),f=e(83),h=e(84),m=e(79),v=e(85),g=e(86),y=e(87),_=e(88),C=e(129),b=e(99),E=(e(137),{}),x={};["abort","animationEnd","animationIteration","animationStart","blur","canPlay","canPlayThrough","click","contextMenu","copy","cut","doubleClick","drag","dragEnd","dragEnter","dragExit","dragLeave","dragOver","dragStart","drop","durationChange","emptied","encrypted","ended","error","focus","input","invalid","keyDown","keyPress","keyUp","load","loadedData","loadedMetadat

TypeError: Cannot read property 'ReactCurrentOwner' of undefined
    at Object.r.120 (/Users/sobear/work/ssr/server-side-rendering-comparison/node_modules/.15.4.2@react-dom/dist/react-dom.min.js:15:20480)
    at o (/Users/sobear/work/ssr/server-side-rendering-comparison/node_modules/.15.4.2@react-dom/dist/react-dom.min.js:12:624)
    at /Users/sobear/work/ssr/server-side-rendering-comparison/node_modules/.15.4.2@react-dom/dist/react-dom.min.js:12:675
    at Object.r.61.113 (/Users/sobear/work/ssr/server-side-rendering-comparison/node_modules/.15.4.2@react-dom/dist/react-dom.min.js:14:14876)
    at o (/Users/sobear/work/ssr/server-side-rendering-comparison/node_modules/.15.4.2@react-dom/dist/react-dom.min.js:12:624)
    at /Users/sobear/work/ssr/server-side-rendering-comparison/node_modules/.15.4.2@react-dom/dist/react-dom.min.js:12:675
    at Object.r.31.10 (/Users/sobear/work/ssr/server-side-rendering-comparison/node_modules/.15.4.2@react-dom/dist/react-dom.min.js:13:14983)
    at o (/Users/sobear/work/ssr/server-side-rendering-comparison/node_modules/.15.4.2@react-dom/dist/react-dom.min.js:12:624)
    at /Users/sobear/work/ssr/server-side-rendering-comparison/node_modules/.15.4.2@react-dom/dist/react-dom.min.js:12:675
    at Object.r.47.1 (/Users/sobear/work/ssr/server-side-rendering-comparison/node_modules/.15.4.2@react-dom/dist/react-dom.min.js:14:4823)

Maybe you can help me, thank you.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, requiring react-dom/server from the dist directory was never really supported, and it no longer works.

As I said in my comment, if you merge this PR as is, I will happily make a PR that does the optimizations on top of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will merge this PR fisrt. Thank you.

@imsobear
Copy link
Collaborator Author

I will try use react-dom/dist to compiling out the access to process.env.NODE_ENV. But I think this way looks strange and a little ugly for developer. I dare not promise to use this way in my project lol.

@imsobear imsobear merged commit 5e6e9c6 into master Jan 16, 2017
@imsobear imsobear deleted the use-benchmark.js branch January 16, 2017 04:14
@developit
Copy link

This PR made me so happy @imsobear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants