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

Invalid actualDuration+treeBaseDuration for hidden+suspended trees #14065

Merged
merged 7 commits into from
Nov 7, 2018
40 changes: 36 additions & 4 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,12 @@ import {
} from './ReactChildFiber';
import {processUpdateQueue} from './ReactUpdateQueue';
import {NoWork, Never} from './ReactFiberExpirationTime';
import {ConcurrentMode, StrictMode, NoContext} from './ReactTypeOfMode';
import {
ConcurrentMode,
NoContext,
ProfileMode,
StrictMode,
} from './ReactTypeOfMode';
import {
shouldSetTextContent,
shouldDeprioritizeSubtree,
Expand Down Expand Up @@ -743,7 +748,7 @@ function mountLazyComponent(
) {
if (_current !== null) {
// An lazy component only mounts if it suspended inside a non-
// concurrent tree, in an inconsistent state. We want to tree it like
Copy link
Collaborator

Choose a reason for hiding this comment

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

🌳

// concurrent tree, in an inconsistent state. We want to treat it like
// a new mount, even though an empty version of it already committed.
// Disconnect the alternate pointers.
_current.alternate = null;
Expand Down Expand Up @@ -829,7 +834,7 @@ function mountIncompleteClassComponent(
) {
if (_current !== null) {
// An incomplete component only mounts if it suspended inside a non-
// concurrent tree, in an inconsistent state. We want to tree it like
// concurrent tree, in an inconsistent state. We want to treat it like
// a new mount, even though an empty version of it already committed.
// Disconnect the alternate pointers.
_current.alternate = null;
Expand Down Expand Up @@ -886,7 +891,7 @@ function mountIndeterminateComponent(
) {
if (_current !== null) {
// An indeterminate component only mounts if it suspended inside a non-
// concurrent tree, in an inconsistent state. We want to tree it like
// concurrent tree, in an inconsistent state. We want to treat it like
// a new mount, even though an empty version of it already committed.
// Disconnect the alternate pointers.
_current.alternate = null;
Expand Down Expand Up @@ -1188,6 +1193,19 @@ function updateSuspenseComponent(
}
}

// Because primaryChildFragment is a new fiber that we're inserting as the
// parent of a new tree, we need to set its treeBaseDuration.
if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
// treeBaseDuration is the sum of all the child tree base durations.
let treeBaseDuration = 0;
let hiddenChild = primaryChildFragment.child;
while (hiddenChild !== null) {
treeBaseDuration += hiddenChild.treeBaseDuration;
hiddenChild = hiddenChild.sibling;
}
primaryChildFragment.treeBaseDuration = treeBaseDuration;
}

// Clone the fallback child fragment, too. These we'll continue
// working on.
const fallbackChildFragment = (primaryChildFragment.sibling = createWorkInProgress(
Expand Down Expand Up @@ -1239,6 +1257,7 @@ function updateSuspenseComponent(
NoWork,
null,
);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested comment:

Because primaryChildFragment is a new fiber that we're inserting as the parent of a new tree, we need to set its treeBaseDuration.

primaryChildFragment.effectTag |= Placement;
primaryChildFragment.child = currentPrimaryChild;
currentPrimaryChild.return = primaryChildFragment;
Expand All @@ -1254,6 +1273,19 @@ function updateSuspenseComponent(
primaryChildFragment.child = progressedPrimaryChild;
}

// Because primaryChildFragment is a new fiber that we're inserting as the
// parent of a new tree, we need to set its treeBaseDuration.
if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
// treeBaseDuration is the sum of all the child tree base durations.
let treeBaseDuration = 0;
let hiddenChild = primaryChildFragment.child;
while (hiddenChild !== null) {
treeBaseDuration += hiddenChild.treeBaseDuration;
hiddenChild = hiddenChild.sibling;
}
primaryChildFragment.treeBaseDuration = treeBaseDuration;
}

// Create a fragment from the fallback children, too.
const fallbackChildFragment = (primaryChildFragment.sibling = createFiberFromFragment(
nextFallbackChildren,
Expand Down
30 changes: 16 additions & 14 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -1049,9 +1049,18 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null {
return null;
}
} else {
if (workInProgress.mode & ProfileMode) {
if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
// Record the render duration for the fiber that errored.
stopProfilerTimerIfRunningAndRecordDelta(workInProgress, false);

// Include the time spent working on failed children before continuing.
let actualDuration = workInProgress.actualDuration;
let child = workInProgress.child;
while (child !== null) {
actualDuration += child.actualDuration;
child = child.sibling;
}
workInProgress.actualDuration = actualDuration;
}

// This fiber did not complete because something threw. Pop values off
Expand All @@ -1076,19 +1085,6 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null {
ReactFiberInstrumentation.debugTool.onCompleteWork(workInProgress);
}

if (enableProfilerTimer) {
// Include the time spent working on failed children before continuing.
if (next.mode & ProfileMode) {
let actualDuration = next.actualDuration;
let child = next.child;
while (child !== null) {
actualDuration += child.actualDuration;
child = child.sibling;
}
next.actualDuration = actualDuration;
}
}

// If completing this work spawned new work, do that next. We'll come
// back here again.
// Since we're restarting, remove anything that is not a host effect
Expand Down Expand Up @@ -1314,6 +1310,12 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void {
didFatal = true;
onUncaughtError(thrownValue);
} else {
if (enableProfilerTimer && nextUnitOfWork.mode & ProfileMode) {
// Record the time spent rendering before an error was thrown.
// This avoids inaccurate Profiler durations in the case of a suspended render.
stopProfilerTimerIfRunningAndRecordDelta(nextUnitOfWork, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This gets called again in completeUnitOfWork, with no work in between. Seems wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, because completeUnitOfWork also calls startProfilerTimer before it starts working on the complete phase.

This clears out the previous timer, erasing any time spent in the "begin" phase. That's what we're recording here.

}

if (__DEV__) {
// Reset global debug state
// We assume this is defined in DEV
Expand Down
Loading