-
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
[Fizz] Pipeable Stream Perf #24291
[Fizz] Pipeable Stream Perf #24291
Conversation
Comparing: ec52a56...4fd4f93 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
packages/react-server-native-relay/src/__tests__/ReactFlightNativeRelay-test.internal.js
Outdated
Show resolved
Hide resolved
This comment was scratch for capturing some performance timings (code that does not show up in any of the commits currently in this PR). I'll leave it here for clarity's sake but it should be disregarded.
|
Yeah. I'll add it to the list for follow-ups. For now you can inspect the output HTML to see what they do. |
abe95fc
to
39c193d
Compare
… renderToPipeableStream Modified from ssr2 and https://github.com/SuperOleg39/react-ssr-perf-test
The previous implementation of pipeable streaming (Node) suffered some performance issues brought about by the high chunk counts and innefficiencies with how node streams handle this situation. In particular the use of cork/uncork was meant to alleviate this but these methods do not do anything unless the receiving Writable Stream implements _writev which many won't. This change adopts the view based buffering techniques previously implemented for the Browser execution context. The main difference is the use of backpressure provided by the writable stream which is not implementable in the other context. Another change to note is the use of standards constructs like TextEncoder and TypedArrays.
encodeInto allows us to write directly to the view buffer that will end up getting streamed instead of encoding into an intermediate buffer and then copying that data.
What do pw and fcq mean? |
I think I'll delete that comment. it was performance.now timings for If you look in the main description you will see more broad based network results from autocannon which is more real-world |
@@ -437,6 +437,7 @@ const bundles = [ | |||
'ReactFlightNativeRelayServerIntegration', | |||
'JSResourceReferenceImpl', | |||
'ReactNativeInternalFeatureFlags', | |||
'util', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebmarkbage you commented that this should not be necessary. In practice the build fails without this so I assume ReactServerStreamConfigNode.js
is getting pulled in by accident or something changed and now that is the expected file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The React sync for React Native is currently failing because "util could not be found", I confirmed #24322 fixes it, how can we get it landed?
* Add fixture for comparing baseline render perf for renderToString and renderToPipeableStream Modified from ssr2 and https://github.com/SuperOleg39/react-ssr-perf-test * Implement buffering in pipeable streams The previous implementation of pipeable streaming (Node) suffered some performance issues brought about by the high chunk counts and innefficiencies with how node streams handle this situation. In particular the use of cork/uncork was meant to alleviate this but these methods do not do anything unless the receiving Writable Stream implements _writev which many won't. This change adopts the view based buffering techniques previously implemented for the Browser execution context. The main difference is the use of backpressure provided by the writable stream which is not implementable in the other context. Another change to note is the use of standards constructs like TextEncoder and TypedArrays. * Implement encodeInto during flushCompletedQueues encodeInto allows us to write directly to the view buffer that will end up getting streamed instead of encoding into an intermediate buffer and then copying that data.
* Add fixture for comparing baseline render perf for renderToString and renderToPipeableStream Modified from ssr2 and https://github.com/SuperOleg39/react-ssr-perf-test * Implement buffering in pipeable streams The previous implementation of pipeable streaming (Node) suffered some performance issues brought about by the high chunk counts and innefficiencies with how node streams handle this situation. In particular the use of cork/uncork was meant to alleviate this but these methods do not do anything unless the receiving Writable Stream implements _writev which many won't. This change adopts the view based buffering techniques previously implemented for the Browser execution context. The main difference is the use of backpressure provided by the writable stream which is not implementable in the other context. Another change to note is the use of standards constructs like TextEncoder and TypedArrays. * Implement encodeInto during flushCompletedQueues encodeInto allows us to write directly to the view buffer that will end up getting streamed instead of encoding into an intermediate buffer and then copying that data.
* Add fixture for comparing baseline render perf for renderToString and renderToPipeableStream Modified from ssr2 and https://github.com/SuperOleg39/react-ssr-perf-test * Implement buffering in pipeable streams The previous implementation of pipeable streaming (Node) suffered some performance issues brought about by the high chunk counts and innefficiencies with how node streams handle this situation. In particular the use of cork/uncork was meant to alleviate this but these methods do not do anything unless the receiving Writable Stream implements _writev which many won't. This change adopts the view based buffering techniques previously implemented for the Browser execution context. The main difference is the use of backpressure provided by the writable stream which is not implementable in the other context. Another change to note is the use of standards constructs like TextEncoder and TypedArrays. * Implement encodeInto during flushCompletedQueues encodeInto allows us to write directly to the view buffer that will end up getting streamed instead of encoding into an intermediate buffer and then copying that data.
drawing heavily from #24034 this PR should improve performance of renderToPipeableStream.
This PR implements a TypedArray buffer that text is encoded into while writing chunks to the stream. It also adds a fixture which is useful for comparing the baseline rendering performance of renderToPipeableStream and renderToString.
If you want to test out the current implementation simply visit the first commit.
The second commit includes an analog of the buffer which does encoding during the performWork step.
The third commit includes a change to adopt encodeInto which delivers some performance gains by avoiding extra mem copies.
Fixture Performance
The following stats were gathered with
npx autocannon -c 5 -d 30 http://localhost:4000/{stream|string} --renderStatusCodes --excludeErrorStats
Before Change
buffering stream chunks
encodeInto