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

Leverage renderToNodeStream #220

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kevin940726
Copy link

@kevin940726 kevin940726 commented Oct 1, 2019

Continued from #218, we decided to go with renderToNodeStream first since there might be client side features in the future.

The following benchmark is run on my macbook pro (2018, 2.7GHz i7, macOS 10.14.6) on page unpkg.com/browse/[email protected]/lib/tsc.js (Network throttle Fast 4G) under production mode.

renderToStaticMarkup + renderToString renderToNodeStream + moving __DATA__ to body
image image

You can see that the TTFB is lower (3.94s to 1.91s) than the current one in a fairly heavy page.

It's not a huge deal though, since it only matters in the first request, requests after that should be cached either by browser or by cloudflare. However, we can reduce some overhead (CPU, memory) for our server when lots of files are being requested for the first time in a short interval.

@mjackson
Copy link
Owner

mjackson commented Oct 1, 2019

Thanks, @kevin940726! Build is failing for some reason. I'll take a look.

@kevin940726
Copy link
Author

Looks like the build has been failing in master for quite some time 😅

@kevin940726
Copy link
Author

kevin940726 commented Oct 2, 2019

I also moved the __DATA__ script for hydration to the end of the body (above other scripts). So that the html content could flush to the client even sooner.

After adding the above change, the TTFB is getting lower but not significantly. (from 2.23s down to 1.89s)

@mjackson mjackson force-pushed the master branch 4 times, most recently from fe1de4c to e8ccce1 Compare October 7, 2019 04:51
@kevin940726
Copy link
Author

@mjackson Looks like those secrets cannot be exposed in forked PR in CI for security reasons. Might have to skip those tests which use the secrets to be able to run it in PR 🤔.

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.

2 participants