Skip to content

Commit

Permalink
Don't call onCommit et al if there are no effects (#19863)
Browse files Browse the repository at this point in the history
* Don't call onCommit et al if there are no effects

Checks `subtreeFlags` before scheduling an effect on the Profiler.

* Fix failing Profiler tests

The change to conditionally call Profiler commit hooks only if updates were scheduled broke a few of the Profiler tests. I've fixed the tests by either:
* Adding a no-op passive effect into the subtree or
* Converting onPostCommit to onCommit

When possible, I opted to add the no-op passive effect to the tests since that that hook is called later (during passive phase) so the test is a little broader. In a few cases, this required adding awkward act() wrappers so I opted to go with onCommit instead.

Co-authored-by: Brian Vaughn <bvaughn@fb.com>
  • Loading branch information
acdlite and Brian Vaughn authored Sep 22, 2020
1 parent 7355bf5 commit 81aaee5
Show file tree
Hide file tree
Showing 4 changed files with 795 additions and 714 deletions.
15 changes: 0 additions & 15 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@ import {
Hydrating,
ContentReset,
DidCapture,
Update,
Passive,
Ref,
Deletion,
ForceUpdateForLegacySuspense,
Expand Down Expand Up @@ -675,9 +673,6 @@ function updateProfiler(
renderLanes: Lanes,
) {
if (enableProfilerTimer) {
// 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,
const stateNode = workInProgress.stateNode;
Expand Down Expand Up @@ -3117,16 +3112,6 @@ 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 |= Passive | 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
15 changes: 13 additions & 2 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import {
Placement,
Snapshot,
Update,
Callback,
PassiveMask,
} from './ReactFiberFlags';
import getComponentName from 'shared/getComponentName';
Expand Down Expand Up @@ -676,10 +677,17 @@ function commitLifeCycles(
if (enableProfilerTimer) {
const {onCommit, onRender} = finishedWork.memoizedProps;
const {effectDuration} = finishedWork.stateNode;
const flags = finishedWork.flags;

const commitTime = getCommitTime();

if (typeof onRender === 'function') {
const OnRenderFlag = Update;
const OnCommitFlag = Callback;

if (
(flags & OnRenderFlag) !== NoFlags &&
typeof onRender === 'function'
) {
if (enableSchedulerTracing) {
onRender(
finishedWork.memoizedProps.id,
Expand All @@ -703,7 +711,10 @@ function commitLifeCycles(
}

if (enableProfilerCommitHooks) {
if (typeof onCommit === 'function') {
if (
(flags & OnCommitFlag) !== NoFlags &&
typeof onCommit === 'function'
) {
if (enableSchedulerTracing) {
onCommit(
finishedWork.memoizedProps.id,
Expand Down
55 changes: 54 additions & 1 deletion packages/react-reconciler/src/ReactFiberCompleteWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,15 @@ import {
import {
Ref,
Update,
Callback,
Passive,
Deletion,
NoFlags,
DidCapture,
Snapshot,
MutationMask,
LayoutMask,
PassiveMask,
StaticMask,
} from './ReactFiberFlags';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -787,6 +792,8 @@ function bubbleProperties(completedWork: Fiber) {
}

completedWork.childLanes = newChildLanes;

return didBailout;
}

function completeWork(
Expand All @@ -804,7 +811,6 @@ function completeWork(
case ForwardRef:
case Fragment:
case Mode:
case Profiler:
case ContextConsumer:
case MemoComponent:
bubbleProperties(workInProgress);
Expand Down Expand Up @@ -966,6 +972,53 @@ function completeWork(
bubbleProperties(workInProgress);
return null;
}
case Profiler: {
const didBailout = bubbleProperties(workInProgress);
if (!didBailout) {
// Use subtreeFlags to determine which commit callbacks should fire.
// TODO: Move this logic to the commit phase, since we already check if
// a fiber's subtree contains effects. Refactor the commit phase's
// depth-first traversal so that we can put work tag-specific logic
// before or after committing a subtree's effects.
const OnRenderFlag = Update;
const OnCommitFlag = Callback;
const OnPostCommitFlag = Passive;
const subtreeFlags = workInProgress.subtreeFlags;
const flags = workInProgress.flags;
let newFlags = flags;

// Call onRender any time this fiber or its subtree are worked on, even
// if there are no effects
newFlags |= OnRenderFlag;

// Call onCommit only if the subtree contains layout work, or if it
// contains deletions, since those might result in unmount work, which
// we include in the same measure.
// TODO: Can optimize by using a static flag to track whether a tree
// contains layout effects, like we do for passive effects.
if (
(flags & (LayoutMask | Deletion)) !== NoFlags ||
(subtreeFlags & (LayoutMask | Deletion)) !== NoFlags
) {
newFlags |= OnCommitFlag;
}

// Call onPostCommit only if the subtree contains passive work.
// Don't have to check for deletions, because Deletion is already
// a passive flag.
if (
(flags & PassiveMask) !== NoFlags ||
(subtreeFlags & PassiveMask) !== NoFlags
) {
newFlags |= OnPostCommitFlag;
}
workInProgress.flags = newFlags;
} else {
// This fiber and its subtree bailed out, so don't fire any callbacks.
}

return null;
}
case SuspenseComponent: {
popSuspenseContext(workInProgress);
const nextState: null | SuspenseState = workInProgress.memoizedState;
Expand Down
Loading

0 comments on commit 81aaee5

Please sign in to comment.