Remove PassiveSubtreeTag optimization

Instead, always schedule a passive traversal for a subtree containing a deleted node. The overhead of doing this (during the passive phase) is small, and effects are so common that a cleanup in the subtree is likely. This saves some bytes.
facebook · Jul 29, 2020 · cba61f7 · cba61f7
1 parent 7d618ed
commit cba61f7
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 60 deletions.
diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js
@@ -15,16 +15,8 @@ import type {Fiber} from './ReactInternalTypes';
 import type {Lanes} from './ReactFiberLane';
 
 import getComponentName from 'shared/getComponentName';
-import {
- Deletion,
- NoEffect,
- PassiveMask,
- Placement,
-} from './ReactSideEffectTags';
-import {
- NoEffect as NoSubtreeTag,
- Passive as PassiveSubtreeTag,
-} from './ReactSubtreeTags';
+import {Deletion, Placement} from './ReactSideEffectTags';
+import {Passive as PassiveSubtreeTag} from './ReactSubtreeTags';
 import {
  getIteratorFn,
  REACT_ELEMENT_TYPE,
@@ -303,13 +295,11 @@ function ChildReconciler(shouldTrackSideEffects) {
  // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
  returnFiber.effectTag |= Deletion;
 
- // If we are deleting a subtree that contains a passive effect,
- // mark the parent so that we're sure to traverse after commit and run any unmount functions.
- const primaryEffectTag = childToDelete.effectTag & PassiveMask;
- const primarySubtreeTag = childToDelete.subtreeTag & PassiveSubtreeTag;
- if (primaryEffectTag !== NoEffect || primarySubtreeTag !== NoSubtreeTag) {
- returnFiber.subtreeTag |= PassiveSubtreeTag;
- }
+ // We are deleting a subtree that may contain a passive effect.
+ // Mark the parent so we traverse this path after commit and run any unmount functions.
+ // This may cause us to traverse unnecessarily in some cases, but effects are common,
+ // and the cost of over traversing is small (just the path to the deleted node).
+ returnFiber.subtreeTag |= PassiveSubtreeTag;
  } else {
  deletions.push(childToDelete);
  }

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
@@ -64,13 +64,9 @@ import {
  Ref,
  Deletion,
  ForceUpdateForLegacySuspense,
- PassiveMask,
  StaticMask,
 } from './ReactSideEffectTags';
-import {
- NoEffect as NoSubtreeTag,
- Passive as PassiveSubtreeTag,
-} from './ReactSubtreeTags';
+import {Passive as PassiveSubtreeTag} from './ReactSubtreeTags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import {
  debugRenderPhaseSideEffectsForStrictMode,
@@ -2079,15 +2075,11 @@ function updateSuspensePrimaryChildren(
  // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
  workInProgress.effectTag |= Deletion;
 
- // If we are deleting a subtree that contains a passive effect,
- // mark the parent so that we're sure to traverse after commit and run any unmount functions.
- const primaryEffectTag =
- currentFallbackChildFragment.effectTag & PassiveMask;
- const primarySubtreeTag =
- currentFallbackChildFragment.subtreeTag & PassiveSubtreeTag;
- if (primaryEffectTag !== NoEffect || primarySubtreeTag !== NoSubtreeTag) {
- workInProgress.subtreeTag |= PassiveSubtreeTag;
- }
+ // We are deleting a subtree that may contain a passive effect.
+ // Mark the parent so we traverse this path after commit and run any unmount functions.
+ // This may cause us to traverse unnecessarily in some cases, but effects are common,
+ // and the cost of over traversing is small (just the path to the deleted node).
+ workInProgress.subtreeTag |= PassiveSubtreeTag;
  } else {
  deletions.push(currentFallbackChildFragment);
  }
@@ -3073,13 +3065,11 @@ function remountFiber(
  // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
  returnFiber.effectTag |= Deletion;
 
- // If we are deleting a subtree that contains a passive effect,
- // mark the parent so that we're sure to traverse after commit and run any unmount functions.
- const primaryEffectTag = current.effectTag & PassiveMask;
- const primarySubtreeTag = current.subtreeTag & PassiveSubtreeTag;
- if (primaryEffectTag !== NoEffect || primarySubtreeTag !== NoSubtreeTag) {
- returnFiber.subtreeTag |= PassiveSubtreeTag;
- }
+ // We are deleting a subtree that may contain a passive effect.
+ // Mark the parent so we traverse this path after commit and run any unmount functions.
+ // This may cause us to traverse unnecessarily in some cases, but effects are common,
+ // and the cost of over traversing is small (just the path to the deleted node).
+ returnFiber.subtreeTag |= PassiveSubtreeTag;
  } else {
  deletions.push(current);
  }

diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js
@@ -24,17 +24,8 @@ import {
  HostRoot,
  SuspenseComponent,
 } from './ReactWorkTags';
-import {
- Deletion,
- Hydrating,
- NoEffect,
- PassiveMask,
- Placement,
-} from './ReactSideEffectTags';
-import {
- NoEffect as NoSubtreeTag,
- Passive as PassiveSubtreeTag,
-} from './ReactSubtreeTags';
+import {Deletion, Hydrating, Placement} from './ReactSideEffectTags';
+import {Passive as PassiveSubtreeTag} from './ReactSubtreeTags';
 import invariant from 'shared/invariant';
 
 import {
@@ -141,13 +132,11 @@ function deleteHydratableInstance(
  // TODO (effects) Rename this to better reflect its new usage (e.g. ChildDeletions)
  returnFiber.effectTag |= Deletion;
 
- // If we are deleting a subtree that contains a passive effect,
- // mark the parent so that we're sure to traverse after commit and run any unmount functions.
- const primaryEffectTag = childToDelete.effectTag & PassiveMask;
- const primarySubtreeTag = childToDelete.subtreeTag & PassiveSubtreeTag;
- if (primaryEffectTag !== NoEffect || primarySubtreeTag !== NoSubtreeTag) {
- returnFiber.subtreeTag |= PassiveSubtreeTag;
- }
+ // We are deleting a subtree that may contain a passive effect.
+ // Mark the parent so we traverse this path after commit and run any unmount functions.
+ // This may cause us to traverse unnecessarily in some cases, but effects are common,
+ // and the cost of over traversing is small (just the path to the deleted node).
+ returnFiber.subtreeTag |= PassiveSubtreeTag;
  } else {
  deletions.push(childToDelete);
  }

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
@@ -1454,7 +1454,7 @@ describe('ReactSuspense', () => {
  ]);
  });
 
- it('should call onInteractionScheduledWorkCompleted after suspending', done => {
+ it('should call onInteractionScheduledWorkCompleted after suspending', () => {
  const subscriber = {
  onInteractionScheduledWorkCompleted: jest.fn(),
  onInteractionTraced: jest.fn(),
@@ -1512,13 +1512,11 @@ describe('ReactSuspense', () => {
  jest.advanceTimersByTime(1000);
 
  expect(Scheduler).toHaveYielded(['Promise resolved [C]']);
- expect(Scheduler).toFlushExpired([
+ expect(Scheduler).toFlushAndYield([
  // Even though the promise for C was thrown three times, we should only
  // re-render once.
  'C',
  ]);
-
- done();
  });
 
  expect(