-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Server-side rendering performance degradation with renderToPipeableStream #24232
Comments
@timneutkens, sorry for disturbing, perhaps you have done stress tests on Next.js in combination with |
Could you elaborate on this? |
Yes, my team maintain SSR meta-framework, so we tested HTML streaming couple of times, and have some feedback in general (non-React specific):
|
The perf numbers you mentioned. Are those with the polyfilled Writable that buffers that you mentioned above or with native Node.js streams? It sounds like you’re saying that your Writable polyfill is slow since that’s where the bulk of the time is? Do you want help writing a faster one? Or are you saying that it’s faster with that polyfill than the native Node.js streams? Even if you want to buffer it, do you actually need it to be converted back to a string or can it stay as binary? |
I'm using a native
|
Can you share a code example that includes the native |
Also, tried a quick test of new Next.js app, production version,
But the CPU profile is harder, I can't find obviously slowed down things. Test command example - |
Sorry, do you have some reference sandbox with |
It's copied from here so you should be able to take this one: https://github.com/facebook/react/tree/main/fixtures/ssr2 It uses a local build but you can replace it with 18 in package.json. |
If you run the same benchmark but with your custom HtmlWritable with buffering. What kind of numbers do you get? Similarly, if you run React 18 with Just to narrow down how much of the perf issues are with the writable vs the rest. I don't doubt that native Node.js streams have a lot of overhead in the Web Streams have even more overhead in every chunk call. So we added our own buffering to avoid enqueueing so often. We could do the same for Node streams but there's a principle question there. If we do that, we remove optimization opportunities for user space. There's nothing inherent in the Node stream API that it has to have so much overhead. So should it be our responsibility or the responsibility of the recipient stream? One way could be to build an intermediate stream API, like the one you have, that buffers for a bit and then flushes to the underlying one periodically just like our Web Streams buffering does. |
Thanks! Create few examples from this - https://github.com/SuperOleg39/react-ssr-perf-test
At first, I made comparsion between
Then I tested After removal this middleware, I got around 2 RPS, 300+ ms CPU work for request. Still a big overhead for stream internals:
At last, tried buffering (call it |
It seems to be( |
Ongoing work in #24291. |
The initial set of fixes are in 17
18+legacyBefore
After
18+bufferingBefore
After
18+streamBefore
After
|
Impressive work, thanks! Is there any issues / PR with upcoming related improvements, which I can subscribe for? |
I'm not sure if there are any particular ones planned but @gnoff might be able to provide more details. I think it would make sense to track them in this issue as well. |
Good idea 👍 If we can reach (in perspective) the same perf for streaming render, without this "penalty", it will be bright future for HTML streaming in general :) |
I'm going to continue working on this and will likely have future PRs but can't say exactly when. If you look for titles with |
Nice, thanks you! |
The initial fix is in 18.1.0. |
Hello!
When switching from
renderToString
torenderToPipeableStream
, I run load tests on the application, and found a decrease in server throughput, from 50 to 15 RPS, and an increase in response timings.When profiling the CPU, I see a large overhead on the internal work of the stream, specifically the methods
Writable.write
andWritable.uncork
.All these method calls together take more than twice as much CPU time (about 50-60ms) as rendering my test page (about 15-20ms)
Also, I don't want to give the HTML to the client in the stream, this approach has some disadvantages.
So I have to buffer the data, and it slows down the application a bit more.
CPU profiler in production mode:
CPU profiler in development mode:
My custom Writable stream with buffering:
And rendering flow:
The text was updated successfully, but these errors were encountered: