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.

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

[Fiber] Schedule client renders using non-hydration lane #31776

Merged
merged 2 commits into from
Dec 14, 2024

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Dec 14, 2024

Related to #31752.

When hydrating, we have two different ways of handling a Suspense boundary that the server has already given up on and decided to client render. If we have already hydrated the parent and then later this happens, then we'll use the retry lane like any ping. If we discover that it was already in client-render mode when we discover the Suspense boundary for the first time, then schedule a default lane to let us first finish the current render and then upgrade the priority to sync to try to client render this boundary as soon as possible since we're holding back content.

We used to use the DefaultHydrationLane for this but this is not really a Hydration. It's actually a client render. If we get any other updates flowing in from above at the same time we might as well do them in the same pass instead of two passes. So this should be considered more like any update.

This also means that visually the client render pass now gets painted as a render instead of a hydration.

This show the flow of a shell being hydrated at the default priority, then a Suspense boundary being discovered and hydrated at Idle and then an inner boundary being discovered as client rendered which gets upgraded to default.

Screenshot 2024-12-14 at 12 13 57 AM

@sebmarkbage sebmarkbage requested a review from acdlite December 14, 2024 05:22
Copy link

vercel bot commented Dec 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 14, 2024 4:48pm

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 14, 2024
// Schedule a normal pri update to render this content.
workInProgress.lanes = laneToLanes(DefaultHydrationLane);
workInProgress.lanes = laneToLanes(
enableHydrationLaneScheduling ? DefaultLane : DefaultHydrationLane,
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Dec 14, 2024

Choose a reason for hiding this comment

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

I use the same kill switch even though this case is using the hydration lane less than before. Unlike the other change which is the inverse of this in the cases where it is doing hydration.

Copy link
Member

Choose a reason for hiding this comment

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

Will this lower the priority below other things hydrating (maybe that's the intent) or is there a timeout or something that forces 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 sync unification is not complete so the semantics there are kind of nonsensical atm in edge cases.

In general in the model I don't think it should be possible to have other work on the default hydration lane already since that would've already been done if we're hydrating at lower priority or if there is some then that's because we're currently doing it.

The only thing is that it might be batched with other default lane work like updates which might delay the commit but that's also kind of the point that if that work interleaves with this work then we shouldn't have to rewind.

@react-sizebot
Copy link

react-sizebot commented Dec 14, 2024

Comparing: 2e25ee3...abe81bf

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 510.72 kB 510.72 kB = 91.48 kB 91.48 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB +0.05% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 515.66 kB 515.66 kB = 92.19 kB 92.19 kB
facebook-www/ReactDOM-prod.classic.js = 595.04 kB 595.04 kB = 105.00 kB 105.00 kB
facebook-www/ReactDOM-prod.modern.js = 585.31 kB 585.31 kB = 103.44 kB 103.44 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 49c9946

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

LGTM but @acdlite would be a better reviewer

@sebmarkbage sebmarkbage merged commit d1dd7fe into facebook:main Dec 14, 2024
187 checks passed
github-actions bot pushed a commit that referenced this pull request Dec 14, 2024
Related to #31752.

When hydrating, we have two different ways of handling a Suspense
boundary that the server has already given up on and decided to client
render. If we have already hydrated the parent and then later this
happens, then we'll use the retry lane like any ping. If we discover
that it was already in client-render mode when we discover the Suspense
boundary for the first time, then schedule a default lane to let us
first finish the current render and then upgrade the priority to sync to
try to client render this boundary as soon as possible since we're
holding back content.

We used to use the `DefaultHydrationLane` for this but this is not
really a Hydration. It's actually a client render. If we get any other
updates flowing in from above at the same time we might as well do them
in the same pass instead of two passes. So this should be considered
more like any update.

This also means that visually the client render pass now gets painted as
a render instead of a hydration.

This show the flow of a shell being hydrated at the default priority,
then a Suspense boundary being discovered and hydrated at Idle and then
an inner boundary being discovered as client rendered which gets
upgraded to default.

<img width="1363" alt="Screenshot 2024-12-14 at 12 13 57 AM"
src="https://github.com/user-attachments/assets/a141133e-4856-4f38-a11f-f26bd00b6245"
/>

DiffTrain build for [d1dd7fe](d1dd7fe)
github-actions bot pushed a commit that referenced this pull request Dec 14, 2024
Related to #31752.

When hydrating, we have two different ways of handling a Suspense
boundary that the server has already given up on and decided to client
render. If we have already hydrated the parent and then later this
happens, then we'll use the retry lane like any ping. If we discover
that it was already in client-render mode when we discover the Suspense
boundary for the first time, then schedule a default lane to let us
first finish the current render and then upgrade the priority to sync to
try to client render this boundary as soon as possible since we're
holding back content.

We used to use the `DefaultHydrationLane` for this but this is not
really a Hydration. It's actually a client render. If we get any other
updates flowing in from above at the same time we might as well do them
in the same pass instead of two passes. So this should be considered
more like any update.

This also means that visually the client render pass now gets painted as
a render instead of a hydration.

This show the flow of a shell being hydrated at the default priority,
then a Suspense boundary being discovered and hydrated at Idle and then
an inner boundary being discovered as client rendered which gets
upgraded to default.

<img width="1363" alt="Screenshot 2024-12-14 at 12 13 57 AM"
src="https://github.com/user-attachments/assets/a141133e-4856-4f38-a11f-f26bd00b6245"
/>

DiffTrain build for [d1dd7fe](d1dd7fe)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants