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

Proposal: ReactDOMServer render to Stream #6420

Closed
aickin opened this issue Apr 6, 2016 · 33 comments
Closed

Proposal: ReactDOMServer render to Stream #6420

aickin opened this issue Apr 6, 2016 · 33 comments

Comments

@aickin
Copy link
Contributor

aickin commented Apr 6, 2016

tl;dr: I'd like to know how much enthusiasm there is on the core React team for accepting a PR with the ability to render markup to a stream. I don't need a guarantee, of course, but I'd like to get an sense if it's something the React team might do before spending a ton of time working on it.

Background

Currently, server rendering is accomplished by ReactDOMServer.renderToString or ReactDOMServer.renderToStaticMarkup, which are synchronous functions that return a string.

The server render methods can be somewhat slow. Large (~500K) web pages can easily take over 100 or 200ms to render, depending on the server. Also, since node is single threaded and single-core performance has not been improving rapidly, this is not just a problem that Moore's Law will eventually solve. The slowness in turn causes two problems in production websites with large (or somewhat large) HTML pages:

  1. First, because the methods return a string, they completely render the entire component before you can return even the first byte to the browser, meaning that the browser is waiting idle while the render method is called.
  2. Second, because the methods are synchronous and node is single threaded, the server is unresponsive during rendering, which means that a large page render can easily starve out renders of smaller pages (which may only take a few ms). An asynchronous render method would let large page renders yield CPU to small pages, improving average response time.

Proposed Solution

The proposal is to add two methods to ReactDOMServer, both of which return a node Readable Stream.

Stream renderToStream(ReactElement element)
Stream renderToStaticMarkupStream(ReactElement element)

In terms of behavior, these methods would return a Readable Stream that outputs the same markup as renderToString and renderToStaticMarkup, respectively.

These methods could solve both problems above. A stream can begin emitting content immediately to send to the browser, keeping TTFB constant as components get larger. Streams are also inherently asynchronous, which allows large pages to not starve out smaller ones.

A few months back I forked react into a library called react-dom-stream to do this exact thing, and it's been fairly well received (1,100+ stars on GitHub, though modest actual downloads on npm). There's also a wishlist bug thread on this repo about it. However, maintaining a fork is a less than desirable position, and I wonder if I can get it integrated in to the core code base.

My implementation in react-dom-stream is not ideal; I ended up copying basically every implementation of mountComponent and making the copied versions asynchronous. The benefit of this approach was that it didn't affect renderToString performance, but the obvious drawback was that any code changes or bug fixes would have to be performed in both forks of the code, which is not ideal.

Recently, I figured out a way to share code in a better way between renderToString and renderToStream, but the result was that renderToString slowed down relatively significantly (+30%, plus or minus). I'm trying to figure out how if I can tune it more carefully, but it's not looking great yet.

Questions

So I think the questions I'd like to ask to the team are:

  1. Assuming it were perfectly implemented with no performance degradation of the current code, is this even something you are willing to accept a PR for this feature?
  2. If you were to accept this, how important to you is it that the string and stream versions of the code follow the same code path?
  3. Are you willing to accept any degradation in performance of the string version of server rendering in order to get stream rendering that has constant TTFB and keep a single code path?

Thanks for your time, and thanks for all the great work you do on react!

(Tagging @spicyj, @gaearon, and @sebmarkbage, as I was instructed to do on the react IRC channel.)

@sophiebits
Copy link
Collaborator

Thanks for following up and filing this.

I haven't looked at your code in detail, but I am generally positive on this. It sounds like serious users of server rendering (which include none of us on the core team) would all appreciate this feature. We should get feedback from some of those people before settling on an API.

I would prefer to have parallel codepaths rather than degrade the performance of renderToString. It shouldn't require serious changes: I believe that React[Server]ReconcileTransaction and ReactDOMComponent are really the only modules that have knowledge of the server-rendering difference, so there shouldn't be much to split. In particular, none of our "isomorphic" modules know about server rendering because it is a DOM-specific concept (ex: React Native does not support server rendering).

@mridgway
Copy link
Contributor

mridgway commented Apr 6, 2016

At Yahoo, we do a significant amount of server rendering with React and are extremely interested in getting streaming support in the core. As such, I am more than willing to help out in any way possible.

I have to done some investigation into render streaming and also looked at your implementation of react-dom-stream. I agree with your assessment that as it stands it is difficult to support streaming without modifying a lot of the code in React*Component's mountComponent method. This is because the recursion of children is implemented within this method and you would have to have checks for the current transaction in each of the mountComponent methods for returning a string vs. writing to a stream.

As such, I think the best course of action (and recommendation by @sebmarkbage) was to move the recursion out of the individual React*Component.mountComponent methods and into the ReactReconciler.mountComponent method instead. This method would then receive arrays of metadata: strings and child components that could be written to the transaction or recursed. The transaction would then take care of concatenating strings or writing to a stream via a common interface.

