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.
Andrew Clark committed
Aug 21, 2020 at 20:42 UTC
d2e914ab4eb265bec82c01a083f608208f50d833
9 files changed
+42
-206
packages/react-reconciler/src/ReactChildFiber.new.js
-15
@@ -277,28 +277,13 @@ function ChildReconciler(shouldTrackSideEffects) {
277
// Noop.
278
return;
279
}
280
- // Deletions are added in reversed order so we add it to the front.
281
- // At this point, the return fiber's effect list is empty except for
282
- // deletions, so we can just append the deletion to the list. The remaining
283
- // effects aren't added until the complete phase. Once we implement
284
- // resuming, this may not be true.
285
- // TODO (effects) Get rid of effects list update here.
286
- const last = returnFiber.lastEffect;
287
- if (last !== null) {
288
- last.nextEffect = childToDelete;
289
- returnFiber.lastEffect = childToDelete;
290
- } else {
291
- returnFiber.firstEffect = returnFiber.lastEffect = childToDelete;
292
- }
280
const deletions = returnFiber.deletions;
281
if (deletions === null) {
282
returnFiber.deletions = [childToDelete];
296
- // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
283
returnFiber.effectTag |= Deletion;
284
} else {
285
deletions.push(childToDelete);
286
}
301
- childToDelete.nextEffect = null;
287
}
288
289
function deleteRemainingChildren(
packages/react-reconciler/src/ReactFiber.new.js
+3
-17
@@ -147,10 +147,6 @@ function FiberNode(
147
this.effectTag = NoEffect;
148
this.subtreeTag = NoSubtreeEffect;
149
this.deletions = null;
150
- this.nextEffect = null;
151
-
152
- this.firstEffect = null;
153
- this.lastEffect = null;
150
151
this.lanes = NoLanes;
152
this.childLanes = NoLanes;
@@ -291,11 +287,6 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber {
287
workInProgress.subtreeTag = NoSubtreeEffect;
288
workInProgress.deletions = null;
289
294
- // The effect list is no longer valid.
295
- workInProgress.nextEffect = null;
296
- workInProgress.firstEffect = null;
297
- workInProgress.lastEffect = null;
298
-
290
if (enableProfilerTimer) {
291
// We intentionally reset, rather than copy, actualDuration & actualStartTime.
292
// This prevents time from endlessly accumulating in new commits.
@@ -374,11 +365,6 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) {
365
// that child fiber is setting, not the reconciliation.
366
workInProgress.effectTag &= Placement;
367
377
- // The effect list is no longer valid.
378
- workInProgress.nextEffect = null;
379
- workInProgress.firstEffect = null;
380
- workInProgress.lastEffect = null;
381
-
368
const current = workInProgress.alternate;
369
if (current === null) {
370
// Reset to createFiber's initial values.
@@ -386,6 +372,7 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) {
372
workInProgress.lanes = renderLanes;
373
374
workInProgress.child = null;
375
+ workInProgress.subtreeTag = NoSubtreeEffect;
376
workInProgress.memoizedProps = null;
377
workInProgress.memoizedState = null;
378
workInProgress.updateQueue = null;
@@ -406,6 +393,8 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) {
393
workInProgress.lanes = current.lanes;
394
395
workInProgress.child = current.child;
396
+ workInProgress.subtreeTag = current.subtreeTag;
397
+ workInProgress.deletions = null;
398
workInProgress.memoizedProps = current.memoizedProps;
399
workInProgress.memoizedState = current.memoizedState;
400
workInProgress.updateQueue = current.updateQueue;
@@ -830,9 +819,6 @@ export function assignFiberPropertiesInDEV(
819
target.effectTag = source.effectTag;
820
target.subtreeTag = source.subtreeTag;
821
target.deletions = source.deletions;
833
- target.nextEffect = source.nextEffect;
834
- target.firstEffect = source.firstEffect;
835
- target.lastEffect = source.lastEffect;
822
target.lanes = source.lanes;
823
target.childLanes = source.childLanes;
824
target.alternate = source.alternate;
packages/react-reconciler/src/ReactFiberBeginWork.new.js
+2
-32
@@ -2063,8 +2063,6 @@ function updateSuspensePrimaryChildren(
2063
primaryChildFragment.sibling = null;
2064
if (currentFallbackChildFragment !== null) {
2065
// Delete the fallback child fragment
2066
- currentFallbackChildFragment.nextEffect = null;
2067
- workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment;
2066
const deletions = workInProgress.deletions;
2067
if (deletions === null) {
2068
workInProgress.deletions = [currentFallbackChildFragment];
@@ -2129,21 +2127,8 @@ function updateSuspenseFallbackChildren(
2127
2128
// The fallback fiber was added as a deletion effect during the first pass.
2129
// However, since we're going to remain on the fallback, we no longer want
2132
- // to delete it. So we need to remove it from the list. Deletions are stored
2133
- // on the same list as effects. We want to keep the effects from the primary
2134
- // tree. So we copy the primary child fragment's effect list, which does not
2135
- // include the fallback deletion effect.
2136
- const progressedLastEffect = primaryChildFragment.lastEffect;
2137
- if (progressedLastEffect !== null) {
2138
- workInProgress.firstEffect = primaryChildFragment.firstEffect;
2139
- workInProgress.lastEffect = progressedLastEffect;
2140
- progressedLastEffect.nextEffect = null;
2141
- workInProgress.deletions = null;
2142
- } else {
2143
- // TODO: Reset this somewhere else? Lol legacy mode is so weird.
2144
- workInProgress.firstEffect = workInProgress.lastEffect = null;
2145
- workInProgress.deletions = null;
2146
- }
2130
+ // to delete it.
2131
+ workInProgress.deletions = null;
2132
} else {
2133
primaryChildFragment = createWorkInProgressOffscreenFiber(
2134
currentPrimaryChildFragment,
@@ -2633,7 +2618,6 @@ function initSuspenseListRenderState(
2618
tail: null | Fiber,
2619
lastContentRow: null | Fiber,
2620
tailMode: SuspenseListTailMode,
2636
- lastEffectBeforeRendering: null | Fiber,
2621
): void {
2622
const renderState: null | SuspenseListRenderState =
2623
workInProgress.memoizedState;
@@ -2645,7 +2629,6 @@ function initSuspenseListRenderState(
2629
last: lastContentRow,
2630
tail: tail,
2631
tailMode: tailMode,
2648
- lastEffect: lastEffectBeforeRendering,
2632
}: SuspenseListRenderState);
2633
} else {
2634
// We can reuse the existing object from previous renders.
@@ -2655,7 +2638,6 @@ function initSuspenseListRenderState(
2638
renderState.last = lastContentRow;
2639
renderState.tail = tail;
2640
renderState.tailMode = tailMode;
2658
- renderState.lastEffect = lastEffectBeforeRendering;
2641
}
2642
}
2643
@@ -2737,7 +2719,6 @@ function updateSuspenseListComponent(
2719
tail,
2720
lastContentRow,
2721
tailMode,
2740
- workInProgress.lastEffect,
2722
);
2723
break;
2724
}
@@ -2769,7 +2750,6 @@ function updateSuspenseListComponent(
2750
tail,
2751
null, // last
2752
tailMode,
2772
- workInProgress.lastEffect,
2753
);
2754
break;
2755
}
@@ -2780,7 +2760,6 @@ function updateSuspenseListComponent(
2760
null, // tail
2761
null, // last
2762
undefined,
2783
- workInProgress.lastEffect,
2763
);
2764
break;
2765
}
@@ -3040,13 +3019,6 @@ function remountFiber(
3019
3020
// Delete the old fiber and place the new one.
3021
// Since the old fiber is disconnected, we have to schedule it manually.
3043
- const last = returnFiber.lastEffect;
3044
- if (last !== null) {
3045
- last.nextEffect = current;
3046
- returnFiber.lastEffect = current;
3047
- } else {
3048
- returnFiber.firstEffect = returnFiber.lastEffect = current;
3049
- }
3022
const deletions = returnFiber.deletions;
3023
if (deletions === null) {
3024
returnFiber.deletions = [current];
@@ -3055,7 +3027,6 @@ function remountFiber(
3027
} else {
3028
deletions.push(current);
3029
}
3058
- current.nextEffect = null;
3030
3031
newWorkInProgress.effectTag |= Placement;
3032
@@ -3256,7 +3227,6 @@ function beginWork(
3227
// update in the past but didn't complete it.
3228
renderState.rendering = null;
3229
renderState.tail = null;
3259
- renderState.lastEffect = null;
3230
}
3231
pushSuspenseContext(workInProgress, suspenseStackCursor.current);
3232
packages/react-reconciler/src/ReactFiberCompleteWork.new.js
+1
-21
@@ -1049,18 +1049,8 @@ function completeWork(
1049
1050
// Rerender the whole list, but this time, we'll force fallbacks
1051
// to stay in place.
1052
- // Reset the effect list before doing the second pass since that's now invalid.
1053
- if (renderState.lastEffect === null) {
1054
- workInProgress.firstEffect = null;
1055
- workInProgress.subtreeTag = NoEffect;
1056
- let child = workInProgress.child;
1057
- while (child !== null) {
1058
- child.deletions = null;
1059
- child = child.sibling;
1060
- }
1061
- }
1062
- workInProgress.lastEffect = renderState.lastEffect;
1052
// Reset the child fibers to their original state.
1053
+ workInProgress.subtreeTag = NoEffect;
1054
resetChildFibers(workInProgress, renderLanes);
1055
1056
// Set up the Suspense Context to force suspense and immediately
@@ -1128,15 +1118,6 @@ function completeWork(
1118
!renderedTail.alternate &&
1119
!getIsHydrating() // We don't cut it if we're hydrating.
1120
) {
1131
- // We need to delete the row we just rendered.
1132
- // Reset the effect list to what it was before we rendered this
1133
- // child. The nested children have already appended themselves.
1134
- const lastEffect = (workInProgress.lastEffect =
1135
- renderState.lastEffect);
1136
- // Remove any effects that were appended after this point.
1137
- if (lastEffect !== null) {
1138
- lastEffect.nextEffect = null;
1139
- }
1121
// We're done.
1122
return null;
1123
}
@@ -1192,7 +1173,6 @@ function completeWork(
1173
const next = renderState.tail;
1174
renderState.rendering = next;
1175
renderState.tail = next.sibling;
1195
- renderState.lastEffect = workInProgress.lastEffect;
1176
renderState.renderingStartTime = now();
1177
next.sibling = null;
1178
packages/react-reconciler/src/ReactFiberHydrationContext.new.js
-12
@@ -133,18 +133,6 @@ function deleteHydratableInstance(
133
} else {
134
deletions.push(childToDelete);
135
}
136
-
137
- // This might seem like it belongs on progressedFirstDeletion. However,
138
- // these children are not part of the reconciliation list of children.
139
- // Even if we abort and rereconcile the children, that will try to hydrate
140
- // again and the nodes are still in the host tree so these will be
141
- // recreated.
142
- if (returnFiber.lastEffect !== null) {
143
- returnFiber.lastEffect.nextEffect = childToDelete;
144
- returnFiber.lastEffect = childToDelete;
145
- } else {
146
- returnFiber.firstEffect = returnFiber.lastEffect = childToDelete;
147
- }
136
}
137
138
function insertNonHydratedInstance(returnFiber: Fiber, fiber: Fiber) {
packages/react-reconciler/src/ReactFiberSuspenseComponent.new.js
-3
@@ -49,9 +49,6 @@ export type SuspenseListRenderState = {|
49
tail: null | Fiber,
50
// Tail insertions setting.
51
tailMode: SuspenseListTailMode,
52
- // Last Effect before we rendered the "rendering" item.
53
- // Used to remove new effects added by the rendered item.
54
- lastEffect: null | Fiber,
52
|};
53
54
export function shouldCaptureSuspense(
packages/react-reconciler/src/ReactFiberThrow.new.js
-2
@@ -185,8 +185,6 @@ function throwException(
185
) {
186
// The source fiber did not complete.
187
sourceFiber.effectTag |= Incomplete;
188
- // Its effect list is no longer valid.
189
- sourceFiber.firstEffect = sourceFiber.lastEffect = null;
188
189
if (
190
value !== null &&
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+19
-63
@@ -122,7 +122,6 @@ import {
122
import {LegacyRoot} from './ReactRootTags';
123
import {
124
NoEffect,
125
- PerformedWork,
125
Placement,
126
Update,
127
PlacementAndUpdate,
@@ -1807,45 +1806,6 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
1806
}
1807
1808
resetChildLanes(completedWork);
1810
-
1811
- if (
1812
- returnFiber !== null &&
1813
- // Do not append effects to parents if a sibling failed to complete
1814
- (returnFiber.effectTag & Incomplete) === NoEffect
1815
- ) {
1816
- // Append all the effects of the subtree and this fiber onto the effect
1817
- // list of the parent. The completion order of the children affects the
1818
- // side-effect order.
1819
- if (returnFiber.firstEffect === null) {
1820
- returnFiber.firstEffect = completedWork.firstEffect;
1821
- }
1822
- if (completedWork.lastEffect !== null) {
1823
- if (returnFiber.lastEffect !== null) {
1824
- returnFiber.lastEffect.nextEffect = completedWork.firstEffect;
1825
- }
1826
- returnFiber.lastEffect = completedWork.lastEffect;
1827
- }
1828
-
1829
- // If this fiber had side-effects, we append it AFTER the children's
1830
- // side-effects. We can perform certain side-effects earlier if needed,
1831
- // by doing multiple passes over the effect list. We don't want to
1832
- // schedule our own side-effect on our own list because if end up
1833
- // reusing children we'll schedule this effect onto itself since we're
1834
- // at the end.
1835
- const effectTag = completedWork.effectTag;
1836
-
1837
- // Skip both NoWork and PerformedWork tags when creating the effect
1838
- // list. PerformedWork effect is read by React DevTools but shouldn't be
1839
- // committed.
1840
- if (effectTag > PerformedWork) {
1841
- if (returnFiber.lastEffect !== null) {
1842
- returnFiber.lastEffect.nextEffect = completedWork;
1843
- } else {
1844
- returnFiber.firstEffect = completedWork;
1845
- }
1846
- returnFiber.lastEffect = completedWork;
1847
- }
1848
- }
1809
} else {
1810
// This fiber did not complete because something threw. Pop values off
1811
// the stack without entering the complete phase. If this is a boundary,
@@ -1882,8 +1842,7 @@ function completeUnitOfWork(unitOfWork: Fiber): void {
1842
}
1843
1844
if (returnFiber !== null) {
1885
- // Mark the parent fiber as incomplete and clear its effect list.
1886
- returnFiber.firstEffect = returnFiber.lastEffect = null;
1845
+ // Mark the parent fiber as incomplete
1846
returnFiber.effectTag |= Incomplete;
1847
returnFiber.subtreeTag = NoSubtreeTag;
1848
returnFiber.deletions = null;
@@ -2161,25 +2120,24 @@ function commitRootImpl(root, renderPriorityLevel) {
2120
// times out.
2121
}
2122
2164
- // Get the list of effects.
2165
- let firstEffect;
2166
- if (finishedWork.effectTag > PerformedWork) {
2167
- // A fiber's effect list consists only of its children, not itself. So if
2168
- // the root has an effect, we need to add it to the end of the list. The
2169
- // resulting list is the set that would belong to the root's parent, if it
2170
- // had one; that is, all the effects in the tree including the root.
2171
- if (finishedWork.lastEffect !== null) {
2172
- finishedWork.lastEffect.nextEffect = finishedWork;
2173
- firstEffect = finishedWork.firstEffect;
2174
- } else {
2175
- firstEffect = finishedWork;
2176
- }
2177
- } else {
2178
- // There is no effect on the root.
2179
- firstEffect = finishedWork.firstEffect;
2180
- }
2181
-
2182
- if (firstEffect !== null) {
2123
+ // Check if there are any effects in the whole tree.
2124
+ // TODO: This is left over from the effect list implementation, where we had
2125
+ // to check for the existence of `firstEffect` to satsify Flow. I think the
2126
+ // only other reason this optimization exists is because it affects profiling.
2127
+ // Reconsider whether this is necessary.
2128
+ const subtreeHasEffects =
2129
+ (finishedWork.subtreeTag &
2130
+ (BeforeMutationSubtreeTag |
2131
+ MutationSubtreeTag |
2132
+ LayoutSubtreeTag |
2133
+ PassiveSubtreeTag)) !==
2134
+ NoSubtreeTag;
2135
+ const rootHasEffect =
2136
+ (finishedWork.effectTag &
2137
+ (BeforeMutationMask | MutationMask | LayoutMask | PassiveMask)) !==
2138
+ NoEffect;
2139
+
2140
+ if (subtreeHasEffects || rootHasEffect) {
2141
let previousLanePriority;
2142
if (decoupleUpdatePriorityFromScheduler) {
2143
previousLanePriority = getCurrentUpdateLanePriority();
@@ -4013,8 +3971,6 @@ function detachFiberAfterEffects(fiber: Fiber): void {
3971
fiber.child = null;
3972
fiber.deletions = null;
3973
fiber.dependencies = null;
4016
- fiber.firstEffect = null;
4017
- fiber.lastEffect = null;
3974
fiber.memoizedProps = null;
3975
fiber.memoizedState = null;
3976
fiber.pendingProps = null;
packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js
+17
-41
@@ -451,47 +451,23 @@ describe('SchedulingProfiler', () => {
451
ReactTestRenderer.create(<Example />, {unstable_isConcurrent: true});
452
});
453
454
- gate(({old}) => {
455
- if (old) {
456
- expect(marks).toEqual([
457
- `--react-init-${ReactVersion}`,
458
- '--schedule-render-512',
459
- '--render-start-512',
460
- '--render-stop',
461
- '--commit-start-512',
462
- '--layout-effects-start-512',
463
- '--layout-effects-stop',
464
- '--commit-stop',
465
- '--passive-effects-start-512',
466
- '--schedule-state-update-1024-Example',
467
- '--passive-effects-stop',
468
- '--render-start-1024',
469
- '--render-stop',
470
- '--commit-start-1024',
471
- '--commit-stop',
472
- ]);
473
- } else {
474
- expect(marks).toEqual([
475
- `--react-init-${ReactVersion}`,
476
- '--schedule-render-512',
477
- '--render-start-512',
478
- '--render-stop',
479
- '--commit-start-512',
480
- '--layout-effects-start-512',
481
- '--layout-effects-stop',
482
- '--commit-stop',
483
- '--passive-effects-start-512',
484
- '--schedule-state-update-1024-Example',
485
- '--passive-effects-stop',
486
- '--render-start-1024',
487
- '--render-stop',
488
- '--commit-start-1024',
489
- '--layout-effects-start-1024',
490
- '--layout-effects-stop',
491
- '--commit-stop',
492
- ]);
493
- }
494
- });
454
+ expect(marks).toEqual([
455
+ `--react-init-${ReactVersion}`,
456
+ '--schedule-render-512',
457
+ '--render-start-512',
458
+ '--render-stop',
459
+ '--commit-start-512',
460
+ '--layout-effects-start-512',
461
+ '--layout-effects-stop',
462
+ '--commit-stop',
463
+ '--passive-effects-start-512',
464
+ '--schedule-state-update-1024-Example',
465
+ '--passive-effects-stop',
466
+ '--render-start-1024',
467
+ '--render-stop',
468
+ '--commit-start-1024',
469
+ '--commit-stop',
470
+ ]);
471
});
472
473
// @gate enableSchedulingProfiler