Skip to content

Commit

Permalink
Change passive effect flushing to use arrays instead of lists
Browse files Browse the repository at this point in the history 
This change offers a small advantage over the way we did things previous: it continues invoking destroy functions even after a previous one errored.
  • Loading branch information
Brian Vaughn committed Feb 11, 2020
1 parent 993e110 commit 674bfc4
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 102 deletions.
63 changes: 29 additions & 34 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ import {
captureCommitPhaseError,
resolveRetryThenable,
markCommitTimeOfFallback,
enqueuePendingPassiveEffectDestroyFn,
enqueuePendingPassiveHookEffectMount,
enqueuePendingPassiveHookEffectUnmount,
} from './ReactFiberWorkLoop';
import {
NoEffect as NoHookEffect,
Expand Down Expand Up @@ -396,49 +397,39 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
}
}

export function commitPassiveHookEffects(finishedWork: Fiber): void {
if ((finishedWork.effectTag & Passive) !== NoEffect) {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
// TODO (#17945) We should call all passive destroy functions (for all fibers)
// before calling any create functions. The current approach only serializes
// these for a single fiber.
commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork);
commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
break;
}
default:
break;
function schedulePassiveEffects(finishedWork: Fiber) {
if (deferPassiveEffectCleanupDuringUnmount) {
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;
let effect = firstEffect;
do {
const {next, tag} = effect;
if (
(tag & HookPassive) !== NoHookEffect &&
(tag & HookHasEffect) !== NoHookEffect
) {
enqueuePendingPassiveHookEffectUnmount(finishedWork, effect);
enqueuePendingPassiveHookEffectMount(finishedWork, effect);
}
effect = next;
} while (effect !== firstEffect);
}
}
}

export function commitPassiveHookUnmountEffects(finishedWork: Fiber): void {
export function commitPassiveHookEffects(finishedWork: Fiber): void {
if ((finishedWork.effectTag & Passive) !== NoEffect) {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
// TODO (#17945) We should call all passive destroy functions (for all fibers)
// before calling any create functions. The current approach only serializes
// these for a single fiber.
commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork);
break;
}
default:
break;
}
}
}

