From 24abe671bd02dee930cc49e18ee9601d720c79a5 Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Thu, 2 Nov 2023 18:30:23 -0400 Subject: [PATCH 1/4] add a Cloned flag to indicate a required clone in persistent mode --- .../__tests__/ReactFabric-test.internal.js | 124 ++++++++++++++++++ .../src/ReactFiberCommitWork.js | 7 +- .../src/ReactFiberCompleteWork.js | 45 +++++-- .../react-reconciler/src/ReactFiberFlags.js | 2 +- packages/shared/ReactFeatureFlags.js | 6 + .../ReactFeatureFlags.native-fb-dynamic.js | 1 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + ...actFeatureFlags.test-renderer.native-fb.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 2 + 12 files changed, 180 insertions(+), 12 deletions(-) diff --git a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js index 99d21ea933ecd..d55be5efd24bf 100644 --- a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js +++ b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js @@ -279,6 +279,130 @@ describe('ReactFabric', () => { expect(nativeFabricUIManager.completeRoot).toBeCalled(); }); + // @gate enablePersistedModeClonedFlag + it('should not clone nodes when layout effects are used', async () => { + const View = createReactNativeComponentClass('RCTView', () => ({ + validAttributes: {foo: true}, + uiViewClassName: 'RCTView', + })); + + const ComponentWithEffect = () => { + React.useLayoutEffect(() => {}); + return null; + }; + + await act(() => + ReactFabric.render( + + + , + 11, + ), + ); + expect(nativeFabricUIManager.completeRoot).toBeCalled(); + jest.clearAllMocks(); + + await act(() => + ReactFabric.render( + + + , + 11, + ), + ); + expect(nativeFabricUIManager.cloneNode).not.toBeCalled(); + expect(nativeFabricUIManager.cloneNodeWithNewChildren).not.toBeCalled(); + expect(nativeFabricUIManager.cloneNodeWithNewProps).not.toBeCalled(); + expect( + nativeFabricUIManager.cloneNodeWithNewChildrenAndProps, + ).not.toBeCalled(); + expect(nativeFabricUIManager.completeRoot).not.toBeCalled(); + }); + + // @gate enablePersistedModeClonedFlag + it('should not clone nodes when insertion effects are used', async () => { + const View = createReactNativeComponentClass('RCTView', () => ({ + validAttributes: {foo: true}, + uiViewClassName: 'RCTView', + })); + + const ComponentWithRef = () => { + React.useInsertionEffect(() => {}); + return null; + }; + + await act(() => + ReactFabric.render( + + + , + 11, + ), + ); + expect(nativeFabricUIManager.completeRoot).toBeCalled(); + jest.clearAllMocks(); + + await act(() => + ReactFabric.render( + + + , + 11, + ), + ); + expect(nativeFabricUIManager.cloneNode).not.toBeCalled(); + expect(nativeFabricUIManager.cloneNodeWithNewChildren).not.toBeCalled(); + expect(nativeFabricUIManager.cloneNodeWithNewProps).not.toBeCalled(); + expect( + nativeFabricUIManager.cloneNodeWithNewChildrenAndProps, + ).not.toBeCalled(); + expect(nativeFabricUIManager.completeRoot).not.toBeCalled(); + }); + + // @gate enablePersistedModeClonedFlag + it('should not clone nodes when useImperativeHandle is used', async () => { + const View = createReactNativeComponentClass('RCTView', () => ({ + validAttributes: {foo: true}, + uiViewClassName: 'RCTView', + })); + + const ComponentWithImperativeHandle = props => { + React.useImperativeHandle(props.ref, () => ({greet: () => 'hello'})); + return null; + }; + + const ref = React.createRef(); + + await act(() => + ReactFabric.render( + + + , + 11, + ), + ); + expect(nativeFabricUIManager.completeRoot).toBeCalled(); + expect(ref.current.greet()).toBe('hello'); + jest.clearAllMocks(); + + await act(() => + ReactFabric.render( + + + , + 11, + ), + ); + expect(nativeFabricUIManager.cloneNode).not.toBeCalled(); + expect(nativeFabricUIManager.cloneNodeWithNewChildren).not.toBeCalled(); + expect(nativeFabricUIManager.cloneNodeWithNewProps).not.toBeCalled(); + expect( + nativeFabricUIManager.cloneNodeWithNewChildrenAndProps, + ).not.toBeCalled(); + expect(nativeFabricUIManager.completeRoot).not.toBeCalled(); + expect(ref.current.greet()).toBe('hello'); + }); + it('should call dispatchCommand for native refs', async () => { const View = createReactNativeComponentClass('RCTView', () => ({ validAttributes: {foo: true}, diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 773e5d8f485d9..0f8fa68d0ca79 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -42,6 +42,7 @@ import type { import { alwaysThrottleRetries, enableCreateEventHandleAPI, + enablePersistedModeClonedFlag, enableProfilerTimer, enableProfilerCommitHooks, enableProfilerNestedUpdatePhase, @@ -98,6 +99,7 @@ import { ShouldSuspendCommit, MaySuspendCommit, FormReset, + Cloned, } from './ReactFiberFlags'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; import {runWithFiberInDEV} from './ReactCurrentFiber'; @@ -2554,7 +2556,10 @@ function recursivelyTraverseMutationEffects( } } - if (parentFiber.subtreeFlags & MutationMask) { + if ( + parentFiber.subtreeFlags & + (enablePersistedModeClonedFlag ? MutationMask | Cloned : MutationMask) + ) { let child = parentFiber.child; while (child !== null) { if (__DEV__) { diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index dd797b8d09717..3e53ec8037d9d 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -35,6 +35,7 @@ import { enableLegacyHidden, enableSuspenseCallback, enableScopeAPI, + enablePersistedModeClonedFlag, enableProfilerTimer, enableCache, enableTransitionTracing, @@ -90,6 +91,7 @@ import { MaySuspendCommit, ScheduleRetry, ShouldSuspendCommit, + Cloned, } from './ReactFiberFlags'; import { @@ -182,6 +184,16 @@ function markUpdate(workInProgress: Fiber) { workInProgress.flags |= Update; } +/** + * Tag the fiber with Cloned in persistent mode to signal that + * it received an update that requires a clone of the tree above. + */ +function markCloned(workInProgress: Fiber) { + if (supportsPersistence && enablePersistedModeClonedFlag) { + workInProgress.flags |= Cloned; + } +} + /** * In persistent mode, return whether this update needs to clone the subtree. */ @@ -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 + : MutationMask; if ( - (child.flags & MutationMask) !== NoFlags || - (child.subtreeFlags & MutationMask) !== NoFlags + (child.flags & checkedFlags) !== NoFlags || + (child.subtreeFlags & checkedFlags) !== NoFlags ) { return true; } @@ -450,6 +465,7 @@ function updateHostComponent( let newChildSet = null; if (requiresClone && passChildrenWhenCloningPersistedNodes) { + markCloned(workInProgress); newChildSet = createContainerChildSet(); // If children might have changed, we have to add them all to the set. appendAllChildrenToContainer( @@ -473,6 +489,8 @@ function updateHostComponent( // Note that this might release a previous clone. workInProgress.stateNode = currentInstance; return; + } else { + markCloned(workInProgress); } // Certain renderers require commit-time effects for initial mount. @@ -485,12 +503,14 @@ function updateHostComponent( } workInProgress.stateNode = newInstance; if (!requiresClone) { - // If there are no other effects in this tree, we need to flag this node as having one. - // Even though we're not going to use it for anything. - // Otherwise parents won't know that there are new children to propagate upwards. - markUpdate(workInProgress); + if (!enablePersistedModeClonedFlag) { + // If there are no other effects in this tree, we need to flag this node as having one. + // Even though we're not going to use it for anything. + // Otherwise parents won't know that there are new children to propagate upwards. + markUpdate(workInProgress); + } } else if (!passChildrenWhenCloningPersistedNodes) { - // If children might have changed, we have to add them all to the set. + // If children have changed, we have to add them all to the set. appendAllChildren( newInstance, workInProgress, @@ -618,15 +638,18 @@ function updateHostText( // If the text content differs, we'll create a new text instance for it. const rootContainerInstance = getRootHostContainer(); const currentHostContext = getHostContext(); + markCloned(workInProgress); workInProgress.stateNode = createTextInstance( newText, rootContainerInstance, currentHostContext, workInProgress, ); - // 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); + 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); + } } else { workInProgress.stateNode = current.stateNode; } @@ -1229,6 +1252,7 @@ function completeWork( ); // TODO: For persistent renderers, we should pass children as part // of the initial instance creation + markCloned(workInProgress); appendAllChildren(instance, workInProgress, false, false); workInProgress.stateNode = instance; @@ -1284,6 +1308,7 @@ function completeWork( if (wasHydrated) { prepareToHydrateHostTextInstance(workInProgress); } else { + markCloned(workInProgress); workInProgress.stateNode = createTextInstance( newText, rootContainerInstance, diff --git a/packages/react-reconciler/src/ReactFiberFlags.js b/packages/react-reconciler/src/ReactFiberFlags.js index 49aebe2f9b11c..66e9249f15026 100644 --- a/packages/react-reconciler/src/ReactFiberFlags.js +++ b/packages/react-reconciler/src/ReactFiberFlags.js @@ -20,7 +20,7 @@ export const Hydrating = /* */ 0b0000000000000001000000000000 // You can change the rest (and add more). export const Update = /* */ 0b0000000000000000000000000100; -/* Skipped value: 0b0000000000000000000000001000; */ +export const Cloned = /* */ 0b0000000000000000000000001000; export const ChildDeletion = /* */ 0b0000000000000000000000010000; export const ContentReset = /* */ 0b0000000000000000000000100000; diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 190f2b6b70335..f33391b36e5c6 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -134,6 +134,12 @@ export const passChildrenWhenCloningPersistedNodes = false; export const enableServerComponentLogs = __EXPERIMENTAL__; +/** + * Enables a new Fiber flag used in persisted mode to reduce the number + * of cloned host components. + */ +export const enablePersistedModeClonedFlag = false; + export const enableAddPropertiesFastPath = false; export const enableOwnerStacks = __EXPERIMENTAL__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js index 14009ab6711ad..2426206bc825d 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js @@ -20,6 +20,7 @@ export const alwaysThrottleRetries = __VARIANT__; export const enableAddPropertiesFastPath = __VARIANT__; export const enableObjectFiber = __VARIANT__; +export const enablePersistedModeClonedFlag = __VARIANT__; export const enableShallowPropDiffing = __VARIANT__; export const passChildrenWhenCloningPersistedNodes = __VARIANT__; export const enableFabricCompleteRootInCommitPhase = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 2c680f7738eec..110c152b59400 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -23,6 +23,7 @@ export const { enableAddPropertiesFastPath, enableFabricCompleteRootInCommitPhase, enableObjectFiber, + enablePersistedModeClonedFlag, enableShallowPropDiffing, passChildrenWhenCloningPersistedNodes, enableLazyContextPropagation, diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 9f9c3698d3d85..737ecf01e9669 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -58,6 +58,7 @@ export const enableLegacyHidden = false; export const enableNoCloningMemoCache = false; export const enableObjectFiber = false; export const enableOwnerStacks = false; +export const enablePersistedModeClonedFlag = false; export const enablePostpone = false; export const enableReactTestRendererWarning = false; export const enableRefAsProp = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 2a75c9ad20e2d..eb6bef3b29754 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -70,6 +70,7 @@ export const enableAsyncActions = true; export const alwaysThrottleRetries = true; export const passChildrenWhenCloningPersistedNodes = false; +export const enablePersistedModeClonedFlag = false; export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__; export const disableClientCache = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js index f20f0e16a17d3..0436964f889d2 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -49,6 +49,7 @@ export const enableLegacyHidden = false; export const enableNoCloningMemoCache = false; export const enableObjectFiber = false; export const enableOwnerStacks = false; +export const enablePersistedModeClonedFlag = false; export const enablePostpone = false; export const enableProfilerCommitHooks = __PROFILE__; export const enableProfilerNestedUpdatePhase = __PROFILE__; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index b77f040c24c23..9ce67044464a8 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -73,6 +73,7 @@ export const enableAsyncActions = true; export const alwaysThrottleRetries = true; export const passChildrenWhenCloningPersistedNodes = false; +export const enablePersistedModeClonedFlag = false; export const enableUseDeferredValueInitialArg = true; export const disableClientCache = true; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 125d3c9c0e767..18fc5c8823d38 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -105,6 +105,8 @@ export const enableFizzExternalRuntime = true; export const passChildrenWhenCloningPersistedNodes = false; +export const enablePersistedModeClonedFlag = false; + export const enableAsyncDebugInfo = false; export const disableClientCache = true; From ae601e196cc4d0b3dc7cb1a230e07d2806b45676 Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Wed, 31 Jul 2024 17:41:17 -0400 Subject: [PATCH 2/4] add failing test case --- .../src/__tests__/ReactPersistent-test.js | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactPersistent-test.js b/packages/react-reconciler/src/__tests__/ReactPersistent-test.js index 7900ccdadd451..0d54f88cacc89 100644 --- a/packages/react-reconciler/src/__tests__/ReactPersistent-test.js +++ b/packages/react-reconciler/src/__tests__/ReactPersistent-test.js @@ -213,4 +213,26 @@ describe('ReactPersistent', () => { // The original is unchanged. expect(newPortalChildren).toEqual([div(span(), 'Hello ', 'World')]); }); + + it('remove children', async () => { + function Wrapper({children}) { + return children; + } + + const root = ReactNoopPersistent.createRoot(); + root.render( + + + , + ); + waitForAll([]); + + expect(root.getChildrenAsJSX()).toEqual(); + + console.log('#### SECOND RENDER ####'); + root.render(); + waitForAll([]); + + expect(root.getChildrenAsJSX()).toEqual(null); + }); }); From 51cac50a5fd0fa1a6cfd46d395744bf262a38deb Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Wed, 31 Jul 2024 17:47:35 -0400 Subject: [PATCH 3/4] maybe break now? --- .../src/__tests__/ReactPersistent-test.js | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactPersistent-test.js b/packages/react-reconciler/src/__tests__/ReactPersistent-test.js index 0d54f88cacc89..dba30d6d267bd 100644 --- a/packages/react-reconciler/src/__tests__/ReactPersistent-test.js +++ b/packages/react-reconciler/src/__tests__/ReactPersistent-test.js @@ -12,6 +12,8 @@ let React; let ReactNoopPersistent; + +let act; let waitForAll; describe('ReactPersistent', () => { @@ -20,8 +22,7 @@ describe('ReactPersistent', () => { React = require('react'); ReactNoopPersistent = require('react-noop-renderer/persistent'); - const InternalTestUtils = require('internal-test-utils'); - waitForAll = InternalTestUtils.waitForAll; + ({act, waitForAll} = require('internal-test-utils')); }); // Inlined from shared folder so we can run this test on a bundle. @@ -220,19 +221,18 @@ describe('ReactPersistent', () => { } const root = ReactNoopPersistent.createRoot(); - root.render( - - - , - ); - waitForAll([]); - + await act(() => { + root.render( + + + , + ); + }); expect(root.getChildrenAsJSX()).toEqual(); - console.log('#### SECOND RENDER ####'); - root.render(); - waitForAll([]); - + await act(() => { + root.render(); + }); expect(root.getChildrenAsJSX()).toEqual(null); }); }); From 13251f18a6cfbf7a0651b5b966f9685dd686142a Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Wed, 31 Jul 2024 17:44:55 -0400 Subject: [PATCH 4/4] add missing ChildDeletion flag check --- packages/react-reconciler/src/ReactFiberCompleteWork.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 3e53ec8037d9d..c5d0c1575be04 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -212,7 +212,7 @@ function doesRequireClone(current: null | Fiber, completedWork: Fiber) { let child = completedWork.child; while (child !== null) { const checkedFlags = enablePersistedModeClonedFlag - ? Cloned | Visibility | Placement + ? Cloned | Visibility | Placement | ChildDeletion : MutationMask; if ( (child.flags & checkedFlags) !== NoFlags ||