Skip to content

[RN] Set up test to create public instances lazily in Fabric #32363

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 12, 2025
Merged

[RN] Set up test to create public instances lazily in Fabric #32363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 50 additions & 19 deletions packages/react-native-renderer/src/ReactFiberConfigFabric.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@ import {
getInspectorDataForInstance,
} from './ReactNativeFiberInspector';

import {passChildrenWhenCloningPersistedNodes} from 'shared/ReactFeatureFlags';
import {
passChildrenWhenCloningPersistedNodes,
enableLazyPublicInstanceInFabric,
} from 'shared/ReactFeatureFlags';
import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols';
import type {ReactContext} from 'shared/ReactTypes';

Expand Down Expand Up @@ -93,8 +96,11 @@ export type Instance = {
currentProps: Props,
// Reference to the React handle (the fiber)
internalInstanceHandle: InternalInstanceHandle,
// Exposed through refs.
publicInstance: PublicInstance,
// Exposed through refs. Potentially lazily created.
publicInstance: PublicInstance | null,
// This is only necessary to lazily create `publicInstance`.
Copy link
Member

@javache javache Feb 12, 2025

Choose a reason for hiding this comment

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

Add docs that this will be nulled out once publicInstance is created?

// Will be set to `null` after that is created.
publicRootInstance?: PublicRootInstance | null,
Copy link
Member

@javache javache Feb 12, 2025

Choose a reason for hiding this comment

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

Is there anyway we could retrieve this lazily from internalInstanceHandle for example?

This is an extra slot for every single host component, which we should avoid if we can.

Copy link
Contributor Author

@rubennorte rubennorte Feb 12, 2025

Choose a reason for hiding this comment

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

Unfortunately internalInstanceHandle is pretty much an opaque object here, so we can't tap into the internals. Also, even if we did, the Fiber itself doesn't have a reference to the root, and we'd have to traverse the fiber tree up to it to access this, which would be inefficient.

The benchmarks I used took this field into account already, and even with this it should be a net win.

Copy link
Member

@javache javache Feb 12, 2025

Choose a reason for hiding this comment

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

I wouldn't expect this to show on benchmarks, but rather as memory overhead (increased overall heap size).

Copy link
Contributor Author

@rubennorte rubennorte Feb 12, 2025

Choose a reason for hiding this comment

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

Oh, I only meant runtime performance when I was referencing the benchmarks. What I mean is that this change will overall reduce memory usage, even with that new field, as we'll avoid a large amount of allocations for unused refs.

},
};
export type TextInstance = {
Expand Down Expand Up @@ -186,23 +192,37 @@ export function createInstance(
internalInstanceHandle, // internalInstanceHandle
);

const component = createPublicInstance(
tag,
viewConfig,
internalInstanceHandle,
rootContainerInstance.publicInstance,
);

return {
node: node,
canonical: {
nativeTag: tag,
if (enableLazyPublicInstanceInFabric) {
return {
node: node,
canonical: {
nativeTag: tag,
viewConfig,
currentProps: props,
internalInstanceHandle,
publicInstance: null,
publicRootInstance: rootContainerInstance.publicInstance,
},
};
} else {
const component = createPublicInstance(
tag,
viewConfig,
currentProps: props,
internalInstanceHandle,
publicInstance: component,
},
};
rootContainerInstance.publicInstance,
);

return {
node: node,
canonical: {
nativeTag: tag,
viewConfig,
currentProps: props,
internalInstanceHandle,
publicInstance: component,
},
};
}
}

export function createTextInstance(
Expand Down Expand Up @@ -277,7 +297,18 @@ export function getChildHostContext(
}

export function getPublicInstance(instance: Instance): null | PublicInstance {
if (instance.canonical != null && instance.canonical.publicInstance != null) {
if (instance.canonical != null) {
if (instance.canonical.publicInstance == null) {
instance.canonical.publicInstance = createPublicInstance(
instance.canonical.nativeTag,
instance.canonical.viewConfig,
instance.canonical.internalInstanceHandle,
instance.canonical.publicRootInstance ?? null,
);
// This was only necessary to create the public instance.
instance.canonical.publicRootInstance = null;
}

return instance.canonical.publicInstance;
}

Expand Down
2 changes: 2 additions & 0 deletions packages/shared/ReactFeatureFlags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ export const enableUseEffectCRUDOverload = false;

export const enableFastAddPropertiesInDiffing = true;

export const enableLazyPublicInstanceInFabric = false;

// -----------------------------------------------------------------------------
// Ready for next major.
//
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,4 @@ export const enableUseEffectCRUDOverload = __VARIANT__;
export const enableOwnerStacks = __VARIANT__;
export const enableRemoveConsolePatches = __VARIANT__;
export const enableFastAddPropertiesInDiffing = __VARIANT__;
export const enableLazyPublicInstanceInFabric = __VARIANT__;
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export const {
enableOwnerStacks,
enableRemoveConsolePatches,
enableFastAddPropertiesInDiffing,
enableLazyPublicInstanceInFabric,
} = dynamicFlags;

// The rest of the flags are static for better dead code elimination.
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export const enableYieldingBeforePassive = false;
export const enableThrottledScheduling = false;
export const enableViewTransition = false;
export const enableFastAddPropertiesInDiffing = false;
export const enableLazyPublicInstanceInFabric = false;

// Profiling Only
export const enableProfilerTimer = __PROFILE__;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export const enableYieldingBeforePassive = true;
export const enableThrottledScheduling = false;
export const enableViewTransition = false;
export const enableFastAddPropertiesInDiffing = true;
export const enableLazyPublicInstanceInFabric = false;

// TODO: This must be in sync with the main ReactFeatureFlags file because
// the Test Renderer's value must be the same as the one used by the
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export const enableThrottledScheduling = false;
export const enableViewTransition = false;
export const enableRemoveConsolePatches = false;
export const enableFastAddPropertiesInDiffing = false;
export const enableLazyPublicInstanceInFabric = false;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export const enableThrottledScheduling = false;
export const enableViewTransition = false;
export const enableRemoveConsolePatches = false;
export const enableFastAddPropertiesInDiffing = false;
export const enableLazyPublicInstanceInFabric = false;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www-dynamic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export const enableSiblingPrerendering = __VARIANT__;
export const enableUseEffectCRUDOverload = __VARIANT__;
export const enableRemoveConsolePatches = __VARIANT__;
export const enableFastAddPropertiesInDiffing = __VARIANT__;
export const enableLazyPublicInstanceInFabric = false;
export const enableViewTransition = __VARIANT__;

// TODO: These flags are hard-coded to the default values used in open source.
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,5 +110,7 @@ export const disableLegacyMode = true;

export const enableShallowPropDiffing = false;

export const enableLazyPublicInstanceInFabric = false;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
Loading