I did some initial work on hoisting the recursion in #6011. Centrallizing the recursion as in this PR would make it much easier to support streaming and asynchrony in the future.

It is difficult to commit to working on this further without buy in from the core team and more information on what is involved in #6170.

@aickin
Copy link
Contributor Author

aickin commented Apr 6, 2016

@spicyj:

I am generally positive on this. It sounds like serious users of server rendering (which include none of us on the core team) would all appreciate this feature.

So great to hear, thanks!

We should get feedback from some of those people before settling on an API.

Agreed. Other than this issue, and outreach I will do to react-dom-stream users, are there any other methods to get feedback you would suggest?

I would prefer to have parallel codepaths rather than degrade the performance of renderToString.

Good feedback, thanks.

I believe that React[Server]ReconcileTransaction and ReactDOMComponent are really the only modules that have knowledge of the server-rendering difference, so there shouldn't be much to split.

I think it's a bit more widespread than the code that has knowledge of server-rendering; the issue is that the mountComponent and mountChildren signature (everywhere it shows up, both in DOM-specific and non-DOM-specific code) is synchronous, which won't work with streams. In my current implementation, I wrote a parallel set of mountComponentAsync/mountChildrenAsync methods which have an async function signature.

I'm currently working on a newer version where mountComponent instead returns a data structure that can be processed either synchronously or asynchronously, depending on the caller. The preliminary (pretty messy) code for it is here if you are interested (ignore the test code; most of the interesting stuff is in React***Component and StringLazyTree files).

@mridgway:

also looked at your implementation of react-dom-stream

Eek. The implementation is... not my proudest achievement; I had to rewrite it from scratch about 4 or 5 times (callbacks, generators, async/await) because perf was often abysmal once I got it working.

I think the best course of action (and recommendation by @sebmarkbage) was to move the recursion out of the individual React*Component.mountComponent methods and into the ReactReconciler.mountComponent method instead.

I think I did something like this in my most recent experimental WIP branch (as I said above, most of the interesting stuff is in React***Component files and StringLazyTree). Rather than hoisting the recursion into ReactReconciler, though, I pulled it out into a structure I called StringLazyTree (after DomLazyTree), but the rest of the idea is basically the same. The mountComponent/mountChildren methods now return a StringLazyTree, which is basically an array of raw strings and closures to call as you recurse down the tree. That can be done synchronously or asynchronously, which preserves the same code path for renderToString and renderToStream.