export function commitPassiveHookMountEffects(finishedWork: Fiber): void {
if ((finishedWork.effectTag & Passive) !== NoEffect) {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
break;
}
Expand All @@ -464,6 +455,10 @@ function commitLifeCycles(
// e.g. a destroy function in one component should never override a ref set
// by a create function in another component during the same commit.
commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork);

if (deferPassiveEffectCleanupDuringUnmount) {
schedulePassiveEffects(finishedWork);
}
return;
}
case ClassComponent: {
Expand Down Expand Up @@ -806,7 +801,7 @@ function commitUnmount(
const {destroy, tag} = effect;
if (destroy !== undefined) {
if ((tag & HookPassive) !== NoHookEffect) {
enqueuePendingPassiveEffectDestroyFn(destroy);
enqueuePendingPassiveHookEffectUnmount(current, effect);
} else {
safelyCallDestroy(current, destroy);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ export type Hook = {|
next: Hook | null,
|};

type Effect = {|
export type Effect = {|
tag: HookEffectTag,
create: () => (() => void) | void,
destroy: (() => void) | void,
Expand Down
132 changes: 65 additions & 67 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
import type {Interaction} from 'scheduler/src/Tracing';
import type {SuspenseConfig} from './ReactFiberSuspenseConfig';
import type {SuspenseState} from './ReactFiberSuspenseComponent';
import type {Effect as HookEffect} from './ReactFiberHooks';

import {
warnAboutDeprecatedLifecycles,
Expand Down Expand Up @@ -132,8 +133,6 @@ import {
commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber,
commitLifeCycles as commitLayoutEffectOnFiber,
commitPassiveHookEffects,
commitPassiveHookUnmountEffects,
commitPassiveHookMountEffects,
commitPlacement,
commitWork,
commitDeletion,
Expand Down Expand Up @@ -259,7 +258,8 @@ let rootDoesHavePassiveEffects: boolean = false;
let rootWithPendingPassiveEffects: FiberRoot | null = null;
let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];
let pendingPassiveHookEffectsMount: Array<HookEffect | Fiber> = [];
let pendingPassiveHookEffectsUnmount: Array<HookEffect | Fiber> = [];

let rootsWithPendingDiscreteUpdates: Map<
FiberRoot,
Expand Down Expand Up @@ -2170,11 +2170,28 @@ export function flushPassiveEffects() {
}
}

export function enqueuePendingPassiveEffectDestroyFn(
destroy: () => void,
export function enqueuePendingPassiveHookEffectMount(
fiber: Fiber,
effect: HookEffect,
): void {
if (deferPassiveEffectCleanupDuringUnmount) {
pendingPassiveHookEffectsMount.push(effect, fiber);
if (!rootDoesHavePassiveEffects) {
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
flushPassiveEffects();
return null;
});
}
}
}

export function enqueuePendingPassiveHookEffectUnmount(
fiber: Fiber,
effect: HookEffect,
): void {
if (deferPassiveEffectCleanupDuringUnmount) {
pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
pendingPassiveHookEffectsUnmount.push(effect, fiber);
if (!rootDoesHavePassiveEffects) {
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
Expand All @@ -2185,6 +2202,11 @@ export function enqueuePendingPassiveEffectDestroyFn(
}
}

function invokePassiveEffectCreate(effect: HookEffect): void {
const create = effect.create;
effect.destroy = create();
}

function flushPassiveEffectsImpl() {
if (rootWithPendingPassiveEffects === null) {
return false;
Expand All @@ -2203,18 +2225,6 @@ function flushPassiveEffectsImpl() {
const prevInteractions = pushInteractions(root);

if (deferPassiveEffectCleanupDuringUnmount) {
// Flush any pending passive effect destroy functions that belong to
// components that were unmounted during the most recent commit.
for (
let i = 0;
i < pendingUnmountedPassiveEffectDestroyFunctions.length;
i++
) {
const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i];
invokeGuardedCallback(null, destroy, null);
}
pendingUnmountedPassiveEffectDestroyFunctions.length = 0;

// It's important that ALL pending passive effect destroy functions are called
// before ANY passive effect create functions are called.
// Otherwise effects in sibling components might interfere with each other.
Expand All @@ -2223,70 +2233,58 @@ function flushPassiveEffectsImpl() {
// Layout effects have the same constraint.

// First pass: Destroy stale passive effects.
//
// Note: This currently assumes there are no passive effects on the root fiber
// because the root is not part of its own effect list.
// This could change in the future.
let effect = root.current.firstEffect;
while (effect !== null) {
if (__DEV__) {
setCurrentDebugFiberInDEV(effect);
invokeGuardedCallback(
null,
commitPassiveHookUnmountEffects,
null,
effect,
);
if (hasCaughtError()) {
invariant(effect !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(effect, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitPassiveHookUnmountEffects(effect);
} catch (error) {
invariant(effect !== null, 'Should be working on an effect.');
captureCommitPhaseError(effect, error);
let unmountEffects = pendingPassiveHookEffectsUnmount;
pendingPassiveHookEffectsUnmount = [];
for (let i = 0; i < unmountEffects.length; i += 2) {
const effect = ((unmountEffects[i]: any): HookEffect);
const fiber = ((unmountEffects[i + 1]: any): Fiber);
const destroy = effect.destroy;
effect.destroy = undefined;
if (typeof destroy === 'function') {
if (__DEV__) {
setCurrentDebugFiberInDEV(fiber);
invokeGuardedCallback(null, destroy, null);
if (hasCaughtError()) {
invariant(fiber !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(fiber, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
destroy();
} catch (error) {
invariant(fiber !== null, 'Should be working on an effect.');
captureCommitPhaseError(fiber, error);
}
}
}
effect = effect.nextEffect;
}

// Second pass: Create new passive effects.
//
// Note: This currently assumes there are no passive effects on the root fiber
// because the root is not part of its own effect list.
// This could change in the future.
effect = root.current.firstEffect;
while (effect !== null) {
let mountEffects = pendingPassiveHookEffectsMount;
pendingPassiveHookEffectsMount = [];
for (let i = 0; i < mountEffects.length; i += 2) {
const effect = ((mountEffects[i]: any): HookEffect);
const fiber = ((mountEffects[i + 1]: any): Fiber);
if (__DEV__) {
setCurrentDebugFiberInDEV(effect);
invokeGuardedCallback(
null,
commitPassiveHookMountEffects,
null,
effect,
);
setCurrentDebugFiberInDEV(fiber);
invokeGuardedCallback(null, invokePassiveEffectCreate, null, effect);
if (hasCaughtError()) {
invariant(effect !== null, 'Should be working on an effect.');
invariant(fiber !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(effect, error);
captureCommitPhaseError(fiber, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitPassiveHookMountEffects(effect);
const create = effect.create;
effect.destroy = create();
} catch (error) {
invariant(effect !== null, 'Should be working on an effect.');
captureCommitPhaseError(effect, error);
invariant(fiber !== null, 'Should be working on an effect.');
captureCommitPhaseError(fiber, error);
}
}
const nextNextEffect = effect.nextEffect;
// Remove nextEffect pointer to assist GC
effect.nextEffect = null;
effect = nextNextEffect;
}
} else {
// Note: This currently assumes there are no passive effects on the root fiber
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1753,6 +1753,7 @@ describe('ReactHooksWithNoopRenderer', () => {
// not block the subsequent create functions from being run.
expect(Scheduler).toHaveYielded([
'Oops!',
'Unmount B [0]',
'Mount A [1]',
'Mount B [1]',
]);
Expand Down

0 comments on commit 674bfc4

Please sign in to comment.