Force unwind work loop during selective hydration #25695
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 7b17f7b...e86bb50 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
sebmarkbage
reviewed
Nov 16, 2022
| // work loop state. But using an exception means we don't need to | ||
| // check for this case on every iteration of the work loop. So doing | ||
| // it this way moves the check out of the fast path. | ||
| throw SelectiveHydrationException; | ||
| } else { |
There was a problem hiding this comment.
Is this branch just a bug in React? Not sure how it happens exactly.
There was a problem hiding this comment.
Same answer as here: cd34ec3#r1024411227
There was a problem hiding this comment.
It happens when the hydration lane is suspended. That indicates we already attempted to hydrate but we couldn't because the code/data isn't ready. In that case we switch back to the original update lane and render without hydrating. Without this branch it would keep restarting in a loop until the data resolved.
sebmarkbage
reviewed
Nov 16, 2022
| // we yield. TODO: We could probably just force yielding earlier instead. | ||
| // we yield. | ||
| // TODO: This should only happen for sync updates, which don't have a | ||
| // hydration lane. We should add one. Then, consider removing this call. | ||
| renderDidSuspendDelayIfPossible(); |
There was a problem hiding this comment.
Is this call just unnecessary now? This flag doesn't do anything in sync mode anyway?
sebmarkbage
reviewed
Nov 16, 2022
| @@ -2808,7 +2826,9 @@ function updateDehydratedSuspenseComponent( | |||
|
|
|||
| // If we have scheduled higher pri work above, this will just abort the render | |||
| // since we now have higher priority work. We'll try to infinitely suspend until | |||
There was a problem hiding this comment.
This comment doesn't really make sense if this only happens for sync.
There was a problem hiding this comment.
Yeah the explanation is wrong. It's because if hydration suspends, we go back to the original update lane and switch to a client render. Then we suspend with delay.
sebmarkbage
approved these changes
Nov 16, 2022
Merged
When an update flows into a dehydrated boundary, React cannot apply the update until the boundary has finished hydrating. The way this currently works is by scheduling a slightly higher priority task on the boundary, using a special lane that's reserved only for this purpose. Because the task is slightly higher priority, on the next turn of the work loop, the Scheduler will force the work loop to yield (i.e. shouldYield starts returning `true` because there's a higher priority task). The downside of this approach is that it only works when time slicing is enabled. It doesn't work for synchronous updates, because the synchronous work loop does not consult the Scheduler on each iteration. We plan to add support for selective hydration during synchronous updates, too, so we need to model this some other way. I've added a special internal exception that can be thrown to force the work loop to interrupt the work-in-progress tree. Because it's thrown from a React-only execution stack, throwing isn't strictly necessary — we could instead modify some internal work loop state. But using an exception means we don't need to check for this case on every iteration of the work loop. So doing it this way moves the check out of the fast path. The ideal implementation wouldn't need to unwind the stack at all — we should be able to hydrate the subtree and then apply the update all within a single render phase. This is how we intend to implement it in the future, but this requires a refactor to how we handle "stack" variables, which are currently pushed to a per-render array. We need to make this stack resumable, like how context works in Flight and Fizz.
acdlite
force-pushed
the
force-unwind-selective-hydration
branch
from
cd34ec3 to
e86bb50
Compare
tyao1
added a commit
that referenced
this pull request
Nov 17, 2022
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn debug-test --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> For more context: #25692 Based on #25695. This PR adds the `SyncHydrationLane` so we rewind on sync updates during selective hydration. Also added tests for ContinuouseHydration and DefaultHydration lanes. ## How did you test this change? <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. --> yarn test
rickhanlonii
pushed a commit
that referenced
this pull request
Dec 3, 2022
When an update flows into a dehydrated boundary, React cannot apply the update until the boundary has finished hydrating. The way this currently works is by scheduling a slightly higher priority task on the boundary, using a special lane that's reserved only for this purpose. Because the task is slightly higher priority, on the next turn of the work loop, the Scheduler will force the work loop to yield (i.e. shouldYield starts returning `true` because there's a higher priority task). The downside of this approach is that it only works when time slicing is enabled. It doesn't work for synchronous updates, because the synchronous work loop does not consult the Scheduler on each iteration. We plan to add support for selective hydration during synchronous updates, too, so we need to model this some other way. I've added a special internal exception that can be thrown to force the work loop to interrupt the work-in-progress tree. Because it's thrown from a React-only execution stack, throwing isn't strictly necessary — we could instead modify some internal work loop state. But using an exception means we don't need to check for this case on every iteration of the work loop. So doing it this way moves the check out of the fast path. The ideal implementation wouldn't need to unwind the stack at all — we should be able to hydrate the subtree and then apply the update all within a single render phase. This is how we intend to implement it in the future, but this requires a refactor to how we handle "stack" variables, which are currently pushed to a per-render array. We need to make this stack resumable, like how context works in Flight and Fizz.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Dec 5, 2022
…5695)" We're reverting the stack of changes that this code belongs to in order to unblock the sync to Meta's internal codebase. We will attempt to re-land once the sync is unblocked.
tyao1
pushed a commit
that referenced
this pull request
Dec 7, 2022
When an update flows into a dehydrated boundary, React cannot apply the update until the boundary has finished hydrating. The way this currently works is by scheduling a slightly higher priority task on the boundary, using a special lane that's reserved only for this purpose. Because the task is slightly higher priority, on the next turn of the work loop, the Scheduler will force the work loop to yield (i.e. shouldYield starts returning `true` because there's a higher priority task). The downside of this approach is that it only works when time slicing is enabled. It doesn't work for synchronous updates, because the synchronous work loop does not consult the Scheduler on each iteration. We plan to add support for selective hydration during synchronous updates, too, so we need to model this some other way. I've added a special internal exception that can be thrown to force the work loop to interrupt the work-in-progress tree. Because it's thrown from a React-only execution stack, throwing isn't strictly necessary — we could instead modify some internal work loop state. But using an exception means we don't need to check for this case on every iteration of the work loop. So doing it this way moves the check out of the fast path. The ideal implementation wouldn't need to unwind the stack at all — we should be able to hydrate the subtree and then apply the update all within a single render phase. This is how we intend to implement it in the future, but this requires a refactor to how we handle "stack" variables, which are currently pushed to a per-render array. We need to make this stack resumable, like how context works in Flight and Fizz.
tyao1
pushed a commit
that referenced
this pull request
Dec 8, 2022
When an update flows into a dehydrated boundary, React cannot apply the update until the boundary has finished hydrating. The way this currently works is by scheduling a slightly higher priority task on the boundary, using a special lane that's reserved only for this purpose. Because the task is slightly higher priority, on the next turn of the work loop, the Scheduler will force the work loop to yield (i.e. shouldYield starts returning `true` because there's a higher priority task). The downside of this approach is that it only works when time slicing is enabled. It doesn't work for synchronous updates, because the synchronous work loop does not consult the Scheduler on each iteration. We plan to add support for selective hydration during synchronous updates, too, so we need to model this some other way. I've added a special internal exception that can be thrown to force the work loop to interrupt the work-in-progress tree. Because it's thrown from a React-only execution stack, throwing isn't strictly necessary — we could instead modify some internal work loop state. But using an exception means we don't need to check for this case on every iteration of the work loop. So doing it this way moves the check out of the fast path. The ideal implementation wouldn't need to unwind the stack at all — we should be able to hydrate the subtree and then apply the update all within a single render phase. This is how we intend to implement it in the future, but this requires a refactor to how we handle "stack" variables, which are currently pushed to a per-render array. We need to make this stack resumable, like how context works in Flight and Fizz.
tyao1
added a commit
that referenced
this pull request
Dec 10, 2022
tyao1
pushed a commit
that referenced
this pull request
Dec 13, 2022
When an update flows into a dehydrated boundary, React cannot apply the update until the boundary has finished hydrating. The way this currently works is by scheduling a slightly higher priority task on the boundary, using a special lane that's reserved only for this purpose. Because the task is slightly higher priority, on the next turn of the work loop, the Scheduler will force the work loop to yield (i.e. shouldYield starts returning `true` because there's a higher priority task). The downside of this approach is that it only works when time slicing is enabled. It doesn't work for synchronous updates, because the synchronous work loop does not consult the Scheduler on each iteration. We plan to add support for selective hydration during synchronous updates, too, so we need to model this some other way. I've added a special internal exception that can be thrown to force the work loop to interrupt the work-in-progress tree. Because it's thrown from a React-only execution stack, throwing isn't strictly necessary — we could instead modify some internal work loop state. But using an exception means we don't need to check for this case on every iteration of the work loop. So doing it this way moves the check out of the fast path. The ideal implementation wouldn't need to unwind the stack at all — we should be able to hydrate the subtree and then apply the update all within a single render phase. This is how we intend to implement it in the future, but this requires a refactor to how we handle "stack" variables, which are currently pushed to a per-render array. We need to make this stack resumable, like how context works in Flight and Fizz.
tyao1
added a commit
that referenced
this pull request
Dec 15, 2022
github-actions bot
pushed a commit
that referenced
this pull request
Dec 15, 2022
This PR includes the previously reverted #25695 and #25754, and the fix for the regression test added in #25867. Tested internally with a previous failed test, and it's passing now. Co-authored-by: Andrew Clark <git@andrewclark.io> DiffTrain build for [7efa9e5](7efa9e5) [View git log for this commit](https://github.com/facebook/react/commits/7efa9e59707b341f10fab79724e0fca373187925)
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Jan 30, 2023
Summary: Three problems popped up during the sync: - facebook/react@07f46ecf2 breaks breaks tests - facebook/react@6fb8133ed breaks fbsource tests. I added a workaround and created a test for the team that owns the test. - https://fb.workplace.com/groups/flowlang/permalink/1198137807458547/ enables local type interference in fbsource but not in github React repo and some code breaks. Addressed in facebook/react#26064 This sync includes the following changes: - **[17f6912a4](facebook/react@17f6912a4 )**: Add flow types to ReactFiberHooks ([#25752](facebook/react#25752)) //<Samuel Susla>// - **[f101c2d0d](facebook/react@f101c2d0d )**: Remove Reconciler fork (2/2) ([#25775](facebook/react#25775)) //<Jan Kassens>// - **[420f0b7fa](facebook/react@420f0b7fa )**: Remove Reconciler fork (1/2) ([#25774](facebook/react#25774)) //<Jan Kassens>// - **[3ba7add60](facebook/react@3ba7add60 )**: Allow async blocks in `to(Error|Warn)Dev` ([#25338](facebook/react#25338)) //<Sebastian Silbermann>// - **[fa11bd6ec](facebook/react@fa11bd6ec )**: [ServerRenderer] Add option to send instructions as data attributes ([#25437](facebook/react#25437)) //<mofeiZ>// - **[e98225485](facebook/react@e98225485 )**: Add ref cleanup function ([#25686](facebook/react#25686)) //<Samuel Susla>// - **[15557fa67](facebook/react@15557fa67 )**: [Fix] properly track `useId` use in StrictMode in development ([#25713](facebook/react#25713)) //<Josh Story>// - **[8a23def32](facebook/react@8a23def32 )**: Resubmit Add HydrationSyncLane ([#25711](facebook/react#25711)) //<Tianyu Yao>// - **[2655c9354](facebook/react@2655c9354 )**: Fizz Browser: fix precomputed chunk being cleared on Node 18 ([#25645](facebook/react#25645)) //<Jimmy Lai>// - **[c08d8b804](facebook/react@c08d8b804 )**: Revert "Add SyncHydrationLane" ([#25708](facebook/react#25708)) //<Tianyu Yao>// - **[56ffca8b9](facebook/react@56ffca8b9 )**: Add Bun streaming server renderer ([#25597](facebook/react#25597)) //<Colin McDonnell>// - **[f31005d6a](facebook/react@f31005d6a )**: Add SyncHydrationLane ([#25698](facebook/react#25698)) //<Tianyu Yao>// - **[f284d9faf](facebook/react@f284d9faf )**: Track ThenableState alongside other hooks //<Andrew Clark>// - **[6b4c0314e](facebook/react@6b4c0314e )**: Check thenable instead of thenableState //<Andrew Clark>// - **[33e3d2878](facebook/react@33e3d2878 )**: Reuse hooks when replaying a suspended component //<Andrew Clark>// - **[4387d752d](facebook/react@4387d752d )**: Allow more hooks to be added when replaying mount //<Andrew Clark>// - **[5eb78d0a0](facebook/react@5eb78d0a0 )**: Pass ThenableState to replaySuspendedUnitOfWork //<Andrew Clark>// - **[4a2d86bdd](facebook/react@4a2d86bdd )**: Don't reset work loop until stack is unwound //<Andrew Clark>// - **[9dfbd9fa9](facebook/react@9dfbd9fa9 )**: use: Don't suspend if there are pending updates //<Andrew Clark>// - **[44c4e6f4d](facebook/react@44c4e6f4d )**: Force unwind work loop during selective hydration ([#25695](facebook/react#25695)) //<Andrew Clark>// - **[7b17f7bbf](facebook/react@7b17f7bbf )**: Enable warning for defaultProps on function components for everyone ([#25699](facebook/react#25699)) //<Sebastian Markbåge>// - **[6fb8133ed](facebook/react@6fb8133ed )**: Turn on string ref deprecation warning for everybody (not codemoddable) ([#25383](facebook/react#25383)) //<Sebastian Silbermann>// - **[07f46ecf2](facebook/react@07f46ecf2 )**: Turn on key spread warning in jsx-runtime for everyone ([#25697](facebook/react#25697)) //<Sebastian Markbåge>// - **[d65b88d03](facebook/react@d65b88d03 )**: Eagerly initialize an mutable object for instance.refs ([#25696](facebook/react#25696)) //<Sebastian Markbåge>// - **[c343f8025](facebook/react@c343f8025 )**: [react-float] feature detect getRootNode ([#25689](facebook/react#25689)) //<Jan Kassens>// - **[e1dd0a2f5](facebook/react@e1dd0a2f5 )**: Remove recoverable error when a sync update flows into a dehydrated boundary ([#25692](facebook/react#25692)) //<Sebastian Markbåge>// - **[c54e3541b](facebook/react@c54e3541b )**: [DevTools] bug fix for Hydrating fibers ([#25663](facebook/react#25663)) //<Mengdi Chen>// Changelog: [General][Changed] - React Native sync for revisions d1e35c7...17f6912 jest_e2e[run_all_tests] Reviewed By: makovkastar Differential Revision: D42804802 fbshipit-source-id: 6a9f00724cc73378025bbd04edb2d17760a87280
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: Three problems popped up during the sync: - facebook/react@07f46ecf2 breaks breaks tests - facebook/react@6fb8133ed breaks fbsource tests. I added a workaround and created a test for the team that owns the test. - https://fb.workplace.com/groups/flowlang/permalink/1198137807458547/ enables local type interference in fbsource but not in github React repo and some code breaks. Addressed in facebook/react#26064 This sync includes the following changes: - **[17f6912a4](facebook/react@17f6912a4 )**: Add flow types to ReactFiberHooks ([facebook#25752](facebook/react#25752)) //<Samuel Susla>// - **[f101c2d0d](facebook/react@f101c2d0d )**: Remove Reconciler fork (2/2) ([facebook#25775](facebook/react#25775)) //<Jan Kassens>// - **[420f0b7fa](facebook/react@420f0b7fa )**: Remove Reconciler fork (1/2) ([facebook#25774](facebook/react#25774)) //<Jan Kassens>// - **[3ba7add60](facebook/react@3ba7add60 )**: Allow async blocks in `to(Error|Warn)Dev` ([facebook#25338](facebook/react#25338)) //<Sebastian Silbermann>// - **[fa11bd6ec](facebook/react@fa11bd6ec )**: [ServerRenderer] Add option to send instructions as data attributes ([facebook#25437](facebook/react#25437)) //<mofeiZ>// - **[e98225485](facebook/react@e98225485 )**: Add ref cleanup function ([facebook#25686](facebook/react#25686)) //<Samuel Susla>// - **[15557fa67](facebook/react@15557fa67 )**: [Fix] properly track `useId` use in StrictMode in development ([facebook#25713](facebook/react#25713)) //<Josh Story>// - **[8a23def32](facebook/react@8a23def32 )**: Resubmit Add HydrationSyncLane ([facebook#25711](facebook/react#25711)) //<Tianyu Yao>// - **[2655c9354](facebook/react@2655c9354 )**: Fizz Browser: fix precomputed chunk being cleared on Node 18 ([facebook#25645](facebook/react#25645)) //<Jimmy Lai>// - **[c08d8b804](facebook/react@c08d8b804 )**: Revert "Add SyncHydrationLane" ([facebook#25708](facebook/react#25708)) //<Tianyu Yao>// - **[56ffca8b9](facebook/react@56ffca8b9 )**: Add Bun streaming server renderer ([facebook#25597](facebook/react#25597)) //<Colin McDonnell>// - **[f31005d6a](facebook/react@f31005d6a )**: Add SyncHydrationLane ([facebook#25698](facebook/react#25698)) //<Tianyu Yao>// - **[f284d9faf](facebook/react@f284d9faf )**: Track ThenableState alongside other hooks //<Andrew Clark>// - **[6b4c0314e](facebook/react@6b4c0314e )**: Check thenable instead of thenableState //<Andrew Clark>// - **[33e3d2878](facebook/react@33e3d2878 )**: Reuse hooks when replaying a suspended component //<Andrew Clark>// - **[4387d752d](facebook/react@4387d752d )**: Allow more hooks to be added when replaying mount //<Andrew Clark>// - **[5eb78d0a0](facebook/react@5eb78d0a0 )**: Pass ThenableState to replaySuspendedUnitOfWork //<Andrew Clark>// - **[4a2d86bdd](facebook/react@4a2d86bdd )**: Don't reset work loop until stack is unwound //<Andrew Clark>// - **[9dfbd9fa9](facebook/react@9dfbd9fa9 )**: use: Don't suspend if there are pending updates //<Andrew Clark>// - **[44c4e6f4d](facebook/react@44c4e6f4d )**: Force unwind work loop during selective hydration ([facebook#25695](facebook/react#25695)) //<Andrew Clark>// - **[7b17f7bbf](facebook/react@7b17f7bbf )**: Enable warning for defaultProps on function components for everyone ([facebook#25699](facebook/react#25699)) //<Sebastian Markbåge>// - **[6fb8133ed](facebook/react@6fb8133ed )**: Turn on string ref deprecation warning for everybody (not codemoddable) ([facebook#25383](facebook/react#25383)) //<Sebastian Silbermann>// - **[07f46ecf2](facebook/react@07f46ecf2 )**: Turn on key spread warning in jsx-runtime for everyone ([facebook#25697](facebook/react#25697)) //<Sebastian Markbåge>// - **[d65b88d03](facebook/react@d65b88d03 )**: Eagerly initialize an mutable object for instance.refs ([facebook#25696](facebook/react#25696)) //<Sebastian Markbåge>// - **[c343f8025](facebook/react@c343f8025 )**: [react-float] feature detect getRootNode ([facebook#25689](facebook/react#25689)) //<Jan Kassens>// - **[e1dd0a2f5](facebook/react@e1dd0a2f5 )**: Remove recoverable error when a sync update flows into a dehydrated boundary ([facebook#25692](facebook/react#25692)) //<Sebastian Markbåge>// - **[c54e3541b](facebook/react@c54e3541b )**: [DevTools] bug fix for Hydrating fibers ([facebook#25663](facebook/react#25663)) //<Mengdi Chen>// Changelog: [General][Changed] - React Native sync for revisions d1e35c7...17f6912 jest_e2e[run_all_tests] Reviewed By: makovkastar Differential Revision: D42804802 fbshipit-source-id: 6a9f00724cc73378025bbd04edb2d17760a87280
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an update flows into a dehydrated boundary, React cannot apply the update until the boundary has finished hydrating. The way this currently works is by scheduling a slightly higher priority task on the boundary, using a special lane that's reserved only for this purpose. Because the task is slightly higher priority, on the next turn of the work loop, the Scheduler will force the work loop to yield (i.e. shouldYield starts returning
truebecause there's a higher priority task).The downside of this approach is that it only works when time slicing is enabled. It doesn't work for synchronous updates, because the synchronous work loop does not consult the Scheduler on each iteration.
We plan to add support for selective hydration during synchronous updates, too, so we need to model this some other way.
I've added a special internal exception that can be thrown to force the work loop to interrupt the work-in-progress tree. Because it's thrown from a React-only execution stack, throwing isn't strictly necessary — we could instead modify some internal work loop state. But using an exception means we don't need to check for this case on every iteration of the work loop. So doing it this way moves the check out of the fast path.
The ideal implementation wouldn't need to unwind the stack at all — we should be able to hydrate the subtree and then apply the update all within a single render phase. This is how we intend to implement it in the future, but this requires a refactor to how we handle "stack" variables, which are currently pushed to a per-render array. We need to make this stack resumable, like how context works in Flight and Fizz.