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] Don't work on scheduled tasks while we're in an async commit but flush it eagerly if we're sync #31987

Merged
merged 12 commits into from
Jan 6, 2025

Conversation

sebmarkbage
Copy link
Collaborator

This is a follow up to #31930 and a prerequisite for #31975.

With View Transitions, the commit phase becomes async which means that other work can sneak in between. We need to be resilient to that.

This PR first refactors the flushMutationEffects and flushLayoutEffects to use module scope variables to track its arguments so we can defer them. It shares these with how we were already doing it for flushPendingEffects.

We also track how far along the commit phase we are so we know what we have left to flush.

Then callers of flushPassiveEffects become flushPendingEffects. That helper synchronously flushes any remaining phases we've yet to commit. That ensure that things are at least consistent if that happens.

Finally, when we are using a scheduled task, we don't do any work. This ensures that we're not flushing any work too early if we could've deferred it. This still ensures that we always do flush it before starting any new work on any root so new roots observe the committed state.

There are some unfortunate effects that could happen from allowing things to flush eagerly. Such as if a flushSync sneaks in before startViewTransition, it'll skip the animation. If it's during a suspensey font it'll start the transition before the font has loaded which might be better than breaking flushSync. It'll also potentially flush passive effects inside the startViewTransition which should typically be ok.

Copy link

vercel bot commented Jan 6, 2025

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 Jan 6, 2025 4:25pm

@react-sizebot
Copy link

react-sizebot commented Jan 6, 2025

Comparing: 3ce77d5...7adb0b2

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 +0.16% 512.56 kB 513.40 kB +0.12% 91.61 kB 91.72 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js +0.16% 517.34 kB 518.18 kB +0.12% 92.46 kB 92.57 kB
facebook-www/ReactDOM-prod.classic.js +0.18% 594.36 kB 595.45 kB +0.12% 104.68 kB 104.81 kB
facebook-www/ReactDOM-prod.modern.js +0.19% 584.62 kB 585.72 kB +0.11% 103.14 kB 103.25 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.production.js +0.27% 311.69 kB 312.53 kB +0.35% 54.57 kB 54.76 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.production.js +0.27% 311.76 kB 312.61 kB +0.35% 54.59 kB 54.78 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.production.js +0.27% 311.93 kB 312.78 kB +0.35% 54.62 kB 54.81 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.development.js +0.24% 552.27 kB 553.57 kB +0.33% 89.71 kB 90.01 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.development.js +0.24% 552.30 kB 553.60 kB +0.33% 89.72 kB 90.02 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.development.js +0.24% 552.34 kB 553.64 kB +0.33% 89.73 kB 90.03 kB
facebook-www/ReactART-dev.modern.js +0.23% 639.04 kB 640.54 kB +0.29% 101.24 kB 101.53 kB
facebook-www/ReactART-prod.modern.js +0.23% 357.10 kB 357.94 kB +0.22% 59.80 kB 59.94 kB
facebook-www/ReactART-dev.classic.js +0.23% 648.92 kB 650.42 kB +0.26% 103.22 kB 103.49 kB
facebook-www/ReactART-prod.classic.js +0.23% 366.83 kB 367.66 kB +0.17% 61.40 kB 61.51 kB

Generated by 🚫 dangerJS against 424f0dd

