Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use Passive flag to schedule onPostCommit #19862

Merged
merged 1 commit into from
Sep 18, 2020
Merged

Use Passive flag to schedule onPostCommit #19862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import {
ContentReset,
DidCapture,
Update,
Passive,
Ref,
Deletion,
ForceUpdateForLegacySuspense,
Expand Down Expand Up @@ -674,7 +675,8 @@ function updateProfiler(
renderLanes: Lanes,
) {
if (enableProfilerTimer) {
workInProgress.flags |= Update;
// TODO: Only call onRender et al if subtree has effects
workInProgress.flags |= Update | Passive;

// 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,
Expand Down Expand Up @@ -3116,12 +3118,13 @@ function beginWork(
case Profiler:
if (enableProfilerTimer) {
// Profiler should only call onRender when one of its descendants actually rendered.
// TODO: Only call onRender et al if subtree has effects
const hasChildWork = includesSomeLane(
renderLanes,
workInProgress.childLanes,
);
if (hasChildWork) {
workInProgress.flags |= Update;
workInProgress.flags |= Passive | Update;
}

// Reset effect durations for the next eventual effect phase.
Expand Down
100 changes: 51 additions & 49 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,6 @@ import {
captureCommitPhaseError,
resolveRetryWakeable,
markCommitTimeOfFallback,
enqueuePendingPassiveProfilerEffect,
schedulePassiveEffectCallback,
} from './ReactFiberWorkLoop.new';
import {
Expand Down Expand Up @@ -507,57 +506,55 @@ function commitHookEffectListMount2(fiber: Fiber): void {
}
}

export function commitPassiveEffectDurations(
function commitProfilerPassiveEffect(
finishedRoot: FiberRoot,
finishedWork: Fiber,
): void {
if (enableProfilerTimer && enableProfilerCommitHooks) {
// Only Profilers with work in their subtree will have an Update effect scheduled.
if ((finishedWork.flags & Update) !== NoFlags) {
switch (finishedWork.tag) {
case Profiler: {
const {passiveEffectDuration} = finishedWork.stateNode;
const {id, onPostCommit} = finishedWork.memoizedProps;

// This value will still reflect the previous commit phase.
// It does not get reset until the start of the next commit phase.
const commitTime = getCommitTime();

if (typeof onPostCommit === 'function') {
if (enableSchedulerTracing) {
onPostCommit(
id,
finishedWork.alternate === null ? 'mount' : 'update',
passiveEffectDuration,
commitTime,
finishedRoot.memoizedInteractions,
);
} else {
onPostCommit(
id,
finishedWork.alternate === null ? 'mount' : 'update',
passiveEffectDuration,
commitTime,
);
}
switch (finishedWork.tag) {
case Profiler: {
const {passiveEffectDuration} = finishedWork.stateNode;
const {id, onPostCommit} = finishedWork.memoizedProps;

// This value will still reflect the previous commit phase.
// It does not get reset until the start of the next commit phase.
const commitTime = getCommitTime();

if (typeof onPostCommit === 'function') {
if (enableSchedulerTracing) {
onPostCommit(
id,
finishedWork.alternate === null ? 'mount' : 'update',
passiveEffectDuration,
commitTime,
finishedRoot.memoizedInteractions,
);
} else {
onPostCommit(
id,
finishedWork.alternate === null ? 'mount' : 'update',
passiveEffectDuration,
commitTime,
);
}
}

// Bubble times to the next nearest ancestor Profiler.
// After we process that Profiler, we'll bubble further up.
let parentFiber = finishedWork.return;
while (parentFiber !== null) {
if (parentFiber.tag === Profiler) {
const parentStateNode = parentFiber.stateNode;
parentStateNode.passiveEffectDuration += passiveEffectDuration;
break;
}
parentFiber = parentFiber.return;
// Bubble times to the next nearest ancestor Profiler.
// After we process that Profiler, we'll bubble further up.
// TODO: Use JS Stack instead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this comment mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roughly this (typed it out fast, might be a mistake)

let nearestProfiler = null;

function commitWork(finishedWork) {
  switch (finishedWork.tag) {
    case Profiler: {
      // Can push/pop values onto stack before entering/leaving a subtree, like
      // how we do in the render phase, except since this is a recursive
      // function we can use the regular JS stack instead of our fake
      // fiber stack.
      //
      // This is the code for Profiler. We'd do something similar for Offscreen.
      if (finishedWork.subtreeFlags & FlagForThisPhase) {
        const prevProfilerOnStack = nearestProfiler;
        nearestProfiler = finishedWork;

        try {
          let child = finishedWork.child;
          while (child !== null) {
            commitWork(child);
            child = child.sibling;
          }
        } finally {
          nearestProfiler = prevProfilerOnStack;
        }
      }

      commitProfilerEffect(finishedWork);
      break;
    }
    default: {
      // Normal path, no pushing or popping
      if (finishedWork.subtreeFlags & FlagForThisPhase) {
        let child = finishedWork.child;
        while (child !== null) {
          commitWork(child);
          child = child.sibling;
        }
      }

      switch (finishedWork.tag) {
        case FunctionComponent: {
          // Can reference `nearestProfiler` here
          commitFunctionComponent(finishedWork, nearestProfiler);
          break;
        }
        // ... and so on
      }
      break;
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to pick this TODO up if you haven't already started on it. Let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circling back on this.

It's possible that I'm misunderstanding what you're suggesting, but I think the pseudo-code solution is missing a few things.

There are two places we walk the return path to find the nearest Profiler ancestor:

  • commitLifeCycles to bubble up layout effect durations from one Profiler to the next.
  • commitProfilerPassiveEffect to bubble up passive effect durations from one Profiler to the next.

Neither of these functions are recursive. The work loop functions that call them are (e.g. commitLayoutEffects and flushPassiveMountEffects) so maybe that's what you meant to show instead with the above pseudo-code example?

There's another snag though. These durations bubble up one Profiler at a time. The "TODO" code in question is just bubbling the durations from the current Profiler to the next nearest (ancestor) Profiler. If we used a stack for this, the top of the stack would always just point to the current Profiler (which we already have, since its the current Fiber we're working on). So we'd need to maintain pointers to the top two Profilers in the stack.

This could work, but it's not as clean an implementation as you were perhaps thinking? I'll post a PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#19895

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The work loop functions that call them are (e.g. commitLayoutEffects and flushPassiveMountEffects) so maybe that's what you meant to show instead with the above pseudo-code example?

Yeah commitWork in my example is meant to correspond to commitLayoutEffects et al

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha.

The naming of commitWork confused the heck out of me then, given where the TODO comments were previously (in ReactFiberCommitWork - which also has a function called commitWork)

let parentFiber = finishedWork.return;
while (parentFiber !== null) {
if (parentFiber.tag === Profiler) {
const parentStateNode = parentFiber.stateNode;
parentStateNode.passiveEffectDuration += passiveEffectDuration;
break;
}
break;
parentFiber = parentFiber.return;
}
default:
break;
break;
}
default:
break;
}
}
}
Expand Down Expand Up @@ -841,13 +838,9 @@ function commitLifeCycles(
}
}

// Schedule a passive effect for this Profiler to call onPostCommit hooks.
// This effect should be scheduled even if there is no onPostCommit callback for this Profiler,
// because the effect is also where times bubble to parent Profilers.
enqueuePendingPassiveProfilerEffect(finishedWork);

// Propagate layout effect durations to the next nearest Profiler ancestor.
// Do not reset these values until the next render so DevTools has a chance to read them first.
// TODO: Use JS Stack instead
let parentFiber = finishedWork.return;
while (parentFiber !== null) {
if (parentFiber.tag === Profiler) {
Expand Down Expand Up @@ -1912,6 +1905,7 @@ function commitPassiveWork(finishedWork: Fiber): void {
finishedWork,
finishedWork.return,
);
break;
}
}
}
Expand All @@ -1933,13 +1927,21 @@ function commitPassiveUnmount(
}
}

function commitPassiveLifeCycles(finishedWork: Fiber): void {
function commitPassiveLifeCycles(
finishedRoot: FiberRoot,
finishedWork: Fiber,
): void {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Block: {
commitHookEffectListMount2(finishedWork);
break;
}
case Profiler: {
commitProfilerPassiveEffect(finishedRoot, finishedWork);
break;
}
}
}
Expand Down
33 changes: 4 additions & 29 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
enableSuspenseServerRenderer,
replayFailedUnitOfWorkWithInvokeGuardedCallback,
enableProfilerTimer,
enableProfilerCommitHooks,
enableSchedulerTracing,
warnAboutUnmockedScheduler,
deferRenderPhaseUpdateToNextBatch,
Expand Down Expand Up @@ -197,7 +196,6 @@ import {
commitPassiveLifeCycles as commitPassiveEffectOnFiber,
commitDetachRef,
commitAttachRef,
commitPassiveEffectDurations,
commitResetTextContent,
isSuspenseBoundaryBeingHidden,
} from './ReactFiberCommitWork.new';
Expand Down Expand Up @@ -336,7 +334,6 @@ let rootDoesHavePassiveEffects: boolean = false;
let rootWithPendingPassiveEffects: FiberRoot | null = null;
let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoSchedulerPriority;
let pendingPassiveEffectsLanes: Lanes = NoLanes;
let pendingPassiveProfilerEffects: Array<Fiber> = [];

let rootsWithPendingDiscreteUpdates: Set<FiberRoot> | null = null;

Expand Down Expand Up @@ -2451,31 +2448,18 @@ export function flushPassiveEffects(): boolean {
return false;
}

export function enqueuePendingPassiveProfilerEffect(fiber: Fiber): void {
if (enableProfilerTimer && enableProfilerCommitHooks) {
pendingPassiveProfilerEffects.push(fiber);
if (!rootDoesHavePassiveEffects) {
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalSchedulerPriority, () => {
flushPassiveEffects();
return null;
});
}
}
}

function flushPassiveMountEffects(firstChild: Fiber): void {
function flushPassiveMountEffects(root, firstChild: Fiber): void {
let fiber = firstChild;
while (fiber !== null) {
const primarySubtreeFlags = fiber.subtreeFlags & PassiveMask;

if (fiber.child !== null && primarySubtreeFlags !== NoFlags) {
flushPassiveMountEffects(fiber.child);
flushPassiveMountEffects(root, fiber.child);
}

if ((fiber.flags & Passive) !== NoFlags) {
setCurrentDebugFiberInDEV(fiber);
commitPassiveEffectOnFiber(fiber);
commitPassiveEffectOnFiber(root, fiber);
resetCurrentDebugFiberInDEV();
}

Expand Down Expand Up @@ -2586,16 +2570,7 @@ function flushPassiveEffectsImpl() {
// value set by a create function in another component.
// Layout effects have the same constraint.
flushPassiveUnmountEffects(root.current);
flushPassiveMountEffects(root.current);

if (enableProfilerTimer && enableProfilerCommitHooks) {
const profilerEffects = pendingPassiveProfilerEffects;
pendingPassiveProfilerEffects = [];
for (let i = 0; i < profilerEffects.length; i++) {
const fiber = ((profilerEffects[i]: any): Fiber);
commitPassiveEffectDurations(root, fiber);
}
}
flushPassiveMountEffects(root, root.current);

if (enableSchedulerTracing) {
popInteractions(((prevInteractions: any): Set<Interaction>));
Expand Down