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: 8416ebe...c73400f

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.05%	1.83 kB	1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	497.93 kB	497.93 kB	=	89.26 kB	89.26 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	=	502.75 kB	502.75 kB	=	89.96 kB	89.96 kB
facebook-www/ReactDOM-prod.classic.js	=	597.10 kB	597.10 kB	=	105.31 kB	105.31 kB
facebook-www/ReactDOM-prod.modern.js	=	571.44 kB	571.44 kB	=	101.24 kB	101.24 kB
test_utils/ReactAllWarnings.js	Deleted	62.88 kB	0.00 kB	Deleted	15.69 kB	0.00 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%	365.56 kB	366.30 kB	+0.15%	64.23 kB	64.32 kB
test_utils/ReactAllWarnings.js	Deleted	62.88 kB	0.00 kB	Deleted	15.69 kB	0.00 kB

Generated by 🚫 dangerJS against c73400f

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	Jun 26, 2024 8:16pm

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

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)

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

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

c73400f

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~~ Jul 1, 2024

gnoff reviewed Jul 10, 2024

View reviewed changes

josephsavona reviewed Jul 11, 2024

View reviewed changes

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

gnoff Jul 10, 2024

gnoff Jul 10, 2024

gnoff Jul 10, 2024

josephsavona left a comment

Cloned flag to avoid extra clones in persistent renderer #27647

Are you sure you want to change the base?

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

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