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

[Suspense] Change Suspending and Restarting Heuristics #15769

Merged
merged 13 commits into from
May 30, 2019

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented May 30, 2019

This introduces a new principle for when we restart a render instead of continuing to completion. The principle is that if we're going to suspend when we complete a root, then we should also restart if we get an update or ping that might unsuspend it, and vice versa. The only reason to suspend is because you think you might want to restart before committing. However, it doesn't make sense to restart only while in the period we're suspended. That's just a race condition depending on how long it took to render. We should also be able to restart before that.

However, restarting too aggressively is also not good because it starves out any intermediate loading state. So this PR uses a set of heuristics.

Heuristics

If nothing threw a Promise or all the same fallbacks are already showing, then don't suspend/restart.

If this is an initial render of a new tree of Suspense boundaries and those trigger a fallback, then don't suspend/restart. We want to ensure that we can show the initial loading state as quickly as possible.

If we hit a "Delayed" case, such as when we'd switch from content back into a fallback, then we should always suspend/restart. SuspenseConfig applies to this case. If none is defined, JND is used instead.

If we're already showing a fallback and it gets "retried", allowing us to show another level, but there's still an inner boundary that would show a fallback, then we suspend/restart for 500ms since the last time we showed a fallback anywhere in the tree. This effectively throttles progressive loading into a consistent train of commits. This also gives us an opportunity to restart to get to the completed state slightly earlier.

If there's ambiguity due to batching it's resolved in preference of: "delayed", "initial render", "retry". We want to ensure that a "busy" state doesn't get force committed. We want to ensure that new initial loading states can commit as soon as possible.

More info about the mechanisms in the individual commits.

Tests

This PR needed to adjust a lot of tests.

Most of them is because initial render no longer suspends in the traditional way. I fixed this mostly by first rendering an empty Suspense boundary and then updating it. This transition works mostly the traditional way.

Another quirk is that restarting doesn't happen until we know that this render is going to suspend. Currently, we only know that once we complete the Suspense boundary. Pings can happen before or after this. If we never yield after completing it, we don't restart until we get to the root. I left a comment to fix that. In the meantime I had to rewrite the tests to continue render past the Suspense boundary before pausing so that it can restart after that.

@sizebot
Copy link

sizebot commented May 30, 2019

ReactDOM: size: 0.0%, gzip: -0.1%

Details of bundled changes.

