Cloned flag to avoid extra clones in persistent renderer #27647

kassens · 2023-11-03T20:59:59Z

Persistent renderers used the Update effect flag to check if a subtree needs to be cloned. In some cases, that causes extra renders, such as when a layout effect is triggered which only has an effect on the JS side, but doesn't update the host components.

It's been a bit tricky to find the right places where this needs to be set and I'm not 100% sure I got all the cases even though the tests passed.

acdlite · 2023-11-04T00:25:48Z

packages/react-reconciler/src/ReactFiberCompleteWork.js

  // then we only have to check the `completedWork.subtreeFlags`.
  let child = completedWork.child;
  while (child !== null) {
    if (
-      (child.flags & MutationMask) !== NoFlags ||
-      (child.subtreeFlags & MutationMask) !== NoFlags
+      (child.flags & (Cloned | Visibility | Placement)) !== NoFlags ||


this is the function where I would put the roll out flag

acdlite · 2023-11-04T00:28:25Z

packages/react-reconciler/src/ReactFiberFlags.js

-/* Skipped value:                                 0b0000000000000000000000001000; */
+export const Update = /*                       */ 0b00000000000000000000000000100;
+export const Cloned = /*                       */ 0b10000000000000000000000000000;
+/* Skipped value:                                 0b00000000000000000000000001000; */


Could we just use the skipped value? Can't think of any reason why not. I'm guessing this is the old Deletion flag that was replaced by ChildDeletion and then removed

It was introduced in d69b2cf reverting some changes to the Flags. Some versioning issues with DevTools that I don't fully understand. Maybe it's okay when the devtools have since auto-updated to a newer version?

cc @hoxyq in case you know if this would be safe?

react-devtools-shared/src/backend/ReactFiberFlags.js was removed in https://github.com/facebook/react/pull/26542/files, so we could mirror the whole function implementation, instead of just mirroring flags.

We should update these flags for DevTools accordingly, like here, for example: https://github.com/facebook/react/blame/main/packages/react-devtools-shared/src/backend/renderer.js#L1515.

I think it should be safe to use skipped value here, because DevTools were not depending on this flag at any time, AFAIK. I can help with testing these changes if you end up with using skipped value.

react-sizebot · 2024-03-06T23:38:34Z

Comparing: 56dbd58...966936b

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	+0.11%	1.82 kB	1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	500.39 kB	500.39 kB	=	89.78 kB	89.78 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	=	507.52 kB	507.52 kB	=	90.95 kB	90.95 kB
facebook-www/ReactDOM-prod.classic.js	=	595.41 kB	595.41 kB	=	105.58 kB	105.58 kB
facebook-www/ReactDOM-prod.modern.js	=	571.71 kB	571.71 kB	=	101.78 kB	101.78 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
react-native/implementations/ReactFabric-prod.fb.js	+0.20%	370.95 kB	371.69 kB	+0.18%	64.92 kB	65.04 kB

Generated by 🚫 dangerJS against 13251f1

vercel · 2024-06-20T22:03:05Z

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	Jul 31, 2024 10:08pm

gnoff

Definitely out of my comfort zone so I can't approve but left some comments. I think there is a bug that might be causing cloned tags to be assigned more than they should (but maybe I misunderstand the mechanic)

gnoff · 2024-07-10T20:32:13Z

packages/react-reconciler/src/ReactFiberCompleteWork.js

@@ -473,6 +489,8 @@ function updateHostComponent(
      // Note that this might release a previous clone.
      workInProgress.stateNode = currentInstance;
      return;
+    } else {
+      markCloned(workInProgress);


is this right? seems like we should only mark clone if requiresClone is true

cloneInstance above returned a new instance since newInstance !== currentInstance in this branch, so the cloned tree needs to bubble up (I think)

gnoff · 2024-07-10T20:35:06Z

packages/react-reconciler/src/ReactFiberCompleteWork.js

+      if (!enablePersistedModeClonedFlag) {
+        // We'll have to mark it as having an effect, even though we won't use the effect for anything.
+        // This lets the parents know that at least one of their children has changed.
+        markUpdate(workInProgress);
+      }


This may be right but it's a little confusing that markCloned and markUpdate are not apparently mutually exclusive (when they actually are given the implementation of markCloned)

This is actually the core of the PR. We can have an Update for example due to effect dependencies changing, but do not want to trigger a clone up the tree indicated by the Cloned flag.

Previously they were both grouped together which means we'd do a full subtree clone if there was an effect dependency change in the subtree.

Even with the feature flag an effect change will still set the Update flag, but Update (behind flag) no longer causes a clone.

gnoff · 2024-07-10T20:47:17Z

packages/react-reconciler/src/ReactFiberCommitWork.js

-  if (parentFiber.subtreeFlags & MutationMask) {
+  if (
+    parentFiber.subtreeFlags &
+    (enablePersistedModeClonedFlag ? MutationMask | Cloned : MutationMask)


Should Cloned just be part of the MutationMask?

gnoff · 2024-07-10T20:52:03Z

packages/react-reconciler/src/ReactFiberCompleteWork.js

@@ -199,9 +211,12 @@ function doesRequireClone(current: null | Fiber, completedWork: Fiber) {
  // then we only have to check the `completedWork.subtreeFlags`.
  let child = completedWork.child;
  while (child !== null) {
+    const checkedFlags = enablePersistedModeClonedFlag
+      ? Cloned | Visibility | Placement


I guess this answers my question about why Cloned isn't just part of MutationMask. you specifically want to track it separately from general mutations like Update. You could still make it part of the Mask and just not use MutationMask here (as you are doing now). I guess the question is does Cloned generally count as a Mutation you need to visit during commit (yes) and if so then it semantically probably does belong in the mask.

josephsavona

I have similar questions to what @gnoff pointed out, but overall this makes sense. Have you tried syncing internally and running RN tests?

kassens

I'll run internal tests too now.

kassens · 2024-07-18T14:12:17Z

packages/react-reconciler/src/ReactFiberCompleteWork.js

@@ -473,6 +489,8 @@ function updateHostComponent(
      // Note that this might release a previous clone.
      workInProgress.stateNode = currentInstance;
      return;
+    } else {
+      markCloned(workInProgress);


cloneInstance above returned a new instance since newInstance !== currentInstance in this branch, so the cloned tree needs to bubble up (I think)

kassens · 2024-07-18T14:16:16Z

packages/react-reconciler/src/ReactFiberCompleteWork.js

+      if (!enablePersistedModeClonedFlag) {
+        // We'll have to mark it as having an effect, even though we won't use the effect for anything.
+        // This lets the parents know that at least one of their children has changed.
+        markUpdate(workInProgress);
+      }


This is actually the core of the PR. We can have an Update for example due to effect dependencies changing, but do not want to trigger a clone up the tree indicated by the Cloned flag.

Previously they were both grouped together which means we'd do a full subtree clone if there was an effect dependency change in the subtree.

Even with the feature flag an effect change will still set the Update flag, but Update (behind flag) no longer causes a clone.

josephsavona

This makes sense. Thanks for the thorough testing, let's move forward and iterate if necessary.

Persistent renderers used the `Update` effect flag to check if a subtree needs to be cloned. In some cases, that causes extra renders, such as when a layout effect is triggered which only has an effect on the JS side, but doesn't update the host components. It's been a bit tricky to find the right places where this needs to be set and I'm not 100% sure I got all the cases even though the tests passed. DiffTrain build for commit 5fb67fa.

) Persistent renderers used the `Update` effect flag to check if a subtree needs to be cloned. In some cases, that causes extra renders, such as when a layout effect is triggered which only has an effect on the JS side, but doesn't update the host components. It's been a bit tricky to find the right places where this needs to be set and I'm not 100% sure I got all the cases even though the tests passed. DiffTrain build for commit facebook@5fb67fa.

kassens requested review from acdlite and noahlemen November 3, 2023 20:59

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Nov 3, 2023

acdlite reviewed Nov 4, 2023

View reviewed changes

This was referenced Nov 9, 2023

Small cleanup in ReactFiberCompleteWork #27681

Merged

Split Update flag into a mutation and effect flag #27533

Closed

kassens force-pushed the persistent-update3 branch from aec7f84 to 8ef9bb2 Compare March 6, 2024 23:34

kassens force-pushed the persistent-update3 branch 2 times, most recently from 830f58f to 6c2c437 Compare March 20, 2024 20:56

kassens force-pushed the persistent-update3 branch from 6c2c437 to 6a9a322 Compare June 20, 2024 22:03

vercel bot deployed to Preview June 20, 2024 22:05 View deployment

kassens force-pushed the persistent-update3 branch from 6a9a322 to 10f326d Compare June 26, 2024 17:00

vercel bot deployed to Preview June 26, 2024 17:02 View deployment

kassens force-pushed the persistent-update3 branch from 10f326d to 1d14580 Compare June 26, 2024 17:51

vercel bot deployed to Preview June 26, 2024 17:53 View deployment

kassens force-pushed the persistent-update3 branch from 1d14580 to c73400f Compare June 26, 2024 20:14

vercel bot deployed to Preview June 26, 2024 20:16 View deployment

kassens requested a review from acdlite June 27, 2024 14:05

kassens changed the title ~~Add Cloned flag to avoid extra clones in persistent renderer~~ Cloned flag to avoid extra clones in persistent renderer Jul 1, 2024

gnoff reviewed Jul 10, 2024

View reviewed changes

josephsavona reviewed Jul 11, 2024

View reviewed changes

kassens commented Jul 18, 2024

View reviewed changes

kassens force-pushed the persistent-update3 branch from c73400f to b18abbb Compare July 18, 2024 14:17

vercel bot deployed to Preview July 18, 2024 14:20 View deployment

vercel bot deployed to Preview July 19, 2024 21:27 View deployment

add a Cloned flag to indicate a required clone in persistent mode

24abe67

add failing test case

ae601e1

kassens force-pushed the persistent-update3 branch from 263b12f to ae601e1 Compare July 31, 2024 21:41

vercel bot deployed to Preview July 31, 2024 21:46 View deployment

maybe break now?

51cac50

vercel bot deployed to Preview July 31, 2024 21:50 View deployment

add missing ChildDeletion flag check

13251f1

vercel bot deployed to Preview July 31, 2024 22:08 View deployment

josephsavona approved these changes Aug 1, 2024

View reviewed changes

kassens merged commit 5fb67fa into facebook:main Aug 1, 2024
185 checks passed

kassens deleted the persistent-update3 branch August 1, 2024 19:13

eps1lon mentioned this pull request Aug 6, 2024

Update React from 06d0b89e-20240801 to 187dd6a7-20240806 vercel/next.js#68594

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloned flag to avoid extra clones in persistent renderer #27647

Cloned flag to avoid extra clones in persistent renderer #27647

kassens commented Nov 3, 2023 •

edited

Loading

acdlite Nov 4, 2023

acdlite Nov 4, 2023

kassens Nov 7, 2023

hoxyq Nov 7, 2023

react-sizebot commented Mar 6, 2024 •

edited

Loading

vercel bot commented Jun 20, 2024 •

edited

Loading

gnoff left a comment

gnoff Jul 10, 2024

kassens Jul 18, 2024

gnoff Jul 10, 2024

kassens Jul 18, 2024

gnoff Jul 10, 2024

gnoff Jul 10, 2024

josephsavona left a comment

kassens left a comment

kassens Jul 18, 2024

kassens Jul 18, 2024

josephsavona left a comment

Cloned flag to avoid extra clones in persistent renderer #27647

Cloned flag to avoid extra clones in persistent renderer #27647

Conversation

kassens commented Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

react-sizebot commented Mar 6, 2024 • edited Loading

Critical size changes

Significant size changes

vercel bot commented Jun 20, 2024 • edited Loading

gnoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephsavona left a comment

Choose a reason for hiding this comment

kassens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephsavona left a comment

Choose a reason for hiding this comment

kassens commented Nov 3, 2023 •

edited

Loading

react-sizebot commented Mar 6, 2024 •

edited

Loading

vercel bot commented Jun 20, 2024 •

edited

Loading