[PR Stack] Refactor mutation phase to use recursion #24308
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: 8dcedba...ec52a56 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
|
gaearon
requested changes
Apr 8, 2022
acdlite
force-pushed
the
recursive-mutation-phase
branch
from
6c60893 to
04d140c
Compare
acdlite
force-pushed
the
recursive-mutation-phase
branch
from
04d140c to
5941a90
Compare
|
Will have another look in 20 min |
gaearon
reviewed
Apr 8, 2022
| return; | ||
| } | ||
| case HostRoot: { | ||
| if (supportsHydration) { | ||
| if (supportsMutation) { |
There was a problem hiding this comment.
Shouldn't this be supportsMutation && supportsHydration? I guess it wouldn't hit isDehydrated in other renderers but might want to cut it out if third-party renderers like three run with GCC.
There was a problem hiding this comment.
Yeah sorry my brain keeps reading these as the same flag
gaearon
reviewed
Apr 8, 2022
gaearon
reviewed
Apr 8, 2022
| // TODO: This is a temporary solution that allowed us to transition away | ||
| // from React Flare on www. | ||
| if (flags & Ref) { | ||
| commitAttachRef(finishedWork); |
There was a problem hiding this comment.
Does this also need
if (current !== null) {
commitDetachRef(current);
}before this line?
gaearon
reviewed
Apr 8, 2022
| try { | ||
| const instance = node.stateNode; | ||
| if (isHidden) { | ||
| hideInstance(instance); |
There was a problem hiding this comment.
Should we also do this for (un)hideTextInstance below?
There was a problem hiding this comment.
Yeah I missed that one
acdlite
commented
Apr 8, 2022
| if (flags & Update) { | ||
| attachSuspenseRetryListeners(finishedWork); | ||
| } | ||
| return; | ||
| } | ||
| case ScopeComponent: { | ||
| if (flags & Update) { | ||
| if (enableScopeAPI) { | ||
| commitReconciliationEffects(finishedWork); |
There was a problem hiding this comment.
This should be inside enableScopeAPI so it gets DCE'd
gaearon
reviewed
Apr 8, 2022
| nextEffect = sibling; | ||
| return; | ||
| const prevDebugFiber = getCurrentDebugFiberInDEV(); | ||
| if (parentFiber.subtreeFlags & MutationMask) { |
There was a problem hiding this comment.
Would a strict comparison here be faster than implicit coercion to boolean?
There was a problem hiding this comment.
We're not consistent elsewhere but my understanding is that it's identical performance-wise (since the memory layout is the same) in most VMs, so lately I've been doing it this way since it's fewer bytes
| // to `null` on the stack to indicate that nested children don't | ||
| // need to be removed. | ||
| if (supportsMutation) { | ||
| const prevHostParent = hostParent; |
There was a problem hiding this comment.
In case the previous host parent was a Container, do we need to remember/restore hostParentIsContainer too?
There was a problem hiding this comment.
We don't use one without checking the other so I left that one out. Ideally they'd be the same variable. I could set it for consistency's sake even though it's never used.
There was a problem hiding this comment.
Hmm I don't understand, can you clarify how this works? The scenario I'm imagining is:
- We're at the top level or directly in a portal,
hostParentIsContainer = true - Then we step into a host component,
hostParentIsContainer = false - Then we step out of the host component, restoring
hostParentto point to the container, buthostParentIsContaineris stillfalse(because we didn't restore it) - There is a deletion. We're calling
removeChildbut should've calledremoveChildFromContainer.
There was a problem hiding this comment.
Then we step into a host component, hostParentIsContainer = false
This assignment doesn't exist. That's the part I intentionally skipped. There's a hostParent = null but not a hostParentIsContainer = false.
In this path, because we don't mutate hostParentIsContainer one the way down, we don't have to restore it on the way up.
I agree it's a bit confusing though so I'll add the hostParentIsContainer (both before and after) for consistency, even though it's not needed.
There was a problem hiding this comment.
Yea I keep forgetting you only ever need it one level down.
There was a problem hiding this comment.
I think it's fine if you add some comment about this specifically.
| // deleted subtree. | ||
| // TODO: Update these during the whole mutation phase, not just during | ||
| // a deletion. | ||
| let hostParent: Instance | Container | null = null; |
There was a problem hiding this comment.
Should we null it out at the end?
gaearon
reviewed
Apr 8, 2022
| switch (deletedFiber.tag) { | ||
| case HostComponent: | ||
| case HostText: { | ||
| safelyDetachRef(deletedFiber, nearestMountedAncestor); |
There was a problem hiding this comment.
We can skip safelyDetachRef for HostText. Would that be better?
There was a problem hiding this comment.
Yeah I suppose we can also skip the traversal since they never have children, either.
| // to `null` on the stack to indicate that nested children don't | ||
| // need to be removed. | ||
| if (supportsMutation) { | ||
| const prevHostParent = hostParent; |
There was a problem hiding this comment.
Hmm I don't understand, can you clarify how this works? The scenario I'm imagining is:
- We're at the top level or directly in a portal,
hostParentIsContainer = true - Then we step into a host component,
hostParentIsContainer = false - Then we step out of the host component, restoring
hostParentto point to the container, buthostParentIsContaineris stillfalse(because we didn't restore it) - There is a deletion. We're calling
removeChildbut should've calledremoveChildFromContainer.
gaearon
approved these changes
Apr 8, 2022
|
Thanks so much for the careful code review! Makes these refactors less scary. And congratulations, now I'm going to tag you as reviewer on the other commit phases when I do those 😆 |
gaearon
reviewed
Apr 8, 2022
| } | ||
| // We only need to remove the topmost host child in each branch. Once we | ||
| // reach that node, we can use a simpler recursive function | ||
| // (commitNestedUnmounts_iterative) that only unmounts effects, refs, and cWU. |
There was a problem hiding this comment.
Looks like you got rid of commitNestedUnmounts_iterative. Stale comment.
|
Ok I'm going to land these as individual commits in case we need to bisect later. I'll push them individually to this PR so CI runs. |
I'm about to refactor part of the commit phase to use recursion instead of iteration. As part of that change, we will no longer assign the `return` pointer when traversing into a subtree. So I'm disabling the internal warning that fires if the return pointer is not consistent with the parent during the commit phase. I had originally added this warning to help prevent mistakes when traversing the tree iteratively, but since we're intentionally switching to recursion instead, we don't need it.
acdlite
force-pushed
the
recursive-mutation-phase
branch
from
5941a90 to
ea7b2ec
Compare
commitWork is forked into a separate implementation for mutation mode (DOM) and persistent mode (React Native). But unlike when it was first introduced, there's more overlap than differences between the forks, mainly because we've added new types of fibers. So this joins the two forks and adds more local branches where the behavior actually diverges: host nodes, host containers, and portals.
There's not really any reason these should be separate functions. The factoring has gotten sloppy and redundant because there's similar logic in both places, which is more obvious now that they're combined. Next I'll start combining the redundant branches.
The fiber tag is more specific than the effect flag, so we should always refine the type of work first, to minimize redundant checks. In the next step I'll move all other other flag checks in this function into the same switch statement.
We should always refine the type of fiber before checking the effect flag, because the fiber tag is more specific. Now we have a single switch statement for all mutation effects.
reportUncaughtErrorInDev is always followed by captureCommitPhaseError, so we can move it into that function.
This moves the try-catch from around each fiber's mutation 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.
Most of the commit phase uses iterative loops to traverse the tree. Originally we thought this would be faster than using recursion, but a while back @trueadm did some performance testing and found that the loop was slower because we assign to the `return` pointer before entering a subtree (which we have to do because the `return` pointer is not always consistent; it could point to one of two fibers). The other motivation is so we can take advantage of the JS stack to track contextual information, like the nearest host parent. We already use recursion in a few places; this changes the mutation phase to use it, too.
This was referenced Nov 8, 2024
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.
Using this PR to track a stack of changes to refactor the mutation phase to use a recursive tree traversal instead of iterative.
It's too big to review all at once, so I broke it into multiple commits. Because GitHub won't let me stack PRs, the easiest way to review is commit-by-commit.
Everything except the final commit is a pure refactor. There should be no observable changes to behavior.
The final commit is an updated version of #24282 that uses the stack.
I probably will land each commit individually so it's easier to bisect if something goes wrong.
I considered wrapping it in a feature flag but because it involves many different functions, the combined code size got pretty large. If we need to, we can use the old/new reconciler fork to gradually roll out so that you only have to pay the cost of one implementation or the other.