Skip to content

Commit

Permalink
Don't prerender siblings of suspended component (#26380)
Browse files Browse the repository at this point in the history 
Today if something suspends, React will continue rendering the siblings
of that component.

Our original rationale for prerendering the siblings of a suspended
component was to initiate any lazy fetches that they might contain. This
was when we were more bullish about lazy fetching being a good idea some
of the time (when combined with prefetching), as opposed to our latest
thinking, which is that it's almost always a bad idea.

Another rationale for the original behavior was that the render was I/O
bound, anyway, so we might as do some extra work in the meantime. But
this was before we had the concept of instant loading states: when
navigating to a new screen, it's better to show a loading state as soon
as you can (often a skeleton UI), rather than delay the transition.
(There are still cases where we block the render, when a suitable
loading state is not available; it's just not _all_ cases where
something suspends.) So the biggest issue with our existing
implementation is that the prerendering of the siblings happens within
the same render pass as the one that suspended — _before_ the loading
state appears.

What we should do instead is immediately unwind the stack as soon as
something suspends, to unblock the loading state.

If we want to preserve the ability to prerender the siblings, what we
could do is schedule special render pass immediately after the fallback
is displayed. This is likely what we'll do in the future. However, in
the new implementation of `use`, there's another reason we don't
prerender siblings: so we can preserve the state of the stack when
something suspends, and resume where we left of when the promise
resolves without replaying the parents. The only way to do this
currently is to suspend the entire work loop. Fiber does not currently
support rendering multiple siblings in "parallel". Once you move onto
the next sibling, the stack of the previous sibling is discarded and
cannot be restored. We do plan to implement this feature, but it will
require a not-insignificant refactor.

Given that lazy data fetching is already bad for performance, the best
trade off for now seems to be to disable prerendering of siblings. This
gives us the best performance characteristics when you're following best
practices (i.e. hoist data fetches to Server Components or route
loaders), at the expense of making an already bad pattern a bit worse.

Later, when we implement resumable context stacks, we can reenable
sibling prerendering. Though even then the use case will mostly be to
prerender the CPU-bound work, not lazy fetches.

DiffTrain build for [12a1d14](12a1d14)
  • Loading branch information
@acdlite
acdlite committed Mar 21, 2023
1 parent 2473ff0 commit b08f056
Show file tree
Hide file tree
Showing 19 changed files with 2,006 additions and 1,343 deletions.
2 changes: 1 addition & 1 deletion compiled/facebook-www/REVISION
View file
Original file line number Diff line number Diff line change
@@ -1 +1 @@
77ba1618a528787baaa8e007fadaff93a86fb675
12a1d140e366aa8d95338e4412117f16da79a078
2 changes: 1 addition & 1 deletion compiled/facebook-www/React-dev.modern.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ if (
}
"use strict";

var ReactVersion = "18.3.0-www-modern-1423f459";
var ReactVersion = "18.3.0-www-modern-0d5680f8";

// ATTENTION
// When adding new symbols to this file,
Expand Down
216 changes: 145 additions & 71 deletions compiled/facebook-www/ReactART-dev.classic.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ function _assertThisInitialized(self) {
return self;
}

var ReactVersion = "18.3.0-www-classic-26dea44b";
var ReactVersion = "18.3.0-www-classic-f4456f06";

var LegacyRoot = 0;
var ConcurrentRoot = 1;
Expand Down Expand Up @@ -165,7 +165,9 @@ var ReactSharedInternals =
// Re-export dynamic flags from the www version.
var dynamicFeatureFlags = require("ReactFeatureFlags");

var replayFailedUnitOfWorkWithInvokeGuardedCallback =
var revertRemovalOfSiblingPrerendering =
dynamicFeatureFlags.revertRemovalOfSiblingPrerendering,
replayFailedUnitOfWorkWithInvokeGuardedCallback =
dynamicFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback,
deferRenderPhaseUpdateToNextBatch =
dynamicFeatureFlags.deferRenderPhaseUpdateToNextBatch,
Expand Down Expand Up @@ -24762,10 +24764,10 @@ function renderRootSync(root, lanes) {
}

default: {
// Continue with the normal work loop.
// Unwind then continue with the normal work loop.
workInProgressSuspendedReason = NotSuspended;
workInProgressThrownValue = null;
unwindSuspendedUnitOfWork(unitOfWork, thrownValue);
throwAndUnwindWorkLoop(unitOfWork, thrownValue);
break;
}
}
Expand Down Expand Up @@ -24872,7 +24874,7 @@ function renderRootConcurrent(root, lanes) {
// Unwind then continue with the normal work loop.
workInProgressSuspendedReason = NotSuspended;
workInProgressThrownValue = null;
unwindSuspendedUnitOfWork(unitOfWork, thrownValue);
throwAndUnwindWorkLoop(unitOfWork, thrownValue);
break;
}

Expand Down Expand Up @@ -24937,7 +24939,7 @@ function renderRootConcurrent(root, lanes) {
// Otherwise, unwind then continue with the normal work loop.
workInProgressSuspendedReason = NotSuspended;
workInProgressThrownValue = null;
unwindSuspendedUnitOfWork(unitOfWork, thrownValue);
throwAndUnwindWorkLoop(unitOfWork, thrownValue);
}

break;
Expand Down Expand Up @@ -25002,7 +25004,7 @@ function renderRootConcurrent(root, lanes) {

workInProgressSuspendedReason = NotSuspended;
workInProgressThrownValue = null;
unwindSuspendedUnitOfWork(unitOfWork, thrownValue);
throwAndUnwindWorkLoop(unitOfWork, thrownValue);
break;
}

Expand All @@ -25013,7 +25015,7 @@ function renderRootConcurrent(root, lanes) {
// always unwind.
workInProgressSuspendedReason = NotSuspended;
workInProgressThrownValue = null;
unwindSuspendedUnitOfWork(unitOfWork, thrownValue);
throwAndUnwindWorkLoop(unitOfWork, thrownValue);
break;
}

Expand Down Expand Up @@ -25216,7 +25218,7 @@ function replaySuspendedUnitOfWork(unitOfWork) {
ReactCurrentOwner.current = null;
}

function unwindSuspendedUnitOfWork(unitOfWork, thrownValue) {
function throwAndUnwindWorkLoop(unitOfWork, thrownValue) {
// This is a fork of performUnitOfWork specifcally for unwinding a fiber
// that threw an exception.
//
Expand Down Expand Up @@ -25259,9 +25261,23 @@ function unwindSuspendedUnitOfWork(unitOfWork, thrownValue) {
// To prevent an infinite loop, bubble the error up to the next parent.
workInProgress = returnFiber;
throw error;
} // Return to the normal work loop.
}

completeUnitOfWork(unitOfWork);
if (unitOfWork.flags & Incomplete) {
// Unwind the stack until we reach the nearest boundary.
unwindUnitOfWork(unitOfWork);
} else {
// Although the fiber suspended, we're intentionally going to commit it in
// an inconsistent state. We can do this safely in cases where we know the
// inconsistent tree will be hidden.
//
// This currently only applies to Legacy Suspense implementation, but we may
// port a version of this to concurrent roots, too, when performing a
// synchronous render. Because that will allow us to mutate the tree as we
// go instead of buffering mutations until the end. Though it's unclear if
// this particular path is how that would be implemented.
completeUnitOfWork(unitOfWork);
}
}

function completeUnitOfWork(unitOfWork) {
Expand All @@ -25270,75 +25286,53 @@ function completeUnitOfWork(unitOfWork) {
var completedWork = unitOfWork;

do {
// The current, flushed, state of this fiber is the alternate. Ideally
// nothing should rely on this, but relying on it here means that we don't
// need an additional field on the work in progress.
var current = completedWork.alternate;
var returnFiber = completedWork.return; // Check if the work completed or if something threw.

if ((completedWork.flags & Incomplete) === NoFlags$1) {
setCurrentFiber(completedWork);
var next = void 0;

if ((completedWork.mode & ProfileMode) === NoMode) {
next = completeWork(current, completedWork, renderLanes);
} else {
startProfilerTimer(completedWork);
next = completeWork(current, completedWork, renderLanes); // Update render duration assuming we didn't error.

stopProfilerTimerIfRunningAndRecordDelta(completedWork, false);
}

resetCurrentFiber();

if (next !== null) {
// Completing this fiber spawned new work. Work on that next.
workInProgress = next;
if (revertRemovalOfSiblingPrerendering) {
if ((completedWork.flags & Incomplete) !== NoFlags$1) {
// This fiber did not complete, because one of its children did not
// complete. Switch to unwinding the stack instead of completing it.
//
// The reason "unwind" and "complete" is interleaved is because when
// something suspends, we continue rendering the siblings even though
// they will be replaced by a fallback.
// TODO: Disable sibling prerendering, then remove this branch.
unwindUnitOfWork(completedWork);
return;
}
} else {
// This fiber did not complete because something threw. Pop values off
// the stack without entering the complete phase. If this is a boundary,
// capture values if possible.
var _next = unwindWork(current, completedWork); // Because this fiber did not complete, don't reset its lanes.

if (_next !== null) {
// 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
// from the effect tag.
_next.flags &= HostEffectMask;
workInProgress = _next;
return;
{
if ((completedWork.flags & Incomplete) !== NoFlags$1) {
// NOTE: If we re-enable sibling prerendering in some cases, this branch
// is where we would switch to the unwinding path.
error(
"Internal React error: Expected this fiber to be complete, but " +
"it isn't. It should have been unwound. This is a bug in React."
);
}
}
} // The current, flushed, state of this fiber is the alternate. Ideally
// nothing should rely on this, but relying on it here means that we don't
// need an additional field on the work in progress.

if ((completedWork.mode & ProfileMode) !== NoMode) {
// Record the render duration for the fiber that errored.
stopProfilerTimerIfRunningAndRecordDelta(completedWork, false); // Include the time spent working on failed children before continuing.
var current = completedWork.alternate;
var returnFiber = completedWork.return;
setCurrentFiber(completedWork);
var next = void 0;

var actualDuration = completedWork.actualDuration;
var child = completedWork.child;
if ((completedWork.mode & ProfileMode) === NoMode) {
next = completeWork(current, completedWork, renderLanes);
} else {
startProfilerTimer(completedWork);
next = completeWork(current, completedWork, renderLanes); // Update render duration assuming we didn't error.

while (child !== null) {
// $FlowFixMe[unsafe-addition] addition with possible null/undefined value
actualDuration += child.actualDuration;
child = child.sibling;
}
stopProfilerTimerIfRunningAndRecordDelta(completedWork, false);
}

completedWork.actualDuration = actualDuration;
}
resetCurrentFiber();

if (returnFiber !== null) {
// Mark the parent fiber as incomplete and clear its subtree flags.
returnFiber.flags |= Incomplete;
returnFiber.subtreeFlags = NoFlags$1;
returnFiber.deletions = null;
} else {
// We've unwound all the way to the root.
workInProgressRootExitStatus = RootDidNotComplete;
workInProgress = null;
return;
}
if (next !== null) {
// Completing this fiber spawned new work. Work on that next.
workInProgress = next;
return;
}

var siblingFiber = completedWork.sibling;
Expand All @@ -25360,6 +25354,86 @@ function completeUnitOfWork(unitOfWork) {
}
}

function unwindUnitOfWork(unitOfWork) {
var incompleteWork = unitOfWork;

do {
// The current, flushed, state of this fiber is the alternate. Ideally
// nothing should rely on this, but relying on it here means that we don't
// need an additional field on the work in progress.
var current = incompleteWork.alternate; // This fiber did not complete because something threw. Pop values off
// the stack without entering the complete phase. If this is a boundary,
// capture values if possible.

var next = unwindWork(current, incompleteWork); // Because this fiber did not complete, don't reset its lanes.

if (next !== null) {
// Found a boundary that can handle this exception. Re-renter the
// begin phase. This branch will return us to the normal work loop.
//
// Since we're restarting, remove anything that is not a host effect
// from the effect tag.
next.flags &= HostEffectMask;
workInProgress = next;
return;
} // Keep unwinding until we reach either a boundary or the root.

if ((incompleteWork.mode & ProfileMode) !== NoMode) {
// Record the render duration for the fiber that errored.
stopProfilerTimerIfRunningAndRecordDelta(incompleteWork, false); // Include the time spent working on failed children before continuing.

var actualDuration = incompleteWork.actualDuration;
var child = incompleteWork.child;

while (child !== null) {
// $FlowFixMe[unsafe-addition] addition with possible null/undefined value
actualDuration += child.actualDuration;
child = child.sibling;
}

incompleteWork.actualDuration = actualDuration;
} // TODO: Once we stop prerendering siblings, instead of resetting the parent
// of the node being unwound, we should be able to reset node itself as we
// unwind the stack. Saves an additional null check.

var returnFiber = incompleteWork.return;

if (returnFiber !== null) {
// Mark the parent fiber as incomplete and clear its subtree flags.
// TODO: Once we stop prerendering siblings, we may be able to get rid of
// the Incomplete flag because unwinding to the nearest boundary will
// happen synchronously.
returnFiber.flags |= Incomplete;
returnFiber.subtreeFlags = NoFlags$1;
returnFiber.deletions = null;
}

if (revertRemovalOfSiblingPrerendering) {
// If there are siblings, work on them now even though they're going to be
// replaced by a fallback. We're "prerendering" them. Historically our
// rationale for this behavior has been to initiate any lazy data requests
// in the siblings, and also to warm up the CPU cache.
// TODO: Don't prerender siblings. With `use`, we suspend the work loop
// until the data has resolved, anyway.
var siblingFiber = incompleteWork.sibling;

if (siblingFiber !== null) {
// This branch will return us to the normal work loop.
workInProgress = siblingFiber;
return;
}
} // Otherwise, return to the parent
// $FlowFixMe[incompatible-type] we bail out when we get a null

incompleteWork = returnFiber; // Update the next thing we're working on in case something throws.

workInProgress = incompleteWork;
} while (incompleteWork !== null); // We've unwound all the way to the root.

workInProgressRootExitStatus = RootDidNotComplete;
workInProgress = null;
}

function commitRoot(root, recoverableErrors, transitions) {
// TODO: This no longer makes any sense. We already wrap the mutation and
// layout phases. Should be able to remove.
Expand Down
Loading

0 comments on commit b08f056

Please sign in to comment.