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] Expose maxBoundarySize as an option #21029

Merged
merged 3 commits into from
Mar 19, 2021

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Mar 18, 2021

Bike shed now or forever hold your peace.

@sebmarkbage sebmarkbage requested a review from gaearon March 18, 2021 03:30
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 18, 2021
@sizebot
Copy link

sizebot commented Mar 18, 2021

Comparing: 154b852...0fa77e1

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 = 122.89 kB 122.89 kB = 39.55 kB 39.55 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 129.38 kB 129.38 kB = 41.61 kB 41.61 kB
facebook-www/ReactDOM-prod.classic.js = 409.01 kB 409.01 kB = 75.85 kB 75.85 kB
facebook-www/ReactDOM-prod.modern.js = 397.27 kB 397.27 kB = 73.91 kB 73.91 kB
facebook-www/ReactDOMForked-prod.classic.js = 409.02 kB 409.02 kB = 75.85 kB 75.85 kB
oss-experimental/react-server/cjs/react-server.development.js +4.10% 32.38 kB 33.70 kB +7.29% 8.33 kB 8.94 kB
oss-stable/react-server/cjs/react-server.development.js +4.10% 32.38 kB 33.70 kB +7.29% 8.33 kB 8.94 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.development.js +2.94% 47.24 kB 48.63 kB +5.22% 11.92 kB 12.54 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.development.js +2.93% 46.98 kB 48.36 kB +5.14% 11.91 kB 12.53 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.development.js +2.85% 49.47 kB 50.88 kB +5.08% 12.08 kB 12.69 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/cjs/react-server.development.js +4.10% 32.38 kB 33.70 kB +7.29% 8.33 kB 8.94 kB
oss-stable/react-server/cjs/react-server.development.js +4.10% 32.38 kB 33.70 kB +7.29% 8.33 kB 8.94 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.development.js +2.94% 47.24 kB 48.63 kB +5.22% 11.92 kB 12.54 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.development.js +2.93% 46.98 kB 48.36 kB +5.14% 11.91 kB 12.53 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.development.js +2.85% 49.47 kB 50.88 kB +5.08% 12.08 kB 12.69 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-server.production.min.js +1.36% 2.50 kB 2.53 kB +1.95% 0.98 kB 1.00 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-server.production.min.js +1.36% 2.50 kB 2.53 kB +1.95% 0.98 kB 1.00 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.production.min.js +1.29% 10.35 kB 10.48 kB +1.19% 3.88 kB 3.92 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.production.min.js +1.27% 10.54 kB 10.68 kB +1.16% 3.96 kB 4.01 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-server.development.js +1.18% 5.17 kB 5.24 kB +1.89% 1.48 kB 1.51 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-server.development.js +1.18% 5.17 kB 5.24 kB +1.89% 1.48 kB 1.51 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.production.min.js +0.98% 10.58 kB 10.68 kB +1.05% 3.92 kB 3.96 kB
oss-experimental/react-server/cjs/react-server.production.min.js +0.85% 8.36 kB 8.43 kB +0.96% 3.03 kB 3.06 kB
oss-stable/react-server/cjs/react-server.production.min.js +0.85% 8.36 kB 8.43 kB +0.96% 3.03 kB 3.06 kB

Generated by 🚫 dangerJS against 0fa77e1

@sebmarkbage sebmarkbage changed the title [Fizz] Expose maxBoundarySize as an option 1e8ad31 [Fizz] Expose maxBoundarySize as an option Mar 18, 2021
@gaearon
Copy link
Collaborator

gaearon commented Mar 18, 2021

This applies to content only, right? E.g. if fallback is longer we still have to emit it. So the size is not really of the boundary itself but its primary content. Is that right?

@gaearon
Copy link
Collaborator

gaearon commented Mar 18, 2021

How about maxContentLength?

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Mar 18, 2021

The implementation is not really correct now because it's shallow. So if you include many small boundaries, it will never break it up. My original heuristic measured the parent instead but had to abandon that for some reason. I think the real heuristic needs to be an accumulation that includes previous siblings. So that it writes until the next boundary would exceed the threshold. Similarly a SuspenseList should stop at some point even if it only has small boundaries for each item.

So it's really about the parent boundary's max size at that point. It won't be exactly capped below that number but roughly.

