-
Notifications
You must be signed in to change notification settings - Fork 31
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
Compile out all calls to process.env.NODE_ENV #5
Conversation
… calls to process.env.NODE_ENV
…fail. Didn't see it because I hadn't cleared out my build folder. Sorry.
Oops, I had a silly bug with the Vue server benchmark that I fixed in ec66722. The bug caused an error in the vue renderer, which made vue run very fast, and therefore biased the test towards Vue. The change means that unlike my initial belief, vue is the slowest in the qps test, not the fastest (although, as I noted above, I have NO IDEA how to optimize Vue, so take that with a grain of salt). I've updated the numerical results above with a recent run of the benchmark after applying ec66722. Sorry for the mixup. Also, in case you care about IP issues, I declare that I am the sole author of the code changes in this PR, and I offer them irrevocably to the public domain. Use as you wish. |
You can also fix this process.env = Object.assign({}, process.env); You will lose the ability to listen to env changes, but I doubt many people are using that. |
@STRML Yeah, that will work for most cases, but as you note, it could break some code. I prefer to compile the references out because it's more provably correct, but in practice it's probably the same 99.9% of the time. |
@aickin I tested applying the same compilation to the Vue case but didn't see significant difference. On the other hand, I found out that increasing the number of items displayed in For example changing the list length to 30 instead of 5:
And increasing the list count to 100:
What I can tell from these numbers:
Ultimate I think the numbers will depend heavily on the actual content being rendered and the specific optimization strategies used (e.g. with full streaming mode + component caching in Vue). |
@yyx990803 Thanks so much for taking time to play with this! That all mostly makes sense to me, although I'm a little confused about streaming mode being faster than non-streaming. IME streaming tends to add a slight overhead, but I know absolutely nothing about Vue and its SSR, so I'm probably just uninformed. FWIW, I don't have a high level of trust in the current results of this test because of the reasons I outlined in #6, but it looks like those are getting fixed in #7. (I mean, I still suspect Vue will come out on top, especially for larger workloads, but I'm reserving judgment until I see the numbers 😄). I'm excited to see the results once those issues have been addressed! (Side note: Vue has server side component caching??? That's SO cool. Component caching was a feature I prototyped in (ETA: To be clear, I'm not trying to take credit for Vue's component caching at all; you implemented it and made it work and should be proud of it! I'm merely interested in how ideas travel and if it was a case of interchange of ideas or "great minds think alike".) |
@aickin re streaming: that's what I thought as well, but when I run Vue's own ssr benchmark streaming mode always finishes faster (note this is intentionally benching a big workload). Streaming should also result in better end-user perceived performance because the browser will be able to start rendering sooner in a streaming fashion. The streaming implementation is indeed heavily inspired by |
Closed temporarily. ref to #10. Thank you ~ |
While PR #3 was a good step towards following production best practices, it only went part way. Implementing prod mode in React rendering on the server has two parts:
NODE_ENV=production
(which was accomplished in Use latest versions of Rax/React/Vue, set NODE_ENV=production #3).process.env.NODE_ENV
altogether.This is because
process.env
is not a simple JavaScript object, and it is surprisingly expensive to use it. Even when running in production mode, just the calls to check theprocces.env.NODE_ENV
can easily take 30% of rendering time in React.In the current master code, this benchmark doesn't compile
react-dom/server
at all, so its perf is sub-optimal.In this PR, I've implemented this change for both the
renderToString
benchmarks and the server benchmarks. To keep things apples to apples, I tried to compile outprocess.env.NODE_ENV
for Rax and Vue, too. However, in Vue's case, I couldn't do it because Vue's renderer is not compatible with webpack. So the Vue code I left basically the same as it was, while I compile Rax and React to removeprocess.env.NODE_ENV
. If anyone has expertise in Vue and can advise about perf optimization for it, I'd be grateful.The qps benchmark seems to bounce around a lot from run to run (which I will file as a separate issue), but here's the data from a relatively representative run on my machine after these changes:
Environment: Retina MBP mid-2014, 2.8GHz i7, 16GB RAM, node 6.9.1
Thanks for all your work, and I'm excited to play around with Rax!