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] Only skip past the end boundary if there is a newline character #26945

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

sebmarkbage
Copy link
Collaborator

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 sebmarkbage requested a review from gnoff June 14, 2023 04:11
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 14, 2023
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) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

react-sizebot commented Jun 14, 2023

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
Copy link
Collaborator Author

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

EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…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.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…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.
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.

3 participants