Comparing: 3b23022...2e1c5ea

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js +0.1% +0.1% 108.78 KB 108.92 KB 34.54 KB 34.59 KB NODE_PROFILING
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.76 KB 60.76 KB 15.85 KB 15.84 KB UMD_DEV
ReactDOM-dev.js +0.8% +1.0% 872.9 KB 879.95 KB 194.31 KB 196.31 KB FB_WWW_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.1% 10.69 KB 10.69 KB 3.67 KB 3.66 KB UMD_PROD
ReactDOMServer-dev.js 0.0% -0.0% 135.44 KB 135.44 KB 34.79 KB 34.79 KB FB_WWW_DEV
react-dom-unstable-fire.development.js +0.8% +1.0% 853.22 KB 860.41 KB 194.39 KB 196.41 KB UMD_DEV
react-dom-unstable-fire.production.min.js 🔺+0.1% 🔺+0.2% 105.46 KB 105.6 KB 34.28 KB 34.35 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js +0.1% +0.1% 108.61 KB 108.75 KB 35.3 KB 35.35 KB UMD_PROFILING
react-dom-unstable-fire.development.js +0.8% +1.0% 847.54 KB 854.72 KB 192.82 KB 194.84 KB NODE_DEV
react-dom-server.node.development.js 0.0% -0.0% 135.18 KB 135.18 KB 35.77 KB 35.77 KB NODE_DEV
react-dom-unstable-fire.production.min.js 🔺+0.1% 🔺+0.2% 105.46 KB 105.6 KB 33.68 KB 33.75 KB NODE_PROD
react-dom-server.node.production.min.js 0.0% -0.0% 19.98 KB 19.98 KB 7.53 KB 7.53 KB NODE_PROD
react-dom.development.js +0.8% +1.0% 852.87 KB 860.06 KB 194.25 KB 196.27 KB UMD_DEV
react-dom-unstable-fire.profiling.min.js +0.1% +0.1% 108.79 KB 108.93 KB 34.55 KB 34.6 KB NODE_PROFILING
react-dom-server.browser.development.js 0.0% -0.0% 137.11 KB 137.11 KB 36.16 KB 36.16 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.2% 105.45 KB 105.59 KB 34.27 KB 34.34 KB UMD_PROD
ReactFire-dev.js +0.8% +1.0% 872.11 KB 879.16 KB 194.27 KB 196.26 KB FB_WWW_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 19.2 KB 19.2 KB 7.23 KB 7.23 KB UMD_PROD
react-dom.profiling.min.js +0.1% +0.1% 108.6 KB 108.74 KB 35.29 KB 35.34 KB UMD_PROFILING
ReactFire-prod.js 🔺+0.3% 🔺+0.3% 342.98 KB 343.88 KB 63.26 KB 63.46 KB FB_WWW_PROD
react-dom.development.js +0.8% +1.0% 847.19 KB 854.38 KB 192.68 KB 194.7 KB NODE_DEV
ReactFire-profiling.js +0.3% +0.2% 348.18 KB 349.16 KB 64.19 KB 64.35 KB FB_WWW_PROFILING
react-dom.production.min.js 🔺+0.1% 🔺+0.2% 105.44 KB 105.59 KB 33.67 KB 33.74 KB NODE_PROD
react-dom-server.browser.production.min.js 0.0% -0.0% 19.12 KB 19.12 KB 7.22 KB 7.22 KB NODE_PROD
ReactDOM-prod.js 🔺+0.3% 🔺+0.4% 355.04 KB 355.95 KB 65.7 KB 65.93 KB FB_WWW_PROD
ReactDOMServer-prod.js 0.0% -0.0% 47.93 KB 47.93 KB 11.04 KB 11.03 KB FB_WWW_PROD
ReactDOM-profiling.js +0.3% +0.3% 360.23 KB 361.21 KB 66.64 KB 66.81 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.43 KB 60.43 KB 15.72 KB 15.72 KB NODE_DEV
react-dom-unstable-fizz.node.development.js 0.0% -0.1% 3.88 KB 3.88 KB 1.51 KB 1.51 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.1% 10.43 KB 10.43 KB 3.57 KB 3.56 KB NODE_PROD
react-dom-unstable-fizz.node.production.min.js 0.0% -0.3% 1.1 KB 1.1 KB 668 B 666 B NODE_PROD
react-dom-test-utils.development.js 0.0% -0.0% 57.47 KB 57.47 KB 15.78 KB 15.78 KB UMD_DEV
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.81 KB 3.81 KB 1.54 KB 1.54 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% -0.1% 10.81 KB 10.81 KB 3.98 KB 3.97 KB UMD_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.3% 1.21 KB 1.21 KB 707 B 705 B UMD_PROD
react-dom-test-utils.development.js 0.0% -0.0% 55.81 KB 55.81 KB 15.46 KB 15.46 KB NODE_DEV
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.64 KB 3.64 KB 1.49 KB 1.49 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% -0.0% 10.56 KB 10.56 KB 3.92 KB 3.92 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% -0.2% 1.05 KB 1.05 KB 637 B 636 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-dev.js +1.3% +1.8% 527.52 KB 534.56 KB 109.98 KB 112 KB FB_WWW_DEV
react-art.development.js +1.2% +1.6% 586.06 KB 593.25 KB 128.18 KB 130.18 KB UMD_DEV
react-art.production.min.js 🔺+0.1% 🔺+0.2% 96.9 KB 97.04 KB 29.83 KB 29.88 KB UMD_PROD
react-art.development.js +1.4% +1.8% 516.97 KB 524.16 KB 110.69 KB 112.7 KB NODE_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.3% 61.88 KB 62.02 KB 19.11 KB 19.16 KB NODE_PROD
ReactART-prod.js 🔺+0.4% 🔺+0.4% 201.69 KB 202.59 KB 34.35 KB 34.49 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +1.4% +1.8% 530.77 KB 537.95 KB 113.62 KB 115.65 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.2% 🔺+0.3% 63.22 KB 63.36 KB 19.53 KB 19.59 KB UMD_PROD
react-test-renderer.development.js +1.4% +1.8% 526.31 KB 533.49 KB 112.5 KB 114.52 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.2% 🔺+0.4% 62.91 KB 63.05 KB 19.32 KB 19.39 KB NODE_PROD
ReactTestRenderer-dev.js +1.3% +1.8% 538.94 KB 545.97 KB 112.54 KB 114.56 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% -0.0% 41.84 KB 41.84 KB 10.82 KB 10.82 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.1% 11.6 KB 11.6 KB 3.55 KB 3.55 KB UMD_PROD
react-test-renderer-shallow.development.js 0.0% -0.0% 35.98 KB 35.98 KB 9.45 KB 9.45 KB NODE_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.1% 11.74 KB 11.74 KB 3.67 KB 3.67 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFabric-dev.js +1.1% +1.4% 650.37 KB 657.39 KB 137.9 KB 139.85 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.4% 🔺+0.4% 244.02 KB 245.04 KB 42.2 KB 42.39 KB RN_FB_PROD
ReactNativeRenderer-dev.js +1.1% +1.4% 661.5 KB 668.53 KB 140.49 KB 142.47 KB RN_OSS_DEV
ReactFabric-profiling.js +0.4% +0.3% 252.1 KB 253.2 KB 43.87 KB 44.01 KB RN_FB_PROFILING
ReactNativeRenderer-prod.js 🔺+0.4% 🔺+0.4% 251.23 KB 252.26 KB 43.46 KB 43.61 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.4% +0.4% 259.29 KB 260.39 KB 45.1 KB 45.27 KB RN_OSS_PROFILING
ReactNativeRenderer-dev.js +1.1% +1.4% 661.59 KB 668.61 KB 140.52 KB 142.5 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.4% 🔺+0.4% 251.21 KB 252.24 KB 43.46 KB 43.61 KB RN_FB_PROD
ReactNativeRenderer-profiling.js +0.4% +0.4% 259.27 KB 260.37 KB 45.1 KB 45.26 KB RN_FB_PROFILING
ReactFabric-dev.js +1.1% +1.4% 650.28 KB 657.3 KB 137.87 KB 139.82 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.4% 🔺+0.4% 244.02 KB 245.04 KB 42.2 KB 42.39 KB RN_OSS_PROD
ReactFabric-profiling.js +0.4% +0.3% 252.12 KB 253.21 KB 43.87 KB 44.01 KB RN_OSS_PROFILING

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-reflection.development.js 0.0% -0.0% 19.05 KB 19.05 KB 6.08 KB 6.08 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% -0.2% 2.51 KB 2.51 KB 1.12 KB 1.11 KB NODE_PROD
react-reconciler-persistent.development.js +1.4% +1.9% 512.25 KB 519.43 KB 108.17 KB 110.19 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.3% 🔺+0.2% 63.01 KB 63.2 KB 18.95 KB 18.98 KB NODE_PROD
react-reconciler.development.js +1.4% +1.8% 514.57 KB 521.76 KB 109.18 KB 111.19 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.3% 🔺+0.2% 63 KB 63.19 KB 18.94 KB 18.98 KB NODE_PROD

