Skip to content

Commit

Permalink
Invalid actualDuration+treeBaseDuration for hidden+suspended trees (f…
Browse files Browse the repository at this point in the history
…acebook#14065)

* Fixed `treeBaseDuration` by propagating its value from the suspended tree to the Fragment React temporarily wraps around it when showing the fallback UI.
* Fixed `actualDuration` by recording elapsed profiler time in the event of an error.
* Fixed `actualDuration` in concurrent mode by propagating the time spent rendering the suspending component to its parent.

Also updated ReactSuspensePlaceholder-test.internal to cover these new cases.
  • Loading branch information
bvaughn authored and jetoneza committed Jan 23, 2019
1 parent 57322a9 commit 5e8391b
Show file tree
Hide file tree
Showing 3 changed files with 297 additions and 24 deletions.
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
// 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,
);

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);
}

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

0 comments on commit 5e8391b

Please sign in to comment.