[Flight] Only skip past the end boundary if there is a newline character #26945

sebmarkbage · 2023-06-14T04:11:56Z

Follow up to #26932

For regular rows, we're increasing the index by one to skip past the last trailing newline character which acts a boundary. For length encoded rows we shouldn't skip an extra byte because it'll leave us missing one.

This only accidentally worked because this was also the end of the current chunk which tests don't account for since we're just passing through the chunks. So I added some noise by splitting and joining the chunks so that this gets tested.

sebmarkbage · 2023-06-14T04:12:43Z

packages/react-client/src/ReactFlightClient.js

      const length = lastIdx - i;
      const lastChunk = new Uint8Array(chunk.buffer, offset, length);
      processFullRow(response, rowID, rowTag, buffer, lastChunk);
      // Reset state machine for a new row
+      i = lastIdx;
+      if (rowState === ROW_CHUNK_BY_NEWLINE) {


Not too happy about this code flow here. Not sure what the most elegant way to restructure this is. I tried to avoid having a helper function.

react-sizebot · 2023-06-14T04:16:28Z

Comparing: a7bf5ba...1f28f5d

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.min.js	=	164.23 kB	164.23 kB	=	51.73 kB	51.73 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	171.67 kB	171.67 kB	=	53.97 kB	53.97 kB
facebook-www/ReactDOM-prod.classic.js	=	570.12 kB	570.12 kB	=	100.58 kB	100.58 kB
facebook-www/ReactDOM-prod.modern.js	=	553.90 kB	553.90 kB	=	97.75 kB	97.75 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/esm/react-server-dom-esm-client.browser.production.min.js	+0.26%	38.53 kB	38.63 kB	+0.40%	9.70 kB	9.74 kB
oss-stable-semver/react-server-dom-esm/esm/react-server-dom-esm-client.browser.production.min.js	+0.26%	38.53 kB	38.63 kB	+0.40%	9.70 kB	9.74 kB
oss-stable/react-server-dom-esm/esm/react-server-dom-esm-client.browser.production.min.js	+0.26%	38.53 kB	38.63 kB	+0.40%	9.70 kB	9.74 kB
oss-experimental/react-client/cjs/react-client-flight.development.js	+0.21%	47.02 kB	47.12 kB	+0.29%	11.87 kB	11.90 kB
oss-stable-semver/react-client/cjs/react-client-flight.development.js	+0.21%	47.02 kB	47.12 kB	+0.29%	11.87 kB	11.90 kB
oss-stable/react-client/cjs/react-client-flight.development.js	+0.21%	47.02 kB	47.12 kB	+0.29%	11.87 kB	11.90 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js	+0.20%	49.69 kB	49.79 kB	+0.28%	12.34 kB	12.38 kB
oss-stable-semver/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js	+0.20%	49.69 kB	49.79 kB	+0.28%	12.34 kB	12.38 kB
oss-stable/react-server-dom-esm/cjs/react-server-dom-esm-client.node.development.js	+0.20%	49.69 kB	49.79 kB	+0.28%	12.34 kB	12.38 kB

Generated by 🚫 dangerJS against 1f28f5d

sebmarkbage · 2023-06-14T04:55:25Z

One thing we could consider is always adding an extra newline even if unnecessary just for readability.

…ter (facebook#26945) Follow up to facebook#26932 For regular rows, we're increasing the index by one to skip past the last trailing newline character which acts a boundary. For length encoded rows we shouldn't skip an extra byte because it'll leave us missing one. This only accidentally worked because this was also the end of the current chunk which tests don't account for since we're just passing through the chunks. So I added some noise by splitting and joining the chunks so that this gets tested.

…ter (#26945) Follow up to #26932 For regular rows, we're increasing the index by one to skip past the last trailing newline character which acts a boundary. For length encoded rows we shouldn't skip an extra byte because it'll leave us missing one. This only accidentally worked because this was also the end of the current chunk which tests don't account for since we're just passing through the chunks. So I added some noise by splitting and joining the chunks so that this gets tested. DiffTrain build for commit a1723e1.

sebmarkbage requested a review from gnoff June 14, 2023 04:11

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 14, 2023

sebmarkbage commented Jun 14, 2023

View reviewed changes

Only skip past the end boundary if there is a newline character

1f28f5d

sebmarkbage force-pushed the fixnewline branch from 55e0b47 to 1f28f5d Compare June 14, 2023 04:20

sebmarkbage merged commit a1723e1 into facebook:main Jun 14, 2023

styfle mentioned this pull request Jun 30, 2023

chore(deps): bump react 18.3.0-canary-1fdacbefd-20230630 vercel/next.js#52005

Merged

styfle mentioned this pull request Jul 5, 2023

chore(deps): bump react to 7118f5dd7 vercel/next.js#52282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flight] Only skip past the end boundary if there is a newline character #26945

[Flight] Only skip past the end boundary if there is a newline character #26945

sebmarkbage commented Jun 14, 2023

sebmarkbage Jun 14, 2023

react-sizebot commented Jun 14, 2023 •

edited

Loading

sebmarkbage commented Jun 14, 2023

[Flight] Only skip past the end boundary if there is a newline character #26945

[Flight] Only skip past the end boundary if there is a newline character #26945

Conversation

sebmarkbage commented Jun 14, 2023

sebmarkbage Jun 14, 2023

Choose a reason for hiding this comment

react-sizebot commented Jun 14, 2023 • edited Loading

Critical size changes

Significant size changes

sebmarkbage commented Jun 14, 2023

react-sizebot commented Jun 14, 2023 •

edited

Loading