Generated by 🚫 dangerJS

This value is going to be used to avoid committing too many fallback
states in quick succession. It doesn't really matter where in the tree
that happened.

This means that we now don't really need the concept of SuspenseState
other than has a flag. It could be made cheaper/simpler.
This now eagerly commits non-delayed suspended trees, unless they're
only retries in which case they're throttled to 500ms.
If we get a ping on this level but have not yet suspended, we might
still suspend later. In that case we should still restart.
We should add this to throwException so we get these markers earlier.
I've had to rewrite tests that test restarting to account for the delayed
restarting heuristic.

Ideally, we should also be able to restart from within throwException if
we're already ready to restart. Right now we wait until the next yield.
They're not testing the exact states of the suspense boundaries, only
the result. I keep assertions that they're not already resolved early.
I also added a blank initial render to ensuree that we cover the suspended
case.
Mostly this just means render the Suspense boundary first so that it
becomes an update instead of initial mount.
@gaearon
Copy link
Collaborator

gaearon commented May 30, 2019

The principle is that if we're going to suspend when we complete a root, then we should also restart if we get an update or ping that might unsuspend it, and vice versa

Which meaning of the word “suspend” do you use here? “Not flush placeholder immediately”?

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

New heuristics make sense, though maybe you could paste this PR's description somewhere in ReactFiberWorkLoop? The inline comments throughout are very helpful but individually they paint an incomplete picture. A person who lacks context probably won't know where to look for them.

Meta note, I like that we're able to change all these heuristics but none of them affect the terminal output of a sequence of events. Feels like we've got most of the right levers and knobs in place, and now we're focused on fiddling with their settings.

@acdlite
Copy link
Collaborator

acdlite commented May 30, 2019

@gaearon Yeah that's what "suspend" means in this context: commit the fallbacks after a setTimeout instead of immediately.

@sebmarkbage sebmarkbage merged commit 113497c into facebook:master May 30, 2019
@gaearon
Copy link
Collaborator

gaearon commented May 31, 2019