This experimental branch passes nearly all the tests (it doesn't do checksums yet, but I know how to do that from previous work), but it's about 50-100% slower for last byte than renderToString in my tests. Working on that now, although I'm not making much progress.

It is difficult to commit to working on this further without buy in from the core team and more information on what is involved in #6170.

Thanks for pointing that out; I didn't know about it at all. Is there any info on what the algorithm rework looks like?

@goatslacker
Copy link
Contributor

Tentative plan:

  1. Get rid of checksum checking on the client and checksum generation on the server. This has the benefit of loosening up the guidelines for how markup is decided to be valid or not on the client when re-rendering. The new approach would walk the DOM tree and verify that the tree is the same, attributes and their values are the same, and content is the same.
  2. Get rid of data-reactid. We can keep track of DOM -> internal tree while we walk the DOM on client render.
  3. Split out client rendering and server rendering. Now we can have server renderers that exist outside of React and can be independent.

Does this sound right? Did I miss something?

Other notes:

  • We need to ensure that if we do have multiple server renderers that live outside of React that React is aware of their existence and runs their tests. I'd like to know about breaking changes ahead of time.
  • Perhaps we should provide a documented spec of what is expected from a server renderer. What lifecycles should we call? What the DOM markup should be like? etc...

@jimfb @aickin and others who'd like to comment.

@sophiebits
Copy link
Collaborator

The new approach would walk the DOM tree and verify that the tree is the same, attributes and their values are the same, and content is the same.

This would probably be very slow and would likely require a bunch of internal refactors to even make possible.

Get rid of data-reactid.

We barely need this at all now but I kept it intentionally so we can distinguish React-rendered nodes from nodes from elsewhere (from browser extensions, for instance).

@jimfb
Copy link
Contributor

jimfb commented Apr 21, 2016

We barely need this at all now but I kept it intentionally so we can distinguish React-rendered nodes from nodes from elsewhere (from browser extensions, for instance).

Nodes from elsewhere are going to disturb a react render anyway, and thus get blown away when React mounted into the dom. I don't think there is much value in knowing where the nodes came from (knowing that the nodes differ is sufficient), but there is a disadvantage to being forced to maintain the data-reactid because it mandates a specific render order.

@aickin
Copy link
Contributor Author

aickin commented Apr 21, 2016

Does this sound right? Did I miss something?

That's what I remember, yes.

This would probably be very slow and would likely require a bunch of internal refactors to even make possible.

We talked about this a while, and we weren't entirely sure how slow it would be, but I agree it's the biggest risk in the idea. @jimfb argued that it would be at least same big(O) of what is done currently when rendering on top of server markup, but the question is how much worse reading dom tag names and attributes is than constructing a giant string. I'm worried that it might be MUCH worse, but we will see.

@goatslacker
Copy link
Contributor

likely require a bunch of internal refactors

This, and slow, is what worries me mostly. Any pointers on where to start looking?

Also helpful would be knowing where the logic that checks if it's React.createClass, React.Component, or SFC is at.

@aickin
Copy link
Contributor Author

aickin commented Apr 21, 2016

@goatslacker also, thanks for writing this up! 👍

FYI, I'm trying to hack my way through a rough cut of step 1 today. I'm in the middle of it right now, and it's definitely a beast. I've got a few dozen new unit tests passing but a bunch not.

@jimfb
Copy link
Contributor

jimfb commented Apr 21, 2016

@goatslacker

Also helpful would be knowing where the logic that checks if it's React.createClass, React.Component, or SFC is at.

src/renderers/shared/reconciler/ReactCompositeComponent.js (eg. #5884)

@lelandrichardson
Copy link
Contributor

I think an important piece of this is building a rough spec + test suite that can be run against external renderers that is also run against react core to ensure that breakages are properly communicated before being published. This should cover:

  1. lifecycles that are expected to get called during render
  2. that react on the client can properly bootstrap itself on top of nodes with the proper structure (accentuating the differences in character-by-character html output that are possible)
  3. obviously covering a lot of the corner-case HTML oddities
  4. (am i missing anything???)

This could be done similarly to ECMA's test262 repo, and breakages + additions could be communicated inside of the github repo that holds these tests.

This is really exciting!

@aickin
Copy link
Contributor Author

aickin commented Apr 22, 2016

Quick update on potential speed of reconnecting to DOM by walking the dom rather than checksum (step 1).

I have a branch that does this now, although it's by no means complete (needs work on interactivity, a bunch more unit tests, 3 existing unit tests fail, and the code is not as readable as I'd like). It is complete enough to do a really rough comparison of performance between the two strategies, though, which I started this morning.

My very preliminary and very rough observation is that DOM walking is about 20% slower than checksum in Safari/OSX and about 40% slower in Chrome/OSX.

This is obviously not ideal (faster is better!), but it's also not an order of magnitude increase. It's also not a giant amount of absolute time: in my (once again, rough) test on a 635KB render, the difference amounted to 20-40ms. When I tested a 107KB render, I couldn't detect a difference between the two result with ad-hoc testing. It's also worth noting that I haven't done any perf optimization on the branch.

I'm still concerned about the potential for bad perf regression, particularly given that different browsers can have wildly different perf characteristics. I think the best path forward is to polish up my test, make it automatable via Selenium, and run it enough times to get statistical significance on a variety of platforms. I think good coverage would look something like:

  • Multiple document sizes
    • ~25K
    • ~100K
    • ~500K
  • Platforms
    • Chrome/Windows 10
    • Firefox/Windows 10
    • OS X latest/Safari
    • iOS 9.2/Safari
    • Android 4.4/Default Browser
    • IE11/Windows 10
    • IE10/Windows 8
    • IE9/Windows 7

Does that make sense? Do folks agree?

Also: How much perf regression on client reconnect to server markup would you accept, if any?

@lencioni
Copy link
Contributor

Great work! Getting better numbers makes a lot of sense to me.

How much perf regression on client reconnect to server markup would you accept, if any?

I think the answer might depend, at least partly, on how much faster we think we can make server rendering with the optimizations we discussed.

@aickin
Copy link
Contributor Author

aickin commented Apr 22, 2016

I think the answer might depend, at least partly, on how much faster we think we can make server rendering with the optimizations we discussed.

Agreed. Even if connecting server markup on the client is slower, it's conceivable that the perf gains from speeding up server rendering and reducing page weight by removing data-reactid could make up for it in practical use. However, it's a hard for me to know how to trade these off without empirical data about the servers and networks that this will be used on.

@jimfb
Copy link
Contributor

jimfb commented Apr 22, 2016

@aickin Great work! This is amazing!

Obviously we do care about performance in all browsers, but if you have perf metrics for a couple of browsers (eg. Chrome+Firefox), my intuition is that it's sufficient to understand the likely ramifications of your changes. Obviously feel free to collect more data if you think it's desirable/impactful, but your effort might be better spent optimizing and fixing bugs. Your call. We can always go collect more perf metrics later if such metrics are needed to make a decision.

Personally, I think 20-40% is fine. Especially since there are so many benefits to this approach. We can give better error messages. We can remove remove all the complicated data-id lookup logic. We can remove the dataids from the markup, making markup smaller. We can have a faster/optimized/dedicated SSR renderer (I predict at least 2-5x improvements there anyway, which will annihilate the additional 20-40% remount cost). Etc. However, that said, it's ultimately a team/community decision. And we should still do everything we can to make it even faster and get better numbers!

I think I would also be curious to hear how the performance compares with an initial pass that was rendered without serverside markup (ie. the createElement path).

@sebmarkbage
Copy link
Collaborator

I think it will be difficult to get this off the ground if you try to make it better in phase 1. Just getting to a point where the code is decoupled while still maintainable in a way is going to be hard enough.

As a first goal, I'd suggest we get to a point where it is just feature/implementation parity with what we have to day.

@aickin
Copy link
Contributor Author

aickin commented Apr 22, 2016

As a first goal, I'd suggest we get to a point where it is just feature/implementation parity with what we have to day.

Sage advice. I wanted to know that it wasn't going to be like 5x worse to walk the DOM before investing a ton more time in this strategy, and I feel pretty confident that's the case now (Selenium-izing the tests shows between 5-70% worse perf, depending on platform). Back to working on correctness!

@aickin
Copy link
Contributor Author

aickin commented Apr 26, 2016

I just opened PR #6618, which attempts to tackle the correctness side of stage 1. Please feel free to comment there. Thanks!

@mridgway
Copy link
Contributor

mridgway commented May 2, 2016

I've been thinking about walking the DOM to do checksum validation and if it's more beneficial to create a virtual DOM based off of the existing DOM instead of trying to do validation. This way we would be able to do a patch to update the server-rendered DOM to be equivalent to the client-rendered markup instead of doing a wholesale replacement. If we're already traversing the DOM for validation is it any slower to create a virtual DOM instead?

This is similar to what incremental-dom proposes, but in React's case it would only be doing this for the initial instantiation over top of existing DOM.

@jzlxiaohei
Copy link

render to stream and mountComponentAsync is very important for cache in server. otherwise , i have to cache component in memory.

@carlosbaraza
Copy link

@aickin @goatslacker @spicyj @sebmarkbage @lencioni do you have an update on this?

It is a feature we are very interested on, and we would like to move it forward. Is there anything we can do to help with the feature?

@gaearon
Copy link
Collaborator

gaearon commented Nov 14, 2016

We are currently working on Fiber which is a complete reimplementation of React core.
This means it doesn't seem to make sense to spend effort in trying to shoehorn streaming into the existing reconciler, as it will be replaced.

Fiber should make it easier to create a separate streaming server renderer that would be focused on performance. This won't be our initial focus but as Fiber becomes more stable I'm sure there will be a lot more space for such community contributions.

@mufumbo
Copy link

mufumbo commented Nov 19, 2016

thanks a lot for the update @gaearon

we had to migrate to markojs because react isn't feasible for isomorphic, but will definitely check Fiber out when it comes out.

@markudevelop
Copy link

too bad we don't have this

@isaachinman
Copy link

So is it correct to assume there is currently no way to have a server render to stream in React v15.x?

@wincent
Copy link
Contributor

wincent commented May 11, 2017

@isaachinman: There is https://github.com/aickin/react-dom-stream and https://github.com/FormidableLabs/rapscallion — but I haven't tried them personally.

@sophiebits
Copy link
Collaborator

React 16 will likely include a streaming renderer that @tomocchino has been working on. We don't plan to add one to React 15.

@wincent
Copy link
Contributor

wincent commented May 11, 2017

@spicyj: Neat. I didn't know @tomocchino was hacking.

@gaearon
Copy link
Collaborator

gaearon commented Jul 12, 2017

It’s landed.

@gaearon gaearon closed this as completed Jul 12, 2017
@hitmands
Copy link

please, provide documentation.

@gaearon
Copy link
Collaborator

gaearon commented Jul 28, 2017

It’s in beta. We add documentation when something becomes stable and part of the official release. The 16 release has not happened yet.

@aickin
Copy link
Contributor Author

aickin commented Jul 28, 2017

@hitmands While documentation isn't 100% stable or published to the web, it is checked in to master here.

@gaearon
Copy link
Collaborator

gaearon commented Aug 4, 2017

Btw that documentation is currently inaccurate. The APIs are exported directly from react-dom/server now:

import { renderToStream } from 'react-dom/server';
// or
import { renderToStaticStream } from 'react-dom/server';

Available since 16 beta 3.

#10294 (comment)

We'll fix the docs before the release.

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

No branches or pull requests