Skip to content

Commit

Permalink
Delete hasSuspendedChildrenAndNewContent optimization
Browse files Browse the repository at this point in the history 
This function exists to optimize the case where all the boundaries in the
head are already suspended. In that case there's no need to do a second
rerender pass. However, in that case, we're not really going to show much
new content on the screen anyway.

So I'm not sure it's worth keeping this special case around.
  • Loading branch information
@sebmarkbage
sebmarkbage committed Jul 12, 2019
1 parent 73d6c0b commit de7e82b
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 110 deletions.
153 changes: 44 additions & 109 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -598,89 +598,6 @@ function cutOffTailIfNeeded(
}
}

// Note this, might mutate the workInProgress passed in.
function hasSuspendedChildrenAndNewContent(
workInProgress: Fiber,
firstChild: null | Fiber,
): boolean {
// Traversal to see if any of the immediately nested Suspense boundaries
// are in their fallback states. I.e. something suspended in them.
// And if some of them have new content that wasn't already visible.
let hasSuspendedBoundaries = false;
let hasNewContent = false;

let node = firstChild;
while (node !== null) {
// TODO: Hidden subtrees should not be considered.
if (node.tag === SuspenseComponent) {
const state: SuspenseState | null = node.memoizedState;
const isShowingFallback = state !== null;
if (isShowingFallback) {
// Tag the parent fiber as having suspended boundaries.
if (!hasSuspendedBoundaries) {
workInProgress.effectTag |= DidCapture;
}

hasSuspendedBoundaries = true;

if (node.updateQueue !== null) {
// If this is a newly suspended tree, it might not get committed as
// part of the second pass. In that case nothing will subscribe to
// its thennables. Instead, we'll transfer its thennables to the
// SuspenseList so that it can retry if they resolve.
// There might be multiple of these in the list but since we're
// going to wait for all of them anyway, it doesn't really matter
// which ones gets to ping. In theory we could get clever and keep
// track of how many dependencies remain but it gets tricky because
// in the meantime, we can add/remove/change items and dependencies.
// We might bail out of the loop before finding any but that
// doesn't matter since that means that the other boundaries that
// we did find already has their listeners attached.
workInProgress.updateQueue = node.updateQueue;
workInProgress.effectTag |= Update;
}
} else {
const current = node.alternate;
const wasNotShowingContent =
current === null || current.memoizedState !== null;
if (wasNotShowingContent) {
hasNewContent = true;
}
}
if (hasSuspendedBoundaries && hasNewContent) {
return true;
}
} else {
// TODO: We can probably just use the information from the list and not
// drill into its children just like if it was a Suspense boundary.
if (node.tag === SuspenseListComponent && node.updateQueue !== null) {
// If there's a nested SuspenseList, we might have transferred
// the thennables set to it already so we must get it from there.
workInProgress.updateQueue = node.updateQueue;
workInProgress.effectTag |= Update;
}

if (node.child !== null) {
node.child.return = node;
node = node.child;
continue;
}
}
if (node === workInProgress) {
return false;
}
while (node.sibling === null) {
if (node.return === null || node.return === workInProgress) {
return false;
}
node = node.return;
}
node.sibling.return = node.return;
node = node.sibling;
}
return false;
}

function completeWork(
current: Fiber | null,
workInProgress: Fiber,
Expand Down Expand Up @@ -1022,37 +939,55 @@ function completeWork(
// We might be suspended if something in this render pass suspended, or
// something in the previous committed pass suspended. Otherwise,
// there's no chance so we can skip the expensive call to
// hasSuspendedChildrenAndNewContent.
// findFirstSuspended.
let cannotBeSuspended =
renderHasNotSuspendedYet() &&
(current === null || (current.effectTag & DidCapture) === NoEffect);
let needsRerender =
!cannotBeSuspended &&
hasSuspendedChildrenAndNewContent(workInProgress, renderedChildren);
if (needsRerender) {
// Rerender the whole list, but this time, we'll force fallbacks
// to stay in place.
// Reset the effect list before doing the second pass since that's now invalid.
workInProgress.firstEffect = workInProgress.lastEffect = null;
// Reset the child fibers to their original state.
resetChildFibers(workInProgress, renderExpirationTime);
if (!cannotBeSuspended) {
let suspended = findFirstSuspended(renderedChildren);
if (suspended !== null) {
didSuspendAlready = true;
workInProgress.effectTag |= DidCapture;
cutOffTailIfNeeded(renderState, false);

// Set up the Suspense Context to force suspense and immediately
// rerender the children.
pushSuspenseContext(
workInProgress,
setShallowSuspenseContext(
suspenseStackCursor.current,
ForceSuspenseFallback,
),
);
return workInProgress.child;
// If this is a newly suspended tree, it might not get committed as
// part of the second pass. In that case nothing will subscribe to
// its thennables. Instead, we'll transfer its thennables to the
// SuspenseList so that it can retry if they resolve.
// There might be multiple of these in the list but since we're
// going to wait for all of them anyway, it doesn't really matter
// which ones gets to ping. In theory we could get clever and keep
// track of how many dependencies remain but it gets tricky because
// in the meantime, we can add/remove/change items and dependencies.
// We might bail out of the loop before finding any but that
// doesn't matter since that means that the other boundaries that
// we did find already has their listeners attached.
let newThennables = suspended.updateQueue;
if (newThennables !== null) {
workInProgress.updateQueue = newThennables;
workInProgress.effectTag |= Update;
}

// Rerender the whole list, but this time, we'll force fallbacks
// to stay in place.
// Reset the effect list before doing the second pass since that's now invalid.
workInProgress.firstEffect = workInProgress.lastEffect = null;
// Reset the child fibers to their original state.
resetChildFibers(workInProgress, renderExpirationTime);

// Set up the Suspense Context to force suspense and immediately
// rerender the children.
pushSuspenseContext(
workInProgress,
setShallowSuspenseContext(
suspenseStackCursor.current,
ForceSuspenseFallback,
),
);
return workInProgress.child;
}
}
// hasSuspendedChildrenAndNewContent could've set didSuspendAlready
didSuspendAlready =
(workInProgress.effectTag & DidCapture) !== NoEffect;
}
if (didSuspendAlready) {
} else {
cutOffTailIfNeeded(renderState, false);
}
// Next we're going to render the tail.
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberSuspenseComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export function shouldCaptureSuspense(
return true;
}

export function findFirstSuspended(row: Fiber): null | Fiber {
export function findFirstSuspended(row: null | Fiber): null | Fiber {
let node = row;
while (node !== null) {
if (node.tag === SuspenseComponent) {
Expand Down

0 comments on commit de7e82b

Please sign in to comment.