From 45c295e8e9d8f7e69083dff94ce91b1791074603 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 10 Nov 2020 12:06:29 -0600 Subject: [PATCH 1/2] Do not fix return pointers during commit phase In the commit phase, we should be able to assume that the `return` pointers in the just-completed tree are consistent. The render phase should be responsible for ensuring these are always correct. I've removed the `return` pointer assignments from the render phase traversal logic. This isn't all of them, only the ones added recently during the effects refactor. The other ones have been around longer so I'll leave those for a later clean up. This breaks a few SuspenseList tests; I'll fix in the next commit. --- .../src/ReactFiberCommitWork.new.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 6f0b923c14445..55c7c986effc7 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -463,7 +463,6 @@ function iterativelyCommitBeforeMutationEffects_begin() { (fiber.subtreeFlags & BeforeMutationMask) !== NoFlags && child !== null ) { - child.return = fiber; nextEffect = child; } else { iterativelyCommitBeforeMutationEffects_complete(); @@ -497,7 +496,6 @@ function iterativelyCommitBeforeMutationEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } @@ -715,7 +713,6 @@ function iterativelyCommitMutationEffects_begin( const child = fiber.child; if ((fiber.subtreeFlags & MutationMask) !== NoFlags && child !== null) { - child.return = fiber; nextEffect = child; } else { iterativelyCommitMutationEffects_complete(root, renderPriorityLevel); @@ -754,7 +751,6 @@ function iterativelyCommitMutationEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } @@ -1176,14 +1172,12 @@ function iterativelyCommitLayoutEffects_begin( } const sibling = finishedWork.sibling; if (sibling !== null) { - sibling.return = finishedWork.return; nextEffect = sibling; } else { nextEffect = finishedWork.return; iterativelyCommitLayoutEffects_complete(subtreeRoot, finishedRoot); } } else { - firstChild.return = finishedWork; nextEffect = firstChild; } } else { @@ -1230,7 +1224,6 @@ function iterativelyCommitLayoutEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } @@ -1764,14 +1757,12 @@ function iterativelyCommitPassiveMountEffects_begin( } const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; } else { nextEffect = fiber.return; iterativelyCommitPassiveMountEffects_complete(subtreeRoot, root); } } else { - firstChild.return = fiber; nextEffect = firstChild; } } else { @@ -1817,7 +1808,6 @@ function iterativelyCommitPassiveMountEffects_complete( const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } @@ -1896,7 +1886,6 @@ function iterativelyCommitPassiveUnmountEffects_begin() { } if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && child !== null) { - child.return = fiber; nextEffect = child; } else { iterativelyCommitPassiveUnmountEffects_complete(); @@ -1915,7 +1904,6 @@ function iterativelyCommitPassiveUnmountEffects_complete() { const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } @@ -1953,7 +1941,6 @@ function iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_begin( const fiber = nextEffect; const child = fiber.child; if ((fiber.subtreeFlags & PassiveStatic) !== NoFlags && child !== null) { - child.return = fiber; nextEffect = child; } else { iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_complete( @@ -1981,7 +1968,6 @@ function iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_complete( const sibling = fiber.sibling; if (sibling !== null) { - sibling.return = fiber.return; nextEffect = sibling; return; } From 858c8a92f739f6ba5071114af960255479636c8b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 10 Nov 2020 12:10:23 -0600 Subject: [PATCH 2/2] Set return pointer when reusing current tree We always set the return pointer on freshly cloned, work-in-progress fibers. However, we were neglecting to set them on trees that are reused from current. I fixed this in the same path of the complete phase where we reset the fiber flags. This is a code smell because it assumes the commit phase is never concurrent with the render phase. Our eventual goal is to make fibers a lock free data structure. Will address further during refactor to alternate model. --- .../react-reconciler/src/ReactFiberCompleteWork.new.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index 0a187f9c84b07..1b353a097268d 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -738,6 +738,11 @@ function bubbleProperties(completedWork: Fiber) { subtreeFlags |= child.subtreeFlags; subtreeFlags |= child.flags; + // Update the return pointer so the tree is consistent. This is a code + // smell because it assumes the commit phase is never concurrent with + // the render phase. Will address during refactor to alternate model. + child.return = completedWork; + child = child.sibling; } } @@ -784,6 +789,11 @@ function bubbleProperties(completedWork: Fiber) { subtreeFlags |= child.subtreeFlags & StaticMask; subtreeFlags |= child.flags & StaticMask; + // Update the return pointer so the tree is consistent. This is a code + // smell because it assumes the commit phase is never concurrent with + // the render phase. Will address during refactor to alternate model. + child.return = completedWork; + child = child.sibling; } }