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

[pull] main from facebook:main #40

Merged
merged 3 commits into from
Dec 15, 2024
Merged

[pull] main from facebook:main #40

merged 3 commits into from
Dec 15, 2024

Conversation

pull[bot]
Copy link

@pull pull bot commented Dec 15, 2024

See Commits and Changes for more details.


Created by pull[bot] (v2.0.0-alpha.1)

Can you help keep this open source service alive? 💖 Please sponsor : )

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"
/>
This highlights the render phase as the tertiary color (green) when
we're render a hydration lane or offscreen lane.

I call the "Render" phase "Hydrated" instead in this case. For the
offscreen case we don't currently have a differentiation between
hydrated or activity. I just called that "Prepared". Even for the
hydration case where there's no discovered client rendered boundaries
it's more like it's preparing for an interaction rather than blocking
one. Where as for the other lanes the hydration might block something.

<img width="1173" alt="Screenshot 2024-12-12 at 11 23 14 PM"
src="https://github.com/user-attachments/assets/49ab1508-840f-4188-a085-18fe94b14187"
/>

In a follow up I'd like to color the components in the Components tree
green if they were hydrated but not the ones that was actually client
rendered e.g. due to a mismatch or forced client rendering so you can
tell the difference. Unfortunately, the current signals we have for this
get reset earlier in the commit phase than when we log these.

Another thing is that a failed hydration should probably be colored red
even though it ends up committing successfully. I.e. a recoverable
error.
When implementing passive effects we did a pretty massive oversight.
While the passive effect is scheduled into its own scheduler task, the
scheduler doesn't always yield to the browser if it has time left. That
means that if you have a fast commit phase, it might try to squeeze in
the passive effects in the same frame but those then might end being
very heavy.

We had `requestPaint()` for this but that was only implemented for the
`isInputPending` experiment. It wasn't thought we needed it for the
regular scheduler because it yields "every frame" anyway - but it
doesn't yield every task. While the `isInputPending` experiment showed
that it wasn't actually any significant impact, and it was better to
keep shorter yield time anyway. Which is why we deleted the code.
Whatever small win it did see in some cases might have been actually due
to this issue rather than anything to do with `isInputPending` at all.

As you can see in #31782 we do
have this implemented in the mock scheduler and a lot of behavior that
we assert assumes that this works.

So this just implements yielding after `requestPaint` is called.

Before:

<img width="1023" alt="Screenshot 2024-12-14 at 3 40 24 PM"
src="https://github.com/user-attachments/assets/d60f4bb2-c8f8-4f91-a402-9ac25b278450"
/>

After:

<img width="1108" alt="Screenshot 2024-12-14 at 3 41 25 PM"
src="https://github.com/user-attachments/assets/170cdb90-a049-436f-9501-be3fb9bc04ca"
/>

Notice how in after the native task is split into two. It might not
always actually paint and the native scheduler might make the same
mistake and think it has enough time left but it's at least less likely
to.

We do have another way to do this. When we yield a continuation we also
yield to the native browser. This is to enable the Suspense Optimization
(currently disabled) to work. We could do the same for passive effects
and, in fact, I have a branch that does but because that requires a lot
more tests to be fixed it's a lot more invasive of a change. The nice
thing about this approach is that this is not even running in tests at
all and the tests we do have assert that this is the behavior already. 😬
@pull pull bot added the ⤵️ pull label Dec 15, 2024
@pull pull bot merged commit c80b336 into code:main Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants