Remove remaining references to effect list (#19673)

* Remove `firstEffect` null check

This is the last remaining place where the effect list has semantic
implications.

I've replaced it with a check of `effectTag` and `subtreeTag`, to see
if there are any effects in the whole tree. This matches the semantics
of the old check. However, I think only reason this optimization exists
is because it affects profiling. We should reconsider whether this
is necessary.

* Remove remaining references to effect list

We no longer use the effect list anywhere in our implementation. It's
been replaced by a recursive traversal in the commit phase.

This removes all references to the effect list in the new fork.
This commit is contained in:
Andrew Clark 2020-08-21 20:42:32 -05:00 committed by GitHub
parent 0386bd0da2
commit d2e914ab4e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 41 additions and 205 deletions

View File

@ -277,28 +277,13 @@ function ChildReconciler(shouldTrackSideEffects) {
// Noop.
return;
}
// Deletions are added in reversed order so we add it to the front.
// At this point, the return fiber's effect list is empty except for
// deletions, so we can just append the deletion to the list. The remaining
// effects aren't added until the complete phase. Once we implement
// resuming, this may not be true.
// TODO (effects) Get rid of effects list update here.
const last = returnFiber.lastEffect;
if (last !== null) {
last.nextEffect = childToDelete;
returnFiber.lastEffect = childToDelete;
} else {
returnFiber.firstEffect = returnFiber.lastEffect = childToDelete;
}
const deletions = returnFiber.deletions;
if (deletions === null) {
returnFiber.deletions = [childToDelete];
// TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
returnFiber.effectTag |= Deletion;
} else {
deletions.push(childToDelete);
}
childToDelete.nextEffect = null;
}
function deleteRemainingChildren(

View File

@ -147,10 +147,6 @@ function FiberNode(
this.effectTag = NoEffect;
this.subtreeTag = NoSubtreeEffect;
this.deletions = null;
this.nextEffect = null;
this.firstEffect = null;
this.lastEffect = null;
this.lanes = NoLanes;
this.childLanes = NoLanes;
@ -291,11 +287,6 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber {
workInProgress.subtreeTag = NoSubtreeEffect;
workInProgress.deletions = null;
// The effect list is no longer valid.
workInProgress.nextEffect = null;
workInProgress.firstEffect = null;
workInProgress.lastEffect = null;
if (enableProfilerTimer) {
// We intentionally reset, rather than copy, actualDuration & actualStartTime.
// This prevents time from endlessly accumulating in new commits.
@ -374,11 +365,6 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) {
// that child fiber is setting, not the reconciliation.
workInProgress.effectTag &= Placement;
// The effect list is no longer valid.
workInProgress.nextEffect = null;
workInProgress.firstEffect = null;
workInProgress.lastEffect = null;
const current = workInProgress.alternate;
if (current === null) {
// Reset to createFiber's initial values.
@ -386,6 +372,7 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) {
workInProgress.lanes = renderLanes;
workInProgress.child = null;
workInProgress.subtreeTag = NoSubtreeEffect;
workInProgress.memoizedProps = null;
workInProgress.memoizedState = null;
workInProgress.updateQueue = null;
@ -406,6 +393,8 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) {
workInProgress.lanes = current.lanes;
workInProgress.child = current.child;
workInProgress.subtreeTag = current.subtreeTag;
workInProgress.deletions = null;
workInProgress.memoizedProps = current.memoizedProps;
workInProgress.memoizedState = current.memoizedState;
workInProgress.updateQueue = current.updateQueue;
@ -830,9 +819,6 @@ export function assignFiberPropertiesInDEV(
target.effectTag = source.effectTag;
target.subtreeTag = source.subtreeTag;
target.deletions = source.deletions;
target.nextEffect = source.nextEffect;
target.firstEffect = source.firstEffect;
target.lastEffect = source.lastEffect;
target.lanes = source.lanes;
target.childLanes = source.childLanes;
target.alternate = source.alternate;

View File

@ -2063,8 +2063,6 @@ function updateSuspensePrimaryChildren(
primaryChildFragment.sibling = null;
if (currentFallbackChildFragment !== null) {
// Delete the fallback child fragment
currentFallbackChildFragment.nextEffect = null;
workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment;
const deletions = workInProgress.deletions;
if (deletions === null) {
workInProgress.deletions = [currentFallbackChildFragment];
@ -2129,21 +2127,8 @@ function updateSuspenseFallbackChildren(
// The fallback fiber was added as a deletion effect during the first pass.
// However, since we're going to remain on the fallback, we no longer want
// to delete it. So we need to remove it from the list. Deletions are stored
// on the same list as effects. We want to keep the effects from the primary
// tree. So we copy the primary child fragment's effect list, which does not
// include the fallback deletion effect.
const progressedLastEffect = primaryChildFragment.lastEffect;
if (progressedLastEffect !== null) {
workInProgress.firstEffect = primaryChildFragment.firstEffect;
workInProgress.lastEffect = progressedLastEffect;
progressedLastEffect.nextEffect = null;
// to delete it.
workInProgress.deletions = null;
} else {
// TODO: Reset this somewhere else? Lol legacy mode is so weird.
workInProgress.firstEffect = workInProgress.lastEffect = null;
workInProgress.deletions = null;
}
} else {
primaryChildFragment = createWorkInProgressOffscreenFiber(
currentPrimaryChildFragment,
@ -2633,7 +2618,6 @@ function initSuspenseListRenderState(
tail: null | Fiber,
lastContentRow: null | Fiber,
tailMode: SuspenseListTailMode,
lastEffectBeforeRendering: null | Fiber,
): void {
const renderState: null | SuspenseListRenderState =
workInProgress.memoizedState;
@ -2645,7 +2629,6 @@ function initSuspenseListRenderState(
last: lastContentRow,
tail: tail,
tailMode: tailMode,
lastEffect: lastEffectBeforeRendering,
}: SuspenseListRenderState);
} else {
// We can reuse the existing object from previous renders.
@ -2655,7 +2638,6 @@ function initSuspenseListRenderState(
renderState.last = lastContentRow;
renderState.tail = tail;
renderState.tailMode = tailMode;
renderState.lastEffect = lastEffectBeforeRendering;
}
}
@ -2737,7 +2719,6 @@ function updateSuspenseListComponent(
tail,
lastContentRow,
tailMode,
workInProgress.lastEffect,
);
break;
}
@ -2769,7 +2750,6 @@ function updateSuspenseListComponent(
tail,
null, // last
tailMode,
workInProgress.lastEffect,
);
break;
}
@ -2780,7 +2760,6 @@ function updateSuspenseListComponent(
null, // tail
null, // last
undefined,
workInProgress.lastEffect,
);
break;
}
@ -3040,13 +3019,6 @@ function remountFiber(
// Delete the old fiber and place the new one.
// Since the old fiber is disconnected, we have to schedule it manually.
const last = returnFiber.lastEffect;
if (last !== null) {
last.nextEffect = current;
returnFiber.lastEffect = current;
} else {
returnFiber.firstEffect = returnFiber.lastEffect = current;
}
const deletions = returnFiber.deletions;
if (deletions === null) {
returnFiber.deletions = [current];
@ -3055,7 +3027,6 @@ function remountFiber(
} else {
deletions.push(current);
}
current.nextEffect = null;
newWorkInProgress.effectTag |= Placement;
@ -3256,7 +3227,6 @@ function beginWork(
// update in the past but didn't complete it.
renderState.rendering = null;
renderState.tail = null;
renderState.lastEffect = null;
}
pushSuspenseContext(workInProgress, suspenseStackCursor.current);

View File

@ -1049,18 +1049,8 @@ function completeWork(
// Rerender the whole list, but this time, we'll force fallbacks
// to stay in place.
// Reset the effect list before doing the second pass since that's now invalid.
if (renderState.lastEffect === null) {
workInProgress.firstEffect = null;
workInProgress.subtreeTag = NoEffect;
let child = workInProgress.child;
while (child !== null) {
child.deletions = null;
child = child.sibling;
}
}
workInProgress.lastEffect = renderState.lastEffect;
// Reset the child fibers to their original state.
workInProgress.subtreeTag = NoEffect;
resetChildFibers(workInProgress, renderLanes);
// Set up the Suspense Context to force suspense and immediately
@ -1128,15 +1118,6 @@ function completeWork(
!renderedTail.alternate &&
!getIsHydrating() // We don't cut it if we're hydrating.
) {
// We need to delete the row we just rendered.
// Reset the effect list to what it was before we rendered this
// child. The nested children have already appended themselves.
const lastEffect = (workInProgress.lastEffect =
renderState.lastEffect);
// Remove any effects that were appended after this point.
if (lastEffect !== null) {
lastEffect.nextEffect = null;
}
// We're done.
return null;
}
@ -1192,7 +1173,6 @@ function completeWork(
const next = renderState.tail;
renderState.rendering = next;
renderState.tail = next.sibling;
renderState.lastEffect = workInProgress.lastEffect;
renderState.renderingStartTime = now();
next.sibling = null;

View File

@ -133,18 +133,6 @@ function deleteHydratableInstance(
} else {
deletions.push(childToDelete);
}
// This might seem like it belongs on progressedFirstDeletion. However,
// these children are not part of the reconciliation list of children.
// Even if we abort and rereconcile the children, that will try to hydrate
// again and the nodes are still in the host tree so these will be
// recreated.
if (returnFiber.lastEffect !== null) {
returnFiber.lastEffect.nextEffect = childToDelete;
returnFiber.lastEffect = childToDelete;
} else {
returnFiber.firstEffect = returnFiber.lastEffect = childToDelete;
}
}
function insertNonHydratedInstance(returnFiber: Fiber, fiber: Fiber) {

View File

@ -49,9 +49,6 @@ export type SuspenseListRenderState = {|
tail: null | Fiber,
// Tail insertions setting.
tailMode: SuspenseListTailMode,
// Last Effect before we rendered the "rendering" item.
// Used to remove new effects added by the rendered item.
lastEffect: null | Fiber,
|};
export function shouldCaptureSuspense(

View File

@ -185,8 +185,6 @@ function throwException(
) {
// The source fiber did not complete.
sourceFiber.effectTag |= Incomplete;
// Its effect list is no longer valid.
sourceFiber.firstEffect = sourceFiber.lastEffect = null;
if (
value !== null &&

View File

@ -122,7 +122,6 @@ import {
import {LegacyRoot} from './ReactRootTags';
import {
NoEffect,
PerformedWork,
Placement,
Update,
PlacementAndUpdate,
@ -1807,45 +1806,6 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
}
resetChildLanes(completedWork);
if (
returnFiber !== null &&
// Do not append effects to parents if a sibling failed to complete
(returnFiber.effectTag & Incomplete) === NoEffect
) {
// Append all the effects of the subtree and this fiber onto the effect
// list of the parent. The completion order of the children affects the
// side-effect order.
if (returnFiber.firstEffect === null) {
returnFiber.firstEffect = completedWork.firstEffect;
}
if (completedWork.lastEffect !== null) {
if (returnFiber.lastEffect !== null) {
returnFiber.lastEffect.nextEffect = completedWork.firstEffect;
}
returnFiber.lastEffect = completedWork.lastEffect;
}
// If this fiber had side-effects, we append it AFTER the children's
// side-effects. We can perform certain side-effects earlier if needed,
// by doing multiple passes over the effect list. We don't want to
// schedule our own side-effect on our own list because if end up
// reusing children we'll schedule this effect onto itself since we're
// at the end.
const effectTag = completedWork.effectTag;
// Skip both NoWork and PerformedWork tags when creating the effect
// list. PerformedWork effect is read by React DevTools but shouldn't be
// committed.
if (effectTag > PerformedWork) {
if (returnFiber.lastEffect !== null) {
returnFiber.lastEffect.nextEffect = completedWork;
} else {
returnFiber.firstEffect = completedWork;
}
returnFiber.lastEffect = completedWork;
}
}
} else {
// This fiber did not complete because something threw. Pop values off
// the stack without entering the complete phase. If this is a boundary,
@ -1882,8 +1842,7 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
}
if (returnFiber !== null) {
// Mark the parent fiber as incomplete and clear its effect list.
returnFiber.firstEffect = returnFiber.lastEffect = null;
// Mark the parent fiber as incomplete
returnFiber.effectTag |= Incomplete;
returnFiber.subtreeTag = NoSubtreeTag;
returnFiber.deletions = null;
@ -2161,25 +2120,24 @@ function commitRootImpl(root, renderPriorityLevel) {
// times out.
}
// Get the list of effects.
let firstEffect;
if (finishedWork.effectTag > PerformedWork) {
// A fiber's effect list consists only of its children, not itself. So if
// the root has an effect, we need to add it to the end of the list. The
// resulting list is the set that would belong to the root's parent, if it
// had one; that is, all the effects in the tree including the root.
if (finishedWork.lastEffect !== null) {
finishedWork.lastEffect.nextEffect = finishedWork;
firstEffect = finishedWork.firstEffect;
} else {
firstEffect = finishedWork;
}
} else {
// There is no effect on the root.
firstEffect = finishedWork.firstEffect;
}
// Check if there are any effects in the whole tree.
// TODO: This is left over from the effect list implementation, where we had
// to check for the existence of `firstEffect` to satsify Flow. I think the
// only other reason this optimization exists is because it affects profiling.
// Reconsider whether this is necessary.
const subtreeHasEffects =
(finishedWork.subtreeTag &
(BeforeMutationSubtreeTag |
MutationSubtreeTag |
LayoutSubtreeTag |
PassiveSubtreeTag)) !==
NoSubtreeTag;
const rootHasEffect =
(finishedWork.effectTag &
(BeforeMutationMask | MutationMask | LayoutMask | PassiveMask)) !==
NoEffect;
if (firstEffect !== null) {
if (subtreeHasEffects || rootHasEffect) {
let previousLanePriority;
if (decoupleUpdatePriorityFromScheduler) {
previousLanePriority = getCurrentUpdateLanePriority();
@ -4013,8 +3971,6 @@ function detachFiberAfterEffects(fiber: Fiber): void {
fiber.child = null;
fiber.deletions = null;
fiber.dependencies = null;
fiber.firstEffect = null;
fiber.lastEffect = null;
fiber.memoizedProps = null;
fiber.memoizedState = null;
fiber.pendingProps = null;

View File

@ -451,8 +451,6 @@ describe('SchedulingProfiler', () => {
ReactTestRenderer.create(<Example />, {unstable_isConcurrent: true});
});
gate(({old}) => {
if (old) {
expect(marks).toEqual([
`--react-init-${ReactVersion}`,
'--schedule-render-512',
@ -470,28 +468,6 @@ describe('SchedulingProfiler', () => {
'--commit-start-1024',
'--commit-stop',
]);
} else {
expect(marks).toEqual([
`--react-init-${ReactVersion}`,
'--schedule-render-512',
'--render-start-512',
'--render-stop',
'--commit-start-512',
'--layout-effects-start-512',
'--layout-effects-stop',
'--commit-stop',
'--passive-effects-start-512',
'--schedule-state-update-1024-Example',
'--passive-effects-stop',
'--render-start-1024',
'--render-stop',
'--commit-start-1024',
'--layout-effects-start-1024',
'--layout-effects-stop',
'--commit-stop',
]);
}
});
});
// @gate enableSchedulingProfiler