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

[Fizz] Optimize end tags chunks #27522

Merged
merged 2 commits into from
Oct 20, 2023
Merged

Conversation

yujunjung
Copy link
Contributor

@yujunjung yujunjung commented Oct 15, 2023

Summary

By caching '</' + tag + '>' Chunk, my hacker news ssr benchmarks showed an improvement of over 10% in requests/second

Benchmark: https://github.com/yujunjung/optimize-end-tag-fizz
Requests/second increased from 815 to 910

How did you test this change?

@react-sizebot
Copy link

react-sizebot commented Oct 15, 2023

Comparing: 67cc9ba...d9ca72a

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 = 174.68 kB 174.68 kB = 54.32 kB 54.31 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 176.77 kB 176.77 kB = 54.99 kB 54.99 kB
facebook-www/ReactDOM-prod.classic.js = 565.55 kB 565.55 kB = 99.53 kB 99.53 kB
facebook-www/ReactDOM-prod.modern.js = 549.40 kB 549.40 kB = 96.60 kB 96.60 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against d9ca72a

@acdlite
Copy link
Collaborator

acdlite commented Oct 15, 2023

Interesting benchmark. I wonder if we should have a hardcoded switch statement for the most common tags: div, p, span, etc. And then fall back to a map. Could you try measuring that?

@@ -2509,7 +2509,7 @@ function pushStyleImpl(
target.push(stringToChunk(escapeTextForBrowser('' + child)));
}
pushInnerHTML(target, innerHTML, children);
target.push(endTag1, stringToChunk('style'), endTag2);
target.push(endChunkForTag('style'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

might as well precompute these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not well, it is already cached so the computation that causes the memory allocation is only executed once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yujunjung
Copy link
Contributor Author

yujunjung commented Oct 16, 2023

Interesting benchmark. I wonder if we should have a hardcoded switch statement for the most common tags: div, p, span, etc. And then fall back to a map. Could you try measuring that?

I experimented with that method but didn't get any performance improvement. Map lookup is fast. Reducing redundant TextEncoder.encode() and String concat that causes memory allocation is very important for performance.

@acdlite
Copy link
Collaborator

acdlite commented Oct 17, 2023

I think this looks good to merge but I'll let @sebmarkbage or @gnoff decide

@acdlite acdlite requested review from sebmarkbage and gnoff October 17, 2023 16:58
Copy link
Collaborator

@gnoff gnoff left a comment

Choose a reason for hiding this comment

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

I wonder if it'd be impactful to just statically reference precomputed chunks for start and end tags for every function that pushes with an explicit type. So no new switch but everywhere we can avoid the map lookup we do and we only use the map for the generic path where the type is dynamic. Either way let's get this merged. LMK if you experiment with this other idea

@gnoff gnoff merged commit 8c85b02 into facebook:main Oct 20, 2023
2 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 20, 2023
Implements `endChunkForTag` to make writing end tags faster

DiffTrain build for [8c85b02](8c85b02)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
Implements `endChunkForTag` to make writing end tags faster
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
Implements `endChunkForTag` to make writing end tags faster

DiffTrain build for commit 8c85b02.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants