-
Notifications
You must be signed in to change notification settings - Fork 46.2k
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
[PR Stack] Refactor mutation phase to use recursion #24308
Conversation
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
|
6c60893
to
04d140c
Compare
04d140c
to
5941a90
Compare
Will have another look in 20 min |
return; | ||
} | ||
case HostRoot: { | ||
if (supportsHydration) { | ||
if (supportsMutation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sorry my brain keeps reading these as the same flag
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also need
if (current !== null) {
commitDetachRef(current);
}
before this line?
try { | ||
const instance = node.stateNode; | ||
if (isHidden) { | ||
hideInstance(instance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also do this for (un)hideTextInstance
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I missed that one
if (flags & Update) { | ||
attachSuspenseRetryListeners(finishedWork); | ||
} | ||
return; | ||
} | ||
case ScopeComponent: { | ||
if (flags & Update) { | ||
if (enableScopeAPI) { | ||
commitReconciliationEffects(finishedWork); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be inside enableScopeAPI
so it gets DCE'd
nextEffect = sibling; | ||
return; | ||
const prevDebugFiber = getCurrentDebugFiberInDEV(); | ||
if (parentFiber.subtreeFlags & MutationMask) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a strict comparison here be faster than implicit coercion to boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case the previous host parent was a Container, do we need to remember/restore hostParentIsContainer
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
hostParent
to point to the container, buthostParentIsContainer
is stillfalse
(because we didn't restore it) - There is a deletion. We're calling
removeChild
but should've calledremoveChildFromContainer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I keep forgetting you only ever need it one level down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we null it out at the end?
switch (deletedFiber.tag) { | ||
case HostComponent: | ||
case HostText: { | ||
safelyDetachRef(deletedFiber, nearestMountedAncestor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can skip safelyDetachRef
for HostText
. Would that be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
hostParent
to point to the container, buthostParentIsContainer
is stillfalse
(because we didn't restore it) - There is a deletion. We're calling
removeChild
but should've calledremoveChildFromContainer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. (Aside from things above.)
SO MUCH CLEANER
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 😆 |
} | ||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.