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

[Flight Reply] Encode binary streams as a single collapsed Blob #28986

Merged
merged 1 commit into from
May 8, 2024

Conversation

sebmarkbage
Copy link
Collaborator

Based on #28893.

For other streams we encode each chunk as a separate form field which is a bit bloated. Especially for binary chunks since they also have an indirection. We need some way to encode the chunks as separate anyway. This way the streaming using busboy actually allows each chunk to stream in over the network one at a time.

For binary streams the actual chunking is not important. The chunks can be split and recombined in whatever size chunk makes sense.

Since we buffer the entire content anyway we can combine the chunks to be consecutive. This PR does that with binary streams and also combine them into a single Blob. That way there's no extra overhead when passing through a binary stream.

Ideally, we'd be able to just use the stream from that one Blob but Node.js doesn't return byob streams from Blob. Additionally, we don't actually stream the content of Blobs due to the layering with busboy atm. We could do that for binary streams in particular by replacing the File layering with a stream and resolving each chunk as it comes in. That could be a follow up.

If we stop buffering in the future, this set up still allows us to split them and send other form fields in between while blocked since the protocol is still the same.

@sebmarkbage sebmarkbage requested a review from gnoff May 3, 2024 18:43
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels May 3, 2024
@react-sizebot
Copy link

react-sizebot commented May 3, 2024

Comparing: ec9400d...f6b75aa

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB +0.05% 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 492.61 kB 492.61 kB = 87.88 kB 87.88 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.11% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 498.86 kB 498.86 kB = 88.92 kB 88.93 kB
facebook-www/ReactDOM-prod.classic.js = 591.22 kB 591.22 kB = 103.96 kB 103.96 kB
facebook-www/ReactDOM-prod.modern.js = 567.44 kB 567.44 kB = 100.36 kB 100.36 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.production.js +1.77% 42.72 kB 43.47 kB +0.70% 8.86 kB 8.92 kB
oss-experimental/react-client/cjs/react-client-flight.production.js +1.76% 45.39 kB 46.19 kB +0.66% 8.80 kB 8.86 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.browser.production.js +1.70% 44.26 kB 45.01 kB +0.65% 9.23 kB 9.29 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.js +1.69% 44.59 kB 45.35 kB +0.64% 9.31 kB 9.37 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.node.production.js +1.60% 47.10 kB 47.86 kB +0.66% 9.89 kB 9.96 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.unbundled.production.js +1.56% 48.35 kB 49.11 kB +0.71% 10.17 kB 10.24 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.production.js +1.56% 48.35 kB 49.11 kB +0.73% 10.17 kB 10.25 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.js +1.54% 49.03 kB 49.79 kB +0.73% 10.34 kB 10.41 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.production.js +1.54% 49.04 kB 49.80 kB +0.73% 10.32 kB 10.40 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.production.js +1.51% 50.07 kB 50.82 kB +0.65% 10.54 kB 10.61 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.edge.production.js +1.51% 50.08 kB 50.83 kB +0.67% 10.52 kB 10.59 kB
oss-experimental/react-server-dom-esm/esm/react-server-dom-esm-client.browser.production.js +1.23% 69.55 kB 70.40 kB = 15.05 kB 15.01 kB
oss-experimental/react-client/cjs/react-client-flight.development.js +0.99% 86.34 kB 87.19 kB = 19.53 kB 19.50 kB
oss-experimental/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js +0.99% 86.35 kB 87.20 kB = 19.24 kB 19.20 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js +0.98% 86.59 kB 87.44 kB = 19.31 kB 19.27 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.browser.development.js +0.95% 89.84 kB 90.69 kB = 20.27 kB 20.24 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.development.js +0.94% 90.35 kB 91.20 kB = 20.44 kB 20.40 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js +0.90% 94.26 kB 95.12 kB = 21.34 kB 21.30 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.unbundled.development.js +0.89% 96.05 kB 96.90 kB = 21.91 kB 21.87 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.development.js +0.89% 96.08 kB 96.93 kB = 21.93 kB 21.90 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.node.development.js +0.87% 97.48 kB 98.33 kB = 22.28 kB 22.24 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.development.js +0.87% 97.51 kB 98.36 kB = 22.32 kB 22.28 kB
oss-experimental/react-server-dom-turbopack/cjs/react-server-dom-turbopack-client.edge.development.js +0.86% 98.55 kB 99.40 kB = 22.48 kB 22.44 kB
oss-experimental/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.development.js +0.86% 98.58 kB 99.43 kB = 22.52 kB 22.49 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Generated by 🚫 dangerJS against f6b75aa

Since we buffer anyway, we might as well combine the chunks of binary
streams.

Ideally, we'd be able to just use the stream from the Blob but Node.js
doesn't return byob streams from Blob. Additionally, we don't actually
stream the content of Blobs due to the layering with busboy atm. We could
do that for binary streams in particular by replacing the File layering
with a stream.
@sebmarkbage sebmarkbage merged commit 826bf4e into facebook:main May 8, 2024
38 checks passed
github-actions bot pushed a commit that referenced this pull request May 8, 2024
Based on #28893.

For other streams we encode each chunk as a separate form field which is
a bit bloated. Especially for binary chunks since they also have an
indirection. We need some way to encode the chunks as separate anyway.
This way the streaming using busboy actually allows each chunk to stream
in over the network one at a time.

For binary streams the actual chunking is not important. The chunks can
be split and recombined in whatever size chunk makes sense.

Since we buffer the entire content anyway we can combine the chunks to
be consecutive. This PR does that with binary streams and also combine
them into a single Blob. That way there's no extra overhead when passing
through a binary stream.

Ideally, we'd be able to just use the stream from that one Blob but
Node.js doesn't return byob streams from Blob. Additionally, we don't
actually stream the content of Blobs due to the layering with busboy
atm. We could do that for binary streams in particular by replacing the
File layering with a stream and resolving each chunk as it comes in.
That could be a follow up.

If we stop buffering in the future, this set up still allows us to split
them and send other form fields in between while blocked since the
protocol is still the same.

DiffTrain build for [826bf4e](826bf4e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants