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.

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

Don't call onCommit et al if there are no effects #19863

Merged
merged 2 commits into from
Sep 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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 @@ -71,6 +71,7 @@ import {
Placement,
Snapshot,
Update,
Callback,
PassiveMask,
} from './ReactFiberFlags';
import getComponentName from 'shared/getComponentName';
Expand Down Expand Up @@ -792,10 +793,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 @@ -819,7 +827,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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wasn't sure what the desired behavior was here, but I think conceptually this should fire any time one of the duration fields changes?

So it should at least fire for the initial mount.

Maybe for updates, we can check if the subtree flags includes PerformedWork?

Copy link
Contributor

@bvaughn bvaughn Sep 22, 2020

Choose a reason for hiding this comment

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

but I think conceptually this should fire any time one of the duration fields changes?

Not sure if I'm reading too literally, but we'll still want to call onRender even if duration happened to be the same– if we did any rendering work in the subtree.

But yes, I think checking for work sounds right to me. I'll take a look at this. I think it might require a few tests updates too.

Copy link
Contributor

Choose a reason for hiding this comment

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


// 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.
Comment on lines +997 to +998
Copy link
Contributor

@bvaughn bvaughn Sep 22, 2020

Choose a reason for hiding this comment

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

Can we though? Static flags aren't specific to a render, so they wouldn't tell us if the layout effect happened to fire during the current update (which we care about for profiler)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right but it would at least let us optimize the case where there are no layout unmount work in the tree at all. Probably not that big of a win, though, since most trees have at least one, like a DOM ref or something.

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