@@ -3458,11 +3483,12 @@ function flushLayoutEffects(
if (rootDidHavePassiveEffects) {
// This commit has passive effects. Stash a reference to them. But don't
// schedule a callback until after flushing layout work.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like we can remove this comment block now. We certainly don't stash a reference at this point anymore. And the scheduling is now obvious by advancing the phase.

suspendedCommitReason,
completedRenderEndTime,
);
pendingEffectsStatus = PENDING_MUTATION_PHASE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it a problem that we potentially leave pendingEffectsRoot as non-nullabble but pendingEffectsStatus === NO_PENDING_EFFECTS when we throw in commitBeforeMutationEffects?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, not really because it'll just leak until we update anything again.

A bigger problem would be leaving pendingEffectsStatus as something else. Leaving PENDING_PASSIVE_PHASE doesn't really happen because it's immediately reset and even if it did it would try again. Leaving PENDING_MUTATION_PHASE or PENDING_LAYOUT_PHASE would stop any new update except sync which would be bad.

So I'll reset it in the beginning in case React itself throws so it doesn't get stuck.

@sebmarkbage sebmarkbage merged commit defffdb into facebook:main Jan 6, 2025
187 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 6, 2025
…but flush it eagerly if we're sync (#31987)

This is a follow up to #31930 and a prerequisite for #31975.

With View Transitions, the commit phase becomes async which means that
other work can sneak in between. We need to be resilient to that.

This PR first refactors the flushMutationEffects and flushLayoutEffects
to use module scope variables to track its arguments so we can defer
them. It shares these with how we were already doing it for
flushPendingEffects.

We also track how far along the commit phase we are so we know what we
have left to flush.

Then callers of flushPassiveEffects become flushPendingEffects. That
helper synchronously flushes any remaining phases we've yet to commit.
That ensure that things are at least consistent if that happens.

Finally, when we are using a scheduled task, we don't do any work. This
ensures that we're not flushing any work too early if we could've
deferred it. This still ensures that we always do flush it before
starting any new work on any root so new roots observe the committed
state.

There are some unfortunate effects that could happen from allowing
things to flush eagerly. Such as if a flushSync sneaks in before
startViewTransition, it'll skip the animation. If it's during a
suspensey font it'll start the transition before the font has loaded
which might be better than breaking flushSync. It'll also potentially
flush passive effects inside the startViewTransition which should
typically be ok.

DiffTrain build for [defffdb](defffdb)
github-actions bot pushed a commit that referenced this pull request Jan 6, 2025
…but flush it eagerly if we're sync (#31987)

This is a follow up to #31930 and a prerequisite for #31975.

With View Transitions, the commit phase becomes async which means that
other work can sneak in between. We need to be resilient to that.

This PR first refactors the flushMutationEffects and flushLayoutEffects
to use module scope variables to track its arguments so we can defer
them. It shares these with how we were already doing it for
flushPendingEffects.

We also track how far along the commit phase we are so we know what we
have left to flush.

Then callers of flushPassiveEffects become flushPendingEffects. That
helper synchronously flushes any remaining phases we've yet to commit.
That ensure that things are at least consistent if that happens.

Finally, when we are using a scheduled task, we don't do any work. This
ensures that we're not flushing any work too early if we could've
deferred it. This still ensures that we always do flush it before
starting any new work on any root so new roots observe the committed
state.

There are some unfortunate effects that could happen from allowing
things to flush eagerly. Such as if a flushSync sneaks in before
startViewTransition, it'll skip the animation. If it's during a
suspensey font it'll start the transition before the font has loaded
which might be better than breaking flushSync. It'll also potentially
flush passive effects inside the startViewTransition which should
typically be ok.

DiffTrain build for [defffdb](defffdb)
github-actions bot pushed a commit to code/lib-react that referenced this pull request Jan 6, 2025
…but flush it eagerly if we're sync (facebook#31987)

This is a follow up to facebook#31930 and a prerequisite for facebook#31975.

With View Transitions, the commit phase becomes async which means that
other work can sneak in between. We need to be resilient to that.

This PR first refactors the flushMutationEffects and flushLayoutEffects
to use module scope variables to track its arguments so we can defer
them. It shares these with how we were already doing it for
flushPendingEffects.

We also track how far along the commit phase we are so we know what we
have left to flush.

Then callers of flushPassiveEffects become flushPendingEffects. That
helper synchronously flushes any remaining phases we've yet to commit.
That ensure that things are at least consistent if that happens.

Finally, when we are using a scheduled task, we don't do any work. This
ensures that we're not flushing any work too early if we could've
deferred it. This still ensures that we always do flush it before
starting any new work on any root so new roots observe the committed
state.

There are some unfortunate effects that could happen from allowing
things to flush eagerly. Such as if a flushSync sneaks in before
startViewTransition, it'll skip the animation. If it's during a
suspensey font it'll start the transition before the font has loaded
which might be better than breaking flushSync. It'll also potentially
flush passive effects inside the startViewTransition which should
typically be ok.

DiffTrain build for [defffdb](facebook@defffdb)
github-actions bot pushed a commit to code/lib-react that referenced this pull request Jan 6, 2025
…but flush it eagerly if we're sync (facebook#31987)

This is a follow up to facebook#31930 and a prerequisite for facebook#31975.

With View Transitions, the commit phase becomes async which means that
other work can sneak in between. We need to be resilient to that.

This PR first refactors the flushMutationEffects and flushLayoutEffects
to use module scope variables to track its arguments so we can defer
them. It shares these with how we were already doing it for
flushPendingEffects.

We also track how far along the commit phase we are so we know what we
have left to flush.

Then callers of flushPassiveEffects become flushPendingEffects. That
helper synchronously flushes any remaining phases we've yet to commit.
That ensure that things are at least consistent if that happens.

Finally, when we are using a scheduled task, we don't do any work. This
ensures that we're not flushing any work too early if we could've
deferred it. This still ensures that we always do flush it before
starting any new work on any root so new roots observe the committed
state.

There are some unfortunate effects that could happen from allowing
things to flush eagerly. Such as if a flushSync sneaks in before
startViewTransition, it'll skip the animation. If it's during a
suspensey font it'll start the transition before the font has loaded
which might be better than breaking flushSync. It'll also potentially
flush passive effects inside the startViewTransition which should
typically be ok.

DiffTrain build for [defffdb](facebook@defffdb)
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.

5 participants