I suspect this option might change implementation details given the various ways it's possible to do this so I wouldn't over focus on the exact details. The number indicates roughly how much HTML you can transfer, layout and render in a meaningful window (likely 500ms since that's what the train model uses). If it's smaller than that you'd show the result quicker than the train model would allow. So it really is mostly for giant documents like long Wikipedia entries or something.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

I feel like it's hard to name until we know more precisely how we end up using it.

@sebmarkbage
Copy link
Collaborator Author

I added a qualified guess for the initial default value to try. We'll need to get more data on this from non-FB sources.

This is a default heuristic for how to split up the HTML content into progressive
loading. Our goal is to be able to display additional new content about every 500ms.
Faster than that is unnecessary and should be throttled on the client. It also
adds unnecessary overhead to do more splits. We don't know if it's a higher or lower
end device but higher end suffer less from the overhead than lower end does from
not getting small enough pieces. We error on the side of low end.
We base this on low end 3G speeds which is about 500kbits per second. We assume
that there can be a reasonable drop off from max bandwidth which leaves you with
as little as 80%. We can receive half of that each 500ms - at best. In practice,
a little bandwidth is lost to processing and contention - e.g. CSS and images that
are downloaded along with the main content. So we estimate about half of that to be
the lower end throughput. In other words, we expect that you can at least show
about 12.5kb of content per 500ms. Not counting starting latency for the first
paint.

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Mar 19, 2021

RFC: maxBoundarySize -> progressiveChunkSize

@gaearon
Copy link
Collaborator

gaearon commented Mar 19, 2021

Like

@sebmarkbage sebmarkbage merged commit f6bc9c8 into facebook:master Mar 19, 2021
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Mar 23, 2021
* Expose maxBoundarySize as an option

* Adjust the heuristic

* Rename to progressiveChunkSize
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 23, 2021
Summary:
This sync includes the following changes:
- **[6d3ecb70d](facebook/react@6d3ecb70d )**: Remove unstable_changedBits ([#20953](facebook/react#20953)) //<Andrew Clark>//
- **[754e30728](facebook/react@754e30728 )**: Delete immediateQueueCallbackNode ([#20980](facebook/react#20980)) //<Andrew Clark>//
- **[be5a2e231](facebook/react@be5a2e231 )**: Land enableSyncMicrotasks ([#20979](facebook/react#20979)) //<Andrew Clark>//
- **[f6bc9c824](facebook/react@f6bc9c824 )**: [Fizz] Expose maxBoundarySize as an option ([#21029](facebook/react#21029)) //<Sebastian Markbåge>//
- **[154b85213](facebook/react@154b85213 )**: [Fizz] Expose a method to explicitly start writing to a Node stream ([#21028](facebook/react#21028)) //<Sebastian Markbåge>//
- **[cf485e6f6](facebook/react@cf485e6f6 )**: [Fizz] Expose a method to abort a pending request ([#21027](facebook/react#21027)) //<Sebastian Markbåge>//
- **[3fb11eed9](facebook/react@3fb11eed9 )**: React Native: Touch Instrumentation Interface ([#21024](facebook/react#21024)) //<Timothy Yung>//
- **[825c3021f](facebook/react@825c3021f )**: Don't delete trailing mismatches during hydration at the root ([#21021](facebook/react#21021)) //<Sebastian Markbåge>//
- **[1d1e49cfa](facebook/react@1d1e49cfa )**: [Fizz] Assign an ID to the first DOM element in a fallback or insert a dummy (and testing infra) ([#21020](facebook/react#21020)) //<Sebastian Markbåge>//
- **[466b26c92](facebook/react@466b26c92 )**: Store commit durations on HostRoot for DevTools access ([#20983](facebook/react#20983)) //<Brian Vaughn>//
- **[89acfa639](facebook/react@89acfa639 )**: Fix native event batching in concurrent mode ([#21010](facebook/react#21010)) //<Ricky>//
- **[0203b6567](facebook/react@0203b6567 )**: chore: update react-reconciler README ([#21016](facebook/react#21016)) //<susiwen8>//

Changelog:
[General][Changed] - React Native sync for revisions c9f6d0a...6d3ecb7

jest_e2e[run_all_tests]

Reviewed By: JoshuaGross, kacieb

Differential Revision: D27231625

fbshipit-source-id: 89c0c0662e69044ae8890486a693013bee6005dd
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
* Expose maxBoundarySize as an option

* Adjust the heuristic

* Rename to progressiveChunkSize
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