This summary is still very dense and a bit hard to understand for me. Let me try to rephrase it and you'll correct me where I misunderstood?

The principle is that if we're going to suspend when we complete a root, then we should also restart if we get an update or ping that might unsuspend it, and vice versa. The only reason to suspend is because you think you might want to restart before committing.

If we've decided not to commit the fallback yet and wait more, we should also handle updates and retry. This is because that might avoid needing to commit fallback altogether — or unlock a level deeper when it has to commit.

Indeed a chance of unlocking more is the whole point of waiting. Otherwise we'd be wasting time and delaying a loading state that we already have.

However, it doesn't make sense to restart only while in the period we're suspended. That's just a race condition depending on how long it took to render.

We shouldn't couple restarting opportunity too closely to whether we're suspended or not. Otherwise the result is too unpredictable because we might miss the window.

We should also be able to restart before that.

Not sure what this is about. Before what?

However, restarting too aggressively is also not good because it starves out any intermediate loading state.

There might be too many new updates that might unlock new levels. If we always restart, we'll never manage to complete. We gotta stop somewhere.

If nothing threw a Promise or all the same fallbacks are already showing, then don't suspend/restart.

I guess this is about the case where an update led to no "suspensey" changes?

If this is an initial render of a new tree of Suspense boundaries and those trigger a fallback, then don't suspend/restart. We want to ensure that we can show the initial loading state as quickly as possible.

This is about top-level render? I think it's saying that there's no JND on first render because the thing is blank. So we fill it in right away and commit fallbacks if that's all we have.

But maybe this isn't just about top-level. Is it about first time Suspense is in a tree? Are we saying we want to flush the initial fallback early — as long as nothing else prevents us from doing it (such as an intention to delay or maybe only having a "shy" fallback available)? I'm not sure.

If we hit a "Delayed" case, such as when we'd switch from content back into a fallback, then we should always suspend/restart. SuspenseConfig applies to this case. If none is defined, JND is used instead.

If committing a state change would lead to a new fallback (that wasn't shown before) being committed, we wait for JND (or longer if opted in via config). While we're waiting, more updates would lead to retries since that's our only chance to actually show more by the time we have to commit.

If we're already showing a fallback and it gets "retried", allowing us to show another level, but there's still an inner boundary that would show a fallback, then we suspend/restart for 500ms since the last time we showed a fallback anywhere in the tree. This effectively throttles progressive loading into a consistent train of commits.

If unlocking a level reveals another fallback, we make sure they don't flash too often. It extends past normal JND because the end result is not ideal (?)

This also gives us an opportunity to restart to get to the completed state slightly earlier.

I don't get this. How does throttling let us get to completed state faster? Or is it just "always restarting" in this mode that let us complete sooner?

If there's ambiguity due to batching it's resolved in preference of: "delayed", "initial render", "retry". We want to ensure that a "busy" state doesn't get force committed. We want to ensure that new initial loading states can commit as soon as possible.

If one of batched updates causes a "navigation" transition, don't worry about showing new loading states ASAP yet. But if there's no "navigations" then we should paint the intentional loading states early. In absence of both of these special cases, get on the throttling unlocking train. (How does that relate to JND again?)

Copy link

@nobodysfool nobodysfool left a comment

Choose a reason for hiding this comment

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

README.md

acdlite added a commit to acdlite/react that referenced this pull request Jul 10, 2020
Now that Suspense retries have their own dedicated set of lanes
(facebook#19287), we can determine if a render includes only retries by checking
if its lanes are a subset of the retry lanes.

Previously we inferred this by checking
`workInProgressRootLatestProcessedEventTime`. If it's not set, that
implies that no updates were processed in the current render, which
implies it must be a Suspense retry. The eventual plan is to get rid of
`workInProgressRootLatestProcessedEventTime` and instead track event
times on the root; this change is one the steps toward that goal.

The relevant tests were originally added in facebook#15769.
acdlite added a commit that referenced this pull request Jul 10, 2020
Now that Suspense retries have their own dedicated set of lanes
(#19287), we can determine if a render includes only retries by checking
if its lanes are a subset of the retry lanes.

Previously we inferred this by checking
`workInProgressRootLatestProcessedEventTime`. If it's not set, that
implies that no updates were processed in the current render, which
implies it must be a Suspense retry. The eventual plan is to get rid of
`workInProgressRootLatestProcessedEventTime` and instead track event
times on the root; this change is one the steps toward that goal.

The relevant tests were originally added in #15769.
@gaearon gaearon mentioned this pull request Mar 29, 2022
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.

6 participants