@samitouri / QOS-React / commits / 5fb67fa25c

Cloned flag to avoid extra clones in persistent renderer (#27647)

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.

Jan Kassens committed Aug 1, 2024 at 15:11 UTC 5fb67fa25c4ea8be046c6d9af41047f3cc379279
13 files changed +204 -14
packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
+124
@@ -279,6 +279,130 @@ describe('ReactFabric', () => {
279 expect(nativeFabricUIManager.completeRoot).toBeCalled();
280 });
281
282 + // @gate enablePersistedModeClonedFlag
283 + it('should not clone nodes when layout effects are used', async () => {
284 + const View = createReactNativeComponentClass('RCTView', () => ({
285 + validAttributes: {foo: true},
286 + uiViewClassName: 'RCTView',
287 + }));
288 +
289 + const ComponentWithEffect = () => {
290 + React.useLayoutEffect(() => {});
291 + return null;
292 + };
293 +
294 + await act(() =>
295 + ReactFabric.render(
296 + <View>
297 + <ComponentWithEffect />
298 + </View>,
299 + 11,
300 + ),
301 + );
302 + expect(nativeFabricUIManager.completeRoot).toBeCalled();
303 + jest.clearAllMocks();
304 +
305 + await act(() =>
306 + ReactFabric.render(
307 + <View>
308 + <ComponentWithEffect />
309 + </View>,
310 + 11,
311 + ),
312 + );
313 + expect(nativeFabricUIManager.cloneNode).not.toBeCalled();
314 + expect(nativeFabricUIManager.cloneNodeWithNewChildren).not.toBeCalled();
315 + expect(nativeFabricUIManager.cloneNodeWithNewProps).not.toBeCalled();
316 + expect(
317 + nativeFabricUIManager.cloneNodeWithNewChildrenAndProps,
318 + ).not.toBeCalled();
319 + expect(nativeFabricUIManager.completeRoot).not.toBeCalled();
320 + });
321 +
322 + // @gate enablePersistedModeClonedFlag
323 + it('should not clone nodes when insertion effects are used', async () => {
324 + const View = createReactNativeComponentClass('RCTView', () => ({
325 + validAttributes: {foo: true},
326 + uiViewClassName: 'RCTView',
327 + }));
328 +
329 + const ComponentWithRef = () => {
330 + React.useInsertionEffect(() => {});
331 + return null;
332 + };
333 +
334 + await act(() =>
335 + ReactFabric.render(
336 + <View>
337 + <ComponentWithRef />
338 + </View>,
339 + 11,
340 + ),
341 + );
342 + expect(nativeFabricUIManager.completeRoot).toBeCalled();
343 + jest.clearAllMocks();
344 +
345 + await act(() =>
346 + ReactFabric.render(
347 + <View>
348 + <ComponentWithRef />
349 + </View>,
350 + 11,
351 + ),
352 + );
353 + expect(nativeFabricUIManager.cloneNode).not.toBeCalled();
354 + expect(nativeFabricUIManager.cloneNodeWithNewChildren).not.toBeCalled();
355 + expect(nativeFabricUIManager.cloneNodeWithNewProps).not.toBeCalled();
356 + expect(
357 + nativeFabricUIManager.cloneNodeWithNewChildrenAndProps,
358 + ).not.toBeCalled();
359 + expect(nativeFabricUIManager.completeRoot).not.toBeCalled();
360 + });
361 +
362 + // @gate enablePersistedModeClonedFlag
363 + it('should not clone nodes when useImperativeHandle is used', async () => {
364 + const View = createReactNativeComponentClass('RCTView', () => ({
365 + validAttributes: {foo: true},
366 + uiViewClassName: 'RCTView',
367 + }));
368 +
369 + const ComponentWithImperativeHandle = props => {
370 + React.useImperativeHandle(props.ref, () => ({greet: () => 'hello'}));
371 + return null;
372 + };
373 +
374 + const ref = React.createRef();
375 +
376 + await act(() =>
377 + ReactFabric.render(
378 + <View>
379 + <ComponentWithImperativeHandle ref={ref} />
380 + </View>,
381 + 11,
382 + ),
383 + );
384 + expect(nativeFabricUIManager.completeRoot).toBeCalled();
385 + expect(ref.current.greet()).toBe('hello');
386 + jest.clearAllMocks();
387 +
388 + await act(() =>
389 + ReactFabric.render(
390 + <View>
391 + <ComponentWithImperativeHandle ref={ref} />
392 + </View>,
393 + 11,
394 + ),
395 + );
396 + expect(nativeFabricUIManager.cloneNode).not.toBeCalled();
397 + expect(nativeFabricUIManager.cloneNodeWithNewChildren).not.toBeCalled();
398 + expect(nativeFabricUIManager.cloneNodeWithNewProps).not.toBeCalled();
399 + expect(
400 + nativeFabricUIManager.cloneNodeWithNewChildrenAndProps,
401 + ).not.toBeCalled();
402 + expect(nativeFabricUIManager.completeRoot).not.toBeCalled();
403 + expect(ref.current.greet()).toBe('hello');
404 + });
405 +
406 it('should call dispatchCommand for native refs', async () => {
407 const View = createReactNativeComponentClass('RCTView', () => ({
408 validAttributes: {foo: true},
packages/react-reconciler/src/ReactFiberCommitWork.js
+6 -1
@@ -42,6 +42,7 @@ import type {
42 import {
43 alwaysThrottleRetries,
44 enableCreateEventHandleAPI,
45 + enablePersistedModeClonedFlag,
46 enableProfilerTimer,
47 enableProfilerCommitHooks,
48 enableProfilerNestedUpdatePhase,
@@ -98,6 +99,7 @@ import {
99 ShouldSuspendCommit,
100 MaySuspendCommit,
101 FormReset,
102 + Cloned,
103 } from './ReactFiberFlags';
104 import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
105 import {runWithFiberInDEV} from './ReactCurrentFiber';
@@ -2554,7 +2556,10 @@ function recursivelyTraverseMutationEffects(
2556 }
2557 }
2558
2557 - if (parentFiber.subtreeFlags & MutationMask) {
2559 + if (
2560 + parentFiber.subtreeFlags &
2561 + (enablePersistedModeClonedFlag ? MutationMask | Cloned : MutationMask)
2562 + ) {
2563 let child = parentFiber.child;
2564 while (child !== null) {
2565 if (__DEV__) {
packages/react-reconciler/src/ReactFiberCompleteWork.js
+35 -10
@@ -35,6 +35,7 @@ import {
35 enableLegacyHidden,
36 enableSuspenseCallback,
37 enableScopeAPI,
38 + enablePersistedModeClonedFlag,
39 enableProfilerTimer,
40 enableCache,
41 enableTransitionTracing,
@@ -90,6 +91,7 @@ import {
91 MaySuspendCommit,
92 ScheduleRetry,
93 ShouldSuspendCommit,
94 + Cloned,
95 } from './ReactFiberFlags';
96
97 import {
@@ -182,6 +184,16 @@ function markUpdate(workInProgress: Fiber) {
184 workInProgress.flags |= Update;
185 }
186
187 +/**
188 + * Tag the fiber with Cloned in persistent mode to signal that
189 + * it received an update that requires a clone of the tree above.
190 + */
191 +function markCloned(workInProgress: Fiber) {
192 + if (supportsPersistence && enablePersistedModeClonedFlag) {
193 + workInProgress.flags |= Cloned;
194 + }
195 +}
196 +
197 /**
198 * In persistent mode, return whether this update needs to clone the subtree.
199 */
@@ -199,9 +211,12 @@ function doesRequireClone(current: null | Fiber, completedWork: Fiber) {
211 // then we only have to check the `completedWork.subtreeFlags`.
212 let child = completedWork.child;
213 while (child !== null) {
214 + const checkedFlags = enablePersistedModeClonedFlag
215 + ? Cloned | Visibility | Placement | ChildDeletion
216 + : MutationMask;
217 if (
203 - (child.flags & MutationMask) !== NoFlags ||
204 - (child.subtreeFlags & MutationMask) !== NoFlags
218 + (child.flags & checkedFlags) !== NoFlags ||
219 + (child.subtreeFlags & checkedFlags) !== NoFlags
220 ) {
221 return true;
222 }
@@ -450,6 +465,7 @@ function updateHostComponent(
465
466 let newChildSet = null;
467 if (requiresClone && passChildrenWhenCloningPersistedNodes) {
468 + markCloned(workInProgress);
469 newChildSet = createContainerChildSet();
470 // If children might have changed, we have to add them all to the set.
471 appendAllChildrenToContainer(
@@ -473,6 +489,8 @@ function updateHostComponent(
489 // Note that this might release a previous clone.
490 workInProgress.stateNode = currentInstance;
491 return;
492 + } else {
493 + markCloned(workInProgress);
494 }
495
496 // Certain renderers require commit-time effects for initial mount.
@@ -485,12 +503,14 @@ function updateHostComponent(
503 }
504 workInProgress.stateNode = newInstance;
505 if (!requiresClone) {
488 - // If there are no other effects in this tree, we need to flag this node as having one.
489 - // Even though we're not going to use it for anything.
490 - // Otherwise parents won't know that there are new children to propagate upwards.
491 - markUpdate(workInProgress);
506 + if (!enablePersistedModeClonedFlag) {
507 + // If there are no other effects in this tree, we need to flag this node as having one.
508 + // Even though we're not going to use it for anything.
509 + // Otherwise parents won't know that there are new children to propagate upwards.
510 + markUpdate(workInProgress);
511 + }
512 } else if (!passChildrenWhenCloningPersistedNodes) {
493 - // If children might have changed, we have to add them all to the set.
513 + // If children have changed, we have to add them all to the set.
514 appendAllChildren(
515 newInstance,
516 workInProgress,
@@ -618,15 +638,18 @@ function updateHostText(
638 // If the text content differs, we'll create a new text instance for it.
639 const rootContainerInstance = getRootHostContainer();
640 const currentHostContext = getHostContext();
641 + markCloned(workInProgress);
642 workInProgress.stateNode = createTextInstance(
643 newText,
644 rootContainerInstance,
645 currentHostContext,
646 workInProgress,
647 );
627 - // We'll have to mark it as having an effect, even though we won't use the effect for anything.
628 - // This lets the parents know that at least one of their children has changed.
629 - markUpdate(workInProgress);
648 + if (!enablePersistedModeClonedFlag) {
649 + // We'll have to mark it as having an effect, even though we won't use the effect for anything.
650 + // This lets the parents know that at least one of their children has changed.
651 + markUpdate(workInProgress);
652 + }
653 } else {
654 workInProgress.stateNode = current.stateNode;
655 }
@@ -1229,6 +1252,7 @@ function completeWork(
1252 );
1253 // TODO: For persistent renderers, we should pass children as part
1254 // of the initial instance creation
1255 + markCloned(workInProgress);
1256 appendAllChildren(instance, workInProgress, false, false);
1257 workInProgress.stateNode = instance;
1258
@@ -1284,6 +1308,7 @@ function completeWork(
1308 if (wasHydrated) {
1309 prepareToHydrateHostTextInstance(workInProgress);
1310 } else {
1311 + markCloned(workInProgress);
1312 workInProgress.stateNode = createTextInstance(
1313 newText,
1314 rootContainerInstance,
packages/react-reconciler/src/ReactFiberFlags.js
+1 -1
@@ -20,7 +20,7 @@ export const Hydrating = /* */ 0b0000000000000001000000000000
20
21 // You can change the rest (and add more).
22 export const Update = /* */ 0b0000000000000000000000000100;
23 -/* Skipped value: 0b0000000000000000000000001000; */
23 +export const Cloned = /* */ 0b0000000000000000000000001000;
24
25 export const ChildDeletion = /* */ 0b0000000000000000000000010000;
26 export const ContentReset = /* */ 0b0000000000000000000000100000;
packages/react-reconciler/src/__tests__/ReactPersistent-test.js
+24 -2
@@ -12,6 +12,8 @@
12
13 let React;
14 let ReactNoopPersistent;
15 +
16 +let act;
17 let waitForAll;
18
19 describe('ReactPersistent', () => {
@@ -20,8 +22,7 @@ describe('ReactPersistent', () => {
22
23 React = require('react');
24 ReactNoopPersistent = require('react-noop-renderer/persistent');
23 - const InternalTestUtils = require('internal-test-utils');
24 - waitForAll = InternalTestUtils.waitForAll;
25 + ({act, waitForAll} = require('internal-test-utils'));
26 });
27
28 // Inlined from shared folder so we can run this test on a bundle.
@@ -213,4 +214,25 @@ describe('ReactPersistent', () => {
214 // The original is unchanged.
215 expect(newPortalChildren).toEqual([div(span(), 'Hello ', 'World')]);
216 });
217 +
218 + it('remove children', async () => {
219 + function Wrapper({children}) {
220 + return children;
221 + }
222 +
223 + const root = ReactNoopPersistent.createRoot();
224 + await act(() => {
225 + root.render(
226 + <Wrapper>
227 + <inner />
228 + </Wrapper>,
229 + );
230 + });
231 + expect(root.getChildrenAsJSX()).toEqual(<inner />);
232 +
233 + await act(() => {
234 + root.render(<Wrapper />);
235 + });
236 + expect(root.getChildrenAsJSX()).toEqual(null);
237 + });
238 });
packages/shared/ReactFeatureFlags.js
+6
@@ -134,6 +134,12 @@ export const passChildrenWhenCloningPersistedNodes = false;
134
135 export const enableServerComponentLogs = __EXPERIMENTAL__;
136
137 +/**
138 + * Enables a new Fiber flag used in persisted mode to reduce the number
139 + * of cloned host components.
140 + */
141 +export const enablePersistedModeClonedFlag = false;
142 +
143 export const enableAddPropertiesFastPath = false;
144
145 export const enableOwnerStacks = __EXPERIMENTAL__;
packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js
+1
@@ -20,6 +20,7 @@
20 export const alwaysThrottleRetries = __VARIANT__;
21 export const enableAddPropertiesFastPath = __VARIANT__;
22 export const enableObjectFiber = __VARIANT__;
23 +export const enablePersistedModeClonedFlag = __VARIANT__;
24 export const enableShallowPropDiffing = __VARIANT__;
25 export const passChildrenWhenCloningPersistedNodes = __VARIANT__;
26 export const enableFabricCompleteRootInCommitPhase = __VARIANT__;
packages/shared/forks/ReactFeatureFlags.native-fb.js
+1
@@ -23,6 +23,7 @@ export const {
23 enableAddPropertiesFastPath,
24 enableFabricCompleteRootInCommitPhase,
25 enableObjectFiber,
26 + enablePersistedModeClonedFlag,
27 enableShallowPropDiffing,
28 passChildrenWhenCloningPersistedNodes,
29 enableLazyContextPropagation,
packages/shared/forks/ReactFeatureFlags.native-oss.js
+1
@@ -58,6 +58,7 @@ export const enableLegacyHidden = false;
58 export const enableNoCloningMemoCache = false;
59 export const enableObjectFiber = false;
60 export const enableOwnerStacks = false;
61 +export const enablePersistedModeClonedFlag = false;
62 export const enablePostpone = false;
63 export const enableReactTestRendererWarning = false;
64 export const enableRefAsProp = true;
packages/shared/forks/ReactFeatureFlags.test-renderer.js
+1
@@ -70,6 +70,7 @@ export const enableAsyncActions = true;
70 export const alwaysThrottleRetries = true;
71
72 export const passChildrenWhenCloningPersistedNodes = false;
73 +export const enablePersistedModeClonedFlag = false;
74 export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__;
75 export const disableClientCache = true;
76
packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js
+1
@@ -49,6 +49,7 @@ export const enableLegacyHidden = false;
49 export const enableNoCloningMemoCache = false;
50 export const enableObjectFiber = false;
51 export const enableOwnerStacks = false;
52 +export const enablePersistedModeClonedFlag = false;
53 export const enablePostpone = false;
54 export const enableProfilerCommitHooks = __PROFILE__;
55 export const enableProfilerNestedUpdatePhase = __PROFILE__;
packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
+1
@@ -73,6 +73,7 @@ export const enableAsyncActions = true;
73 export const alwaysThrottleRetries = true;
74
75 export const passChildrenWhenCloningPersistedNodes = false;
76 +export const enablePersistedModeClonedFlag = false;
77 export const enableUseDeferredValueInitialArg = true;
78 export const disableClientCache = true;
79
packages/shared/forks/ReactFeatureFlags.www.js
+2
@@ -105,6 +105,8 @@ export const enableFizzExternalRuntime = true;
105
106 export const passChildrenWhenCloningPersistedNodes = false;
107
108 +export const enablePersistedModeClonedFlag = false;
109 +
110 export const enableAsyncDebugInfo = false;
111 export const disableClientCache = true;
112