Skip to content

Commit

Permalink
Set return pointer when reusing current tree (#20212)
Browse files Browse the repository at this point in the history
* 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.

* 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.
  • Loading branch information
acdlite authored Nov 10, 2020
1 parent 0898660 commit c896cf9
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 14 deletions.
14 changes: 0 additions & 14 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,6 @@ function iterativelyCommitBeforeMutationEffects_begin() {
(fiber.subtreeFlags & BeforeMutationMask) !== NoFlags &&
child !== null
) {
child.return = fiber;
nextEffect = child;
} else {
iterativelyCommitBeforeMutationEffects_complete();
Expand Down Expand Up @@ -497,7 +496,6 @@ function iterativelyCommitBeforeMutationEffects_complete() {

const sibling = fiber.sibling;
if (sibling !== null) {
sibling.return = fiber.return;
nextEffect = sibling;
return;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -754,7 +751,6 @@ function iterativelyCommitMutationEffects_complete(

const sibling = fiber.sibling;
if (sibling !== null) {
sibling.return = fiber.return;
nextEffect = sibling;
return;
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -1230,7 +1224,6 @@ function iterativelyCommitLayoutEffects_complete(

const sibling = fiber.sibling;
if (sibling !== null) {
sibling.return = fiber.return;
nextEffect = sibling;
return;
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -1817,7 +1808,6 @@ function iterativelyCommitPassiveMountEffects_complete(

const sibling = fiber.sibling;
if (sibling !== null) {
sibling.return = fiber.return;
nextEffect = sibling;
return;
}
Expand Down Expand Up @@ -1896,7 +1886,6 @@ function iterativelyCommitPassiveUnmountEffects_begin() {
}

if ((fiber.subtreeFlags & PassiveMask) !== NoFlags && child !== null) {
child.return = fiber;
nextEffect = child;
} else {
iterativelyCommitPassiveUnmountEffects_complete();
Expand All @@ -1915,7 +1904,6 @@ function iterativelyCommitPassiveUnmountEffects_complete() {

const sibling = fiber.sibling;
if (sibling !== null) {
sibling.return = fiber.return;
nextEffect = sibling;
return;
}
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -1981,7 +1968,6 @@ function iterativelyCommitPassiveUnmountEffectsInsideOfDeletedTree_complete(

const sibling = fiber.sibling;
if (sibling !== null) {
sibling.return = fiber.return;
nextEffect = sibling;
return;
}
Expand Down
10 changes: 10 additions & 0 deletions packages/react-reconciler/src/ReactFiberCompleteWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
}
}
Expand Down

0 comments on commit c896cf9

Please sign in to comment.