Skip to content

Commit

Permalink
Reset new fork to old fork (#20254)
Browse files Browse the repository at this point in the history
* Fix typo

This typo was fixed in the new fork but not the old.

* Reset new fork to old fork

Something in the new fork is causing a topline metrics regression. We're
not sure what it is, so we're going to split it into steps and bisect.

As a first step, this resets the new fork back to the contents of the
old fork. We will land this to confirm that the fork infra itself is
not causing a regression.

* Fix tests: Add `dfsEffectsRefactor` flag

Some of the tests that gated on the effects refactor used the `new`
flag. In order to bisect, we'll need to decompose the new fork changes
into multiple steps.

So I added a hardcoded test flag called `dfsEffectsRefactor` and set it
to false. Will turn back on when we switch back to traversing the
finished tree using DFS and `subtreeTag`.
  • Loading branch information
acdlite authored Nov 13, 2020
1 parent 7548dd5 commit 765e89b
Show file tree
Hide file tree
Showing 19 changed files with 1,380 additions and 2,552 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ describe('ReactDOMServerPartialHydration', () => {
// This is a new node.
expect(span).not.toBe(span2);

if (gate(flags => flags.new)) {
if (gate(flags => flags.dfsEffectsRefactor)) {
// The effects list refactor causes this to be null because the Suspense Offscreen's child
// is null. However, since we can't hydrate Suspense in legacy this change in behavior is ok
expect(ref.current).toBe(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ beforeEach(() => {
});

// Don't feel too guilty if you have to delete this test.
// @gate dfsEffectsRefactor
// @gate new
// @gate __DEV__
test('warns in DEV if return pointer is inconsistent', async () => {
Expand Down
19 changes: 13 additions & 6 deletions packages/react-reconciler/src/ReactChildFiber.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import type {Fiber} from './ReactInternalTypes';
import type {Lanes} from './ReactFiberLane';

import getComponentName from 'shared/getComponentName';
import {Deletion, Placement} from './ReactFiberFlags';
import {Placement, Deletion} from './ReactFiberFlags';
import {
getIteratorFn,
REACT_ELEMENT_TYPE,
Expand Down Expand Up @@ -256,13 +256,20 @@ function ChildReconciler(shouldTrackSideEffects) {
// Noop.
return;
}
const deletions = returnFiber.deletions;
if (deletions === null) {
returnFiber.deletions = [childToDelete];
returnFiber.flags |= Deletion;
// 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.
const last = returnFiber.lastEffect;
if (last !== null) {
last.nextEffect = childToDelete;
returnFiber.lastEffect = childToDelete;
} else {
deletions.push(childToDelete);
returnFiber.firstEffect = returnFiber.lastEffect = childToDelete;
}
childToDelete.nextEffect = null;
childToDelete.flags = Deletion;
}

function deleteRemainingChildren(
Expand Down
17 changes: 13 additions & 4 deletions packages/react-reconciler/src/ReactFiber.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
enableFundamentalAPI,
enableScopeAPI,
} from 'shared/ReactFeatureFlags';
import {NoFlags, Placement, StaticMask} from './ReactFiberFlags';
import {NoFlags, Placement} from './ReactFiberFlags';
import {ConcurrentRoot, BlockingRoot} from './ReactRootTags';
import {
IndeterminateComponent,
Expand Down Expand Up @@ -279,6 +279,13 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber {
workInProgress.type = current.type;

// We already have an alternate.
// Reset the effect tag.
workInProgress.flags = NoFlags;

// The effect list is no longer valid.
workInProgress.nextEffect = null;
workInProgress.firstEffect = null;
workInProgress.lastEffect = null;
workInProgress.subtreeFlags = NoFlags;
workInProgress.deletions = null;

Expand All @@ -292,9 +299,6 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber {
}
}

// Reset all effects except static ones.
// Static effects are not specific to a render.
workInProgress.flags = current.flags & StaticMask;
workInProgress.childLanes = current.childLanes;
workInProgress.lanes = current.lanes;

Expand Down Expand Up @@ -360,6 +364,11 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) {
// that child fiber is setting, not the reconciliation.
workInProgress.flags &= 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.
Expand Down
68 changes: 42 additions & 26 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ import {
Hydrating,
ContentReset,
DidCapture,
Update,
Ref,
Deletion,
ForceUpdateForLegacySuspense,
StaticMask,
} from './ReactFiberFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
Expand Down Expand Up @@ -671,6 +671,8 @@ function updateProfiler(
renderLanes: Lanes,
) {
if (enableProfilerTimer) {
workInProgress.flags |= Update;

// Reset effect durations for the next eventual effect phase.
// These are reset during render to allow the DevTools commit hook a chance to read them,
const stateNode = workInProgress.stateNode;
Expand Down Expand Up @@ -1077,9 +1079,6 @@ function updateHostComponent(
workInProgress.flags |= ContentReset;
}

// React DevTools reads this flag.
workInProgress.flags |= PerformedWork;

markRef(current, workInProgress);
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
return workInProgress.child;
Expand Down Expand Up @@ -2005,14 +2004,9 @@ function updateSuspensePrimaryChildren(
primaryChildFragment.sibling = null;
if (currentFallbackChildFragment !== null) {
// Delete the fallback child fragment
const deletions = workInProgress.deletions;
if (deletions === null) {
workInProgress.deletions = [currentFallbackChildFragment];
// TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
workInProgress.flags |= Deletion;
} else {
deletions.push(currentFallbackChildFragment);
}
currentFallbackChildFragment.nextEffect = null;
currentFallbackChildFragment.flags = Deletion;
workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment;
}

workInProgress.child = primaryChildFragment;
Expand Down Expand Up @@ -2069,19 +2063,24 @@ 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.
workInProgress.deletions = null;
// 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;
} else {
// TODO: Reset this somewhere else? Lol legacy mode is so weird.
workInProgress.firstEffect = workInProgress.lastEffect = null;
}
} else {
primaryChildFragment = createWorkInProgressOffscreenFiber(
currentPrimaryChildFragment,
primaryChildProps,
);

// Since we're reusing a current tree, we need to reuse the flags, too.
// (We don't do this in legacy mode, because in legacy mode we don't re-use
// the current tree; see previous branch.)
primaryChildFragment.subtreeFlags =
currentPrimaryChildFragment.subtreeFlags & StaticMask;
}
let fallbackChildFragment;
if (currentFallbackChildFragment !== null) {
Expand Down Expand Up @@ -2566,6 +2565,7 @@ function initSuspenseListRenderState(
tail: null | Fiber,
lastContentRow: null | Fiber,
tailMode: SuspenseListTailMode,
lastEffectBeforeRendering: null | Fiber,
): void {
const renderState: null | SuspenseListRenderState =
workInProgress.memoizedState;
Expand All @@ -2577,6 +2577,7 @@ function initSuspenseListRenderState(
last: lastContentRow,
tail: tail,
tailMode: tailMode,
lastEffect: lastEffectBeforeRendering,
}: SuspenseListRenderState);
} else {
// We can reuse the existing object from previous renders.
Expand All @@ -2586,6 +2587,7 @@ function initSuspenseListRenderState(
renderState.last = lastContentRow;
renderState.tail = tail;
renderState.tailMode = tailMode;
renderState.lastEffect = lastEffectBeforeRendering;
}
}

Expand Down Expand Up @@ -2667,6 +2669,7 @@ function updateSuspenseListComponent(
tail,
lastContentRow,
tailMode,
workInProgress.lastEffect,
);
break;
}
Expand Down Expand Up @@ -2698,6 +2701,7 @@ function updateSuspenseListComponent(
tail,
null, // last
tailMode,
workInProgress.lastEffect,
);
break;
}
Expand All @@ -2708,6 +2712,7 @@ function updateSuspenseListComponent(
null, // tail
null, // last
undefined,
workInProgress.lastEffect,
);
break;
}
Expand Down Expand Up @@ -2967,14 +2972,15 @@ function remountFiber(

// Delete the old fiber and place the new one.
// Since the old fiber is disconnected, we have to schedule it manually.
const deletions = returnFiber.deletions;
if (deletions === null) {
returnFiber.deletions = [current];
// TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
returnFiber.flags |= Deletion;
const last = returnFiber.lastEffect;
if (last !== null) {
last.nextEffect = current;
returnFiber.lastEffect = current;
} else {
deletions.push(current);
returnFiber.firstEffect = returnFiber.lastEffect = current;
}
current.nextEffect = null;
current.flags = Deletion;

newWorkInProgress.flags |= Placement;

Expand Down Expand Up @@ -3059,6 +3065,15 @@ function beginWork(
}
case Profiler:
if (enableProfilerTimer) {
// Profiler should only call onRender when one of its descendants actually rendered.
const hasChildWork = includesSomeLane(
renderLanes,
workInProgress.childLanes,
);
if (hasChildWork) {
workInProgress.flags |= Update;
}

// Reset effect durations for the next eventual effect phase.
// These are reset during render to allow the DevTools commit hook a chance to read them,
const stateNode = workInProgress.stateNode;
Expand Down Expand Up @@ -3165,6 +3180,7 @@ function beginWork(
// update in the past but didn't complete it.
renderState.rendering = null;
renderState.tail = null;
renderState.lastEffect = null;
}
pushSuspenseContext(workInProgress, suspenseStackCursor.current);

Expand Down
52 changes: 6 additions & 46 deletions packages/react-reconciler/src/ReactFiberClassComponent.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,13 @@ import type {Lanes} from './ReactFiberLane';
import type {UpdateQueue} from './ReactUpdateQueue.new';

import * as React from 'react';
import {Update, Snapshot, MountLayoutDev} from './ReactFiberFlags';
import {Update, Snapshot} from './ReactFiberFlags';
import {
debugRenderPhaseSideEffectsForStrictMode,
disableLegacyContext,
enableDebugTracing,
enableSchedulingProfiler,
warnAboutDeprecatedLifecycles,
enableDoubleInvokingEffects,
} from 'shared/ReactFeatureFlags';
import ReactStrictModeWarnings from './ReactStrictModeWarnings.new';
import {isMounted} from './ReactFiberTreeReflection';
Expand All @@ -30,13 +29,7 @@ import invariant from 'shared/invariant';
import {REACT_CONTEXT_TYPE, REACT_PROVIDER_TYPE} from 'shared/ReactSymbols';

import {resolveDefaultProps} from './ReactFiberLazyComponent.new';
import {
BlockingMode,
ConcurrentMode,
DebugTracingMode,
NoMode,
StrictMode,
} from './ReactTypeOfMode';
import {DebugTracingMode, StrictMode} from './ReactTypeOfMode';

import {
enqueueUpdate,
Expand Down Expand Up @@ -897,16 +890,7 @@ function mountClassInstance(
}

if (typeof instance.componentDidMount === 'function') {
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
) {
// Never double-invoke effects for legacy roots.
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
workInProgress.flags |= Update;
}
}

Expand Down Expand Up @@ -976,15 +960,7 @@ function resumeMountClassInstance(
// If an update was already in progress, we should schedule an Update
// effect even though we're bailing out, so that cWU/cDU are called.
if (typeof instance.componentDidMount === 'function') {
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
) {
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
workInProgress.flags |= Update;
}
return false;
}
Expand Down Expand Up @@ -1027,29 +1003,13 @@ function resumeMountClassInstance(
}
}
if (typeof instance.componentDidMount === 'function') {
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
) {
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
workInProgress.flags |= Update;
}
} else {
// If an update was already in progress, we should schedule an Update
// effect even though we're bailing out, so that cWU/cDU are called.
if (typeof instance.componentDidMount === 'function') {
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
) {
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
workInProgress.flags |= Update;
}

// If shouldComponentUpdate returned false, we should still update the
Expand Down
Loading

0 comments on commit 765e89b

Please sign in to comment.