Refactor layout phase to use recursion #24906
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
Only certain fiber types can have refs attached to them, so this moves the Ref effect logic out of the common path and into the corresponding branch of the layout phase's switch statement. The types of fibers this affects are host components and class components. Function components are not affected because they can only have a ref via useImperativeHandle, which has a different implementation. The experimental Scope type attaches its refs in the mutation phase, not the layout phase.
The fiber tag is more specific than the effect flag, so we should always refine the type of work first, to minimize redundant checks.
(This is the same as f9e6aef, but for the layout phase rather than the mutation phase.) This moves the try-catch from around each fiber's layout phase to direclty around each user function (effect function, callback, etc). We already do this when unmounting because if one unmount function errors, we still need to call all the others so they can clean up their resources. Previously we didn't bother to do this for anything but unmount, because if a mount effect throws, we're going to delete that whole tree anyway. But now that we're switching from an iterative loop to a recursive one, we don't want every call frame on the stack to have a try-catch, since the error handling requires additional memory. Wrapping every user function is a bit tedious, but it's better for performance. Many of them already had try blocks around them already.
This converts the layout phase to iterate over its effects recursively instead of iteratively. This makes it easier to track contextual information, like whether a fiber is inside a hidden tree. We already made this change for the mutation phase. See 481dece for more context.
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: e225fa4...b641d02 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
|
rickhanlonii
approved these changes
Jul 12, 2022
| } | ||
| case OffscreenComponent: { | ||
| const isModernRoot = (finishedWork.mode & ConcurrentMode) !== NoMode; | ||
| if (isModernRoot) { |
There was a problem hiding this comment.
Do we typically inline this?
There was a problem hiding this comment.
What do you mean? Like as opposed to outside the switch statement? Generally I like to put these types of things as local as possible so that it's only checked in the branches that need it
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.
This converts the layout phase to iterate over its effects recursively instead of iteratively. This makes it easier to track
contextual information, like whether a fiber is inside a hidden tree.
We already made this change for the mutation phase in #24308. This PR follows a similar structure, though is a bit more straightforward.
Because this is largely a structural refactor as opposed to a logical one, I would like to attempt landing this using the same strategy as #24308: land the stack directly in main, without a feature flag. If there's a bug somewhere, it should be easy to bisect and fix forward.
If it turns out to be trickier than expected, we can revert and use the reconciler fork infra instead. I don't want to jump straight to using the fork infra because it will make it harder for other contributors to land changes to the commit phase in the meantime.