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

[PR Stack] Refactor mutation phase to use recursion #24308

Merged
merged 10 commits into from Apr 8, 2022

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Apr 8, 2022

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.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 8, 2022
@sizebot
Copy link

sizebot commented Apr 8, 2022

Comparing: 8dcedba...ec52a56

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 131.40 kB 131.23 kB = 41.98 kB 41.95 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.66 kB 136.49 kB +0.05% 43.51 kB 43.53 kB
facebook-www/ReactDOM-prod.classic.js +0.19% 433.62 kB 434.46 kB +0.02% 79.74 kB 79.76 kB
facebook-www/ReactDOM-prod.modern.js +0.20% 418.62 kB 419.45 kB +0.02% 77.38 kB 77.40 kB
facebook-www/ReactDOMForked-prod.classic.js +0.19% 433.62 kB 434.46 kB +0.02% 79.75 kB 79.76 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactDOMTesting-prod.modern.js +0.55% 412.50 kB 414.75 kB +0.23% 77.57 kB 77.75 kB
react-native/implementations/ReactNativeRenderer-prod.js +0.53% 284.08 kB 285.58 kB +0.33% 51.31 kB 51.48 kB
facebook-www/ReactDOMTesting-prod.classic.js +0.53% 428.92 kB 431.18 kB +0.24% 80.31 kB 80.50 kB
react-native/implementations/ReactNativeRenderer-prod.fb.js +0.51% 292.21 kB 293.71 kB +0.32% 53.00 kB 53.17 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js +0.51% 248.14 kB 249.41 kB +0.16% 45.40 kB 45.47 kB
react-native/implementations/ReactNativeRenderer-profiling.fb.js +0.50% 319.07 kB 320.68 kB +0.26% 57.17 kB 57.31 kB
react-native/implementations/ReactFabric-prod.js +0.50% 277.47 kB 278.85 kB +0.44% 50.12 kB 50.34 kB
react-native/implementations/ReactNativeRenderer-profiling.js +0.49% 303.09 kB 304.58 kB +0.21% 54.49 kB 54.60 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-profiling.js +0.48% 262.93 kB 264.20 kB +0.08% 47.74 kB 47.78 kB
react-native/implementations/ReactFabric-prod.fb.js +0.47% 290.22 kB 291.60 kB +0.41% 52.55 kB 52.76 kB
react-native/implementations/ReactFabric-profiling.js +0.46% 296.44 kB 297.80 kB +0.32% 53.39 kB 53.56 kB
react-native/implementations/ReactFabric-profiling.fb.js +0.42% 317.19 kB 318.52 kB +0.26% 56.83 kB 56.98 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-dev.js +0.38% 655.19 kB 657.67 kB +0.28% 141.88 kB 142.28 kB
facebook-www/ReactTestRenderer-dev.classic.js +0.35% 676.61 kB 679.01 kB +0.27% 145.82 kB 146.22 kB
facebook-www/ReactTestRenderer-dev.modern.js +0.35% 676.63 kB 679.03 kB +0.27% 145.83 kB 146.23 kB
react-native/implementations/ReactNativeRenderer-dev.js +0.34% 745.42 kB 747.91 kB +0.22% 163.52 kB 163.89 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.development.js +0.33% 617.64 kB 619.65 kB +0.17% 135.55 kB 135.78 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.development.js +0.33% 617.64 kB 619.65 kB +0.17% 135.55 kB 135.78 kB
facebook-www/ReactART-dev.modern.js +0.32% 755.64 kB 758.06 kB +0.24% 161.45 kB 161.83 kB
facebook-www/ReactART-dev.classic.js +0.32% 765.85 kB 768.27 kB +0.23% 163.58 kB 163.96 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.development.js +0.31% 643.08 kB 645.10 kB +0.16% 140.73 kB 140.96 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.development.js +0.31% 647.48 kB 649.51 kB +0.17% 136.99 kB 137.23 kB
oss-stable/react-test-renderer/umd/react-test-renderer.development.js +0.31% 647.48 kB 649.51 kB +0.17% 136.99 kB 137.23 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.development.js +0.30% 674.24 kB 676.27 kB +0.21% 142.15 kB 142.46 kB
facebook-www/ReactDOMTesting-dev.modern.js +0.30% 1,026.35 kB 1,029.38 kB +0.23% 231.33 kB 231.86 kB
oss-stable-semver/react-art/cjs/react-art.development.js +0.29% 675.39 kB 677.36 kB +0.20% 146.15 kB 146.45 kB
oss-stable/react-art/cjs/react-art.development.js +0.29% 675.39 kB 677.36 kB +0.20% 146.15 kB 146.45 kB
react-native/implementations/ReactNativeRenderer-dev.fb.js +0.29% 787.86 kB 790.16 kB +0.22% 171.31 kB 171.69 kB
facebook-www/ReactDOMTesting-dev.classic.js +0.29% 1,055.44 kB 1,058.46 kB +0.22% 237.31 kB 237.83 kB
oss-experimental/react-art/cjs/react-art.development.js +0.28% 702.51 kB 704.49 kB +0.19% 151.82 kB 152.12 kB
oss-stable-semver/react-art/umd/react-art.development.js +0.26% 778.75 kB 780.75 kB +0.18% 164.39 kB 164.68 kB
oss-stable/react-art/umd/react-art.development.js +0.26% 778.75 kB 780.75 kB +0.18% 164.39 kB 164.68 kB
oss-experimental/react-art/umd/react-art.development.js +0.25% 807.23 kB 809.22 kB +0.17% 170.00 kB 170.29 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.development.js +0.22% 1,001.84 kB 1,004.04 kB +0.13% 226.79 kB 227.08 kB
facebook-www/ReactDOMForked-dev.modern.js +0.22% 1,113.70 kB 1,116.12 kB +0.16% 247.03 kB 247.42 kB
facebook-www/ReactDOM-dev.modern.js +0.22% 1,113.70 kB 1,116.12 kB +0.16% 247.03 kB 247.42 kB
facebook-www/ReactDOMForked-dev.classic.js +0.21% 1,138.15 kB 1,140.57 kB +0.15% 251.61 kB 251.99 kB
facebook-www/ReactDOM-dev.classic.js +0.21% 1,138.15 kB 1,140.57 kB +0.15% 251.61 kB 251.99 kB
react-native/implementations/ReactFabric-dev.js +0.21% 736.94 kB 738.47 kB = 161.76 kB 161.69 kB
oss-experimental/react-art/umd/react-art.production.min.js = 122.52 kB 122.21 kB = 37.93 kB 37.90 kB
oss-stable-semver/react-art/umd/react-art.production.min.js = 117.78 kB 117.47 kB = 36.51 kB 36.49 kB
oss-stable/react-art/umd/react-art.production.min.js = 117.78 kB 117.47 kB = 36.51 kB 36.49 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.production.min.js = 88.65 kB 88.40 kB +0.16% 27.45 kB 27.50 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.production.min.js = 84.20 kB 83.95 kB = 26.16 kB 26.14 kB
oss-stable/react-test-renderer/umd/react-test-renderer.production.min.js = 84.20 kB 83.95 kB = 26.16 kB 26.14 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.production.min.js = 88.46 kB 88.18 kB = 27.22 kB 27.14 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.production.min.js = 84.00 kB 83.72 kB = 25.85 kB 25.73 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.production.min.js = 84.00 kB 83.72 kB = 25.85 kB 25.73 kB
oss-experimental/react-reconciler/cjs/react-reconciler.production.min.js = 97.55 kB 97.19 kB = 29.76 kB 29.75 kB
oss-experimental/react-art/cjs/react-art.production.min.js = 86.69 kB 86.37 kB = 26.78 kB 26.66 kB
oss-stable-semver/react-art/cjs/react-art.production.min.js = 81.88 kB 81.57 kB = 25.31 kB 25.29 kB
oss-stable/react-art/cjs/react-art.production.min.js = 81.88 kB 81.57 kB = 25.31 kB 25.29 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.production.min.js = 92.69 kB 92.34 kB = 28.43 kB 28.39 kB
oss-stable/react-reconciler/cjs/react-reconciler.production.min.js = 92.69 kB 92.34 kB = 28.43 kB 28.39 kB
oss-experimental/react-reconciler/cjs/react-reconciler.profiling.min.js = 106.45 kB 106.02 kB +0.06% 32.04 kB 32.06 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.profiling.min.js = 101.59 kB 101.17 kB = 30.62 kB 30.61 kB
oss-stable/react-reconciler/cjs/react-reconciler.profiling.min.js = 101.59 kB 101.17 kB = 30.62 kB 30.61 kB

Generated by 🚫 dangerJS against ec52a56

@gaearon
Copy link
Collaborator

gaearon commented Apr 8, 2022

Will have another look in 20 min

return;
}
case HostRoot: {
if (supportsHydration) {
if (supportsMutation) {
Copy link
Collaborator

@gaearon gaearon Apr 8, 2022

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.

Copy link
Collaborator Author

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);
Copy link
Collaborator

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);
Copy link
Collaborator

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?

Copy link
Collaborator Author

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);
Copy link
Collaborator Author

@acdlite acdlite Apr 8, 2022

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) {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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;
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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, but hostParentIsContainer is still false (because we didn't restore it)
  • There is a deletion. We're calling removeChild but should've called removeChildFromContainer.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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;
Copy link
Collaborator

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);
Copy link
Collaborator

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?

Copy link
Collaborator Author

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;
Copy link
Collaborator

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, but hostParentIsContainer is still false (because we didn't restore it)
  • There is a deletion. We're calling removeChild but should've called removeChildFromContainer.

Copy link
Collaborator

@gaearon gaearon left a 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

@acdlite
Copy link
Collaborator Author

acdlite commented 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 😆

}
// 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.
Copy link
Collaborator

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.

@acdlite
Copy link
Collaborator Author

acdlite commented Apr 8, 2022

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.
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.
acdlite and others added 2 commits April 8, 2022 18:01
Similar to the previous step, this converts the deletion phase into
a single recursive function. Although there's less code, this one is
a bit trickier because it's already contains some stack-like logic
for tracking the nearest host parent. But instead of using the actual
stack, it repeatedly searches up the fiber return path to find the
nearest host parent.

Instead, I've changed it to track the nearest host parent on the
JS stack.

(We still search up the return path once, to set the initial host parent
right before entering a deleted tree. As a follow up, we can instead
push this to the stack as we traverse during the main mutation phase.)
When a tree goes offscreen, we unmount all the effects just like we
would in a normal deletion. (Conceptually it _is_ a deletion; we keep
the fiber around so we can reuse its state if the tree mounts again.)

If an offscreen component gets deleted "for real", we shouldn't unmount
it again.

The fix is to track on the stack whether we're inside a hidden tree.

We already had a stack variable for this purpose, called
`offscreenSubtreeWasHidden`, in another part of the commit phase, so I
reused that variable instead of creating a new one. (The name is a bit
confusing: "was" refers to the current tree before this commit. So, the
"previous current".)

Co-authored-by: dan <dan.abramov@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants