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.

scan every row
  • Loading branch information
@sebmarkbage
sebmarkbage committed Jul 12, 2019
1 parent 2a052b4 commit 6ec55cb
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 115 deletions.
159 changes: 49 additions & 110 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 @@ -1014,45 +931,67 @@ function completeWork(
if (!didSuspendAlready) {
// This is the first pass. We need to figure out if anything is still
// suspended in the rendered set.
const renderedChildren = workInProgress.child;

// If new content unsuspended, but there's still some content that
// didn't. Then we need to do a second pass that forces everything
// to keep showing their fallbacks.

// 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 row = workInProgress.child;
while (row !== null) {
let suspended = findFirstSuspended(row);
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;
}
row = row.sibling;
}
}
// 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
7 changes: 6 additions & 1 deletion packages/react-reconciler/src/ReactFiberSuspenseComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,12 @@ export function findFirstSuspended(row: Fiber): null | Fiber {
if (state !== null) {
return node;
}
} else if (node.tag === SuspenseListComponent) {
} else if (
node.tag === SuspenseListComponent &&
// revealOrder undefined can't be trusted because it don't
// keep track of whether it suspended or not.
node.memoizedProps.revealOrder !== undefined
) {
let didSuspend = (node.effectTag & DidCapture) !== NoEffect;
if (didSuspend) {
return node;
Expand Down
38 changes: 34 additions & 4 deletions packages/react-reconciler/src/__tests__/ReactSuspenseList-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,9 @@ describe('ReactSuspenseList', () => {
'Loading B',
'Suspend! [C]',
'Loading C',
'A',
'Loading B',
'Loading C',
]);

// This will suspend, since the boundaries are avoided. Give them
Expand Down Expand Up @@ -859,6 +862,10 @@ describe('ReactSuspenseList', () => {
'Suspend! [C]',
'Loading C',
'D',
'Loading A',
'B',
'Loading C',
'D',
'Loading E',
'Loading F',
]);
Expand Down Expand Up @@ -1013,6 +1020,16 @@ describe('ReactSuspenseList', () => {
'E',
'Suspend! [F]',
'Loading F',
'Suspend! [A]',
'Loading A',
'Suspend! [B]',
'Loading B',
'C',
'Suspend! [D]',
'Loading D',
'E',
'Suspend! [F]',
'Loading F',
]);

// This will suspend, since the boundaries are avoided. Give them
Expand Down Expand Up @@ -1392,6 +1409,10 @@ describe('ReactSuspenseList', () => {
'Suspend! [C]',
'Loading C',
'D',
'A',
'Loading B',
'Loading C',
'D',
'Loading E',
]);

Expand Down Expand Up @@ -1516,6 +1537,10 @@ describe('ReactSuspenseList', () => {
'Suspend! [E]',
'Loading E',
'F',
'C',
'Loading D',
'Loading E',
'F',
'Loading B',
]);

Expand All @@ -1530,15 +1555,15 @@ describe('ReactSuspenseList', () => {
</Fragment>,
);

await E.resolve();
await D.resolve();

expect(Scheduler).toFlushAndYield(['Suspend! [D]', 'E']);
expect(Scheduler).toFlushAndYield(['D', 'Suspend! [E]']);

// Incremental loading is suspended.
jest.advanceTimersByTime(500);

// Even though E is unsuspended, it's still in loading state because
// it is blocked by D.
// Even though D is unsuspended, it's still in loading state because
// it is blocked by E.
expect(ReactNoop).toMatchRenderedOutput(
<Fragment>
<span>Loading B</span>
Expand Down Expand Up @@ -1650,6 +1675,11 @@ describe('ReactSuspenseList', () => {
'Loading C',
'Suspend! [D]',
'Loading D',
'A',
'Loading B',
'Loading C',
'Suspend! [D]',
'Loading D',
'Loading E',
]);

Expand Down

0 comments on commit 6ec55cb

Please sign in to comment.