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.

Sign up for GitHub

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

Never attach ping listeners in legacy Suspense #22407

Merged
merged 1 commit into from
Sep 23, 2021
Merged

Never attach ping listeners in legacy Suspense #22407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 22 additions & 17 deletions packages/react-reconciler/src/ReactFiberThrow.new.js
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -297,23 +297,31 @@ function throwException(
wakeables.add(wakeable); wakeables.add(wakeable);
} }


if ((workInProgress.mode & ConcurrentMode) === NoMode) {
// Legacy Mode Suspense
//
// If the boundary is in legacy mode, we should *not* // If the boundary is in legacy mode, we should *not*
// suspend the commit. Pretend as if the suspended component rendered // suspend the commit. Pretend as if the suspended component rendered
// null and keep rendering. In the commit phase, we'll schedule a // null and keep rendering. When the Suspense boundary completes,
// subsequent synchronous update to re-render the Suspense. // we'll do a second pass to render the fallback.
if (workInProgress === returnFiber) {
// Special case where we suspended while reconciling the children of
// a Suspense boundary's inner Offscreen wrapper fiber. This happens
// when a React.lazy component is a direct child of a
// Suspense boundary.
// //
// Note: It doesn't matter whether the component that suspended was // Suspense boundaries are implemented as multiple fibers, but they
// inside a concurrent mode tree. If the Suspense is outside of it, we // are a single conceptual unit. The legacy mode behavior where we
// should *not* suspend the commit. // pretend the suspended fiber committed as `null` won't work,
// because in this case the "suspended" fiber is the inner
// Offscreen wrapper.
// //
// If the suspense boundary suspended itself suspended, we don't have to // Because the contents of the boundary haven't started rendering
// do this trick because nothing was partially started. We can just // yet (i.e. nothing in the tree has partially rendered) we can
// directly do a second pass over the fallback in this render and // switch to the regular, concurrent mode behavior: mark the
// pretend we meant to render that directly. // boundary with ShouldCapture and enter the unwind phase.
if ( workInProgress.flags |= ShouldCapture;
Copy link
Contributor

Choose a reason for hiding this comment

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

So it seems the only place where we do something for the ShouldCapture flag is here: https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberUnwindWork.new.js#L64

So am I right in understanding that your change is just signaling that we haven't done the work for this fiber yet and that this flag is only used for attributing time spent doing work to it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The profiler call is a red herring. The relevant part of this branch is that it marks a DidCapture flag:

react/packages/react-reconciler/src/ReactFiberUnwindWork.new.js

Line 117 in d174d06

workInProgress.flags = (flags & ~ShouldCapture) | DidCapture;

and it returns the work-in-progress fiber (which in this case is a Suspense boundary) instead of returning null:

react/packages/react-reconciler/src/ReactFiberUnwindWork.new.js

Lines 125 to 127 in d174d06

return workInProgress;
}
return null;

That will tell the work loop to render the Suspense boundary again:

react/packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Lines 1700 to 1708 in d174d06

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

And since we set a DidCapture flag, we'll render the fallback this time instead of the regular children:

react/packages/react-reconciler/src/ReactFiberBeginWork.new.js

Line 1936 in d174d06

const didSuspend = (workInProgress.flags & DidCapture) !== NoFlags;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you look at the old code, though, we were already doing this because line 315 here causes us to fall through to the regular concurrent mode path:

react/packages/react-reconciler/src/ReactFiberThrow.new.js

Lines 313 to 316 in d174d06

if (
(workInProgress.mode & ConcurrentMode) === NoMode &&
workInProgress !== returnFiber
) {

which looks like this:

react/packages/react-reconciler/src/ReactFiberThrow.new.js

Lines 412 to 414 in d174d06

attachPingListener(root, wakeable, rootRenderLanes);
workInProgress.flags |= ShouldCapture;

So the net change for the branch I added is:

- attachPingListener(root, wakeable, rootRenderLanes);
workInProgress.flags |= ShouldCapture;

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh okay makes sense!

(workInProgress.mode & ConcurrentMode) === NoMode && } else {
workInProgress !== returnFiber
) {
workInProgress.flags |= DidCapture; workInProgress.flags |= DidCapture;
sourceFiber.flags |= ForceUpdateForLegacySuspense; sourceFiber.flags |= ForceUpdateForLegacySuspense;


Expand Down Expand Up @@ -362,11 +370,9 @@ function throwException(
// The source fiber did not complete. Mark it with Sync priority to // The source fiber did not complete. Mark it with Sync priority to
// indicate that it still has pending work. // indicate that it still has pending work.
sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane); sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);

}
// Exit without suspending.
return; return;
} }

// Confirmed that the boundary is in a concurrent mode tree. Continue // Confirmed that the boundary is in a concurrent mode tree. Continue
// with the normal suspend path. // with the normal suspend path.
// //
Expand Down Expand Up @@ -415,7 +421,6 @@ function throwException(
// TODO: I think we can remove this, since we now use `DidCapture` in // TODO: I think we can remove this, since we now use `DidCapture` in
// the begin phase to prevent an early bailout. // the begin phase to prevent an early bailout.
workInProgress.lanes = rootRenderLanes; workInProgress.lanes = rootRenderLanes;

return; return;
} }
// This boundary already captured during this render. Continue to the next // This boundary already captured during this render. Continue to the next
Expand Down
39 changes: 22 additions & 17 deletions packages/react-reconciler/src/ReactFiberThrow.old.js
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -297,23 +297,31 @@ function throwException(
wakeables.add(wakeable); wakeables.add(wakeable);
} }


if ((workInProgress.mode & ConcurrentMode) === NoMode) {
// Legacy Mode Suspense
//
// If the boundary is in legacy mode, we should *not* // If the boundary is in legacy mode, we should *not*
// suspend the commit. Pretend as if the suspended component rendered // suspend the commit. Pretend as if the suspended component rendered
// null and keep rendering. In the commit phase, we'll schedule a // null and keep rendering. When the Suspense boundary completes,
// subsequent synchronous update to re-render the Suspense. // we'll do a second pass to render the fallback.
if (workInProgress === returnFiber) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the special branch that I moved. The rest of the diff is updating comments that referred to outdated implementation details.

// Special case where we suspended while reconciling the children of
// a Suspense boundary's inner Offscreen wrapper fiber. This happens
// when a React.lazy component is a direct child of a
// Suspense boundary.
// //
// Note: It doesn't matter whether the component that suspended was // Suspense boundaries are implemented as multiple fibers, but they
// inside a concurrent mode tree. If the Suspense is outside of it, we // are a single conceptual unit. The legacy mode behavior where we
// should *not* suspend the commit. // pretend the suspended fiber committed as `null` won't work,
// because in this case the "suspended" fiber is the inner
// Offscreen wrapper.
// //
// If the suspense boundary suspended itself suspended, we don't have to // Because the contents of the boundary haven't started rendering
// do this trick because nothing was partially started. We can just // yet (i.e. nothing in the tree has partially rendered) we can
// directly do a second pass over the fallback in this render and // switch to the regular, concurrent mode behavior: mark the
// pretend we meant to render that directly. // boundary with ShouldCapture and enter the unwind phase.
if ( workInProgress.flags |= ShouldCapture;
(workInProgress.mode & ConcurrentMode) === NoMode && } else {
workInProgress !== returnFiber
) {
workInProgress.flags |= DidCapture; workInProgress.flags |= DidCapture;
sourceFiber.flags |= ForceUpdateForLegacySuspense; sourceFiber.flags |= ForceUpdateForLegacySuspense;


Expand Down Expand Up @@ -362,11 +370,9 @@ function throwException(
// The source fiber did not complete. Mark it with Sync priority to // The source fiber did not complete. Mark it with Sync priority to
// indicate that it still has pending work. // indicate that it still has pending work.
sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane); sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);

}
// Exit without suspending.
return; return;
} }

// Confirmed that the boundary is in a concurrent mode tree. Continue // Confirmed that the boundary is in a concurrent mode tree. Continue
// with the normal suspend path. // with the normal suspend path.
// //
Expand Down Expand Up @@ -415,7 +421,6 @@ function throwException(
// TODO: I think we can remove this, since we now use `DidCapture` in // TODO: I think we can remove this, since we now use `DidCapture` in
// the begin phase to prevent an early bailout. // the begin phase to prevent an early bailout.
workInProgress.lanes = rootRenderLanes; workInProgress.lanes = rootRenderLanes;

return; return;
} }
// This boundary already captured during this render. Continue to the next // This boundary already captured during this render. Continue to the next
Expand Down