From c97dcc2de56dd22b1a72de1265ebfcd150d33dd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Mon, 12 Jun 2023 11:02:22 -0700 Subject: [PATCH] Cache properties in global.nativeFabricUIManager when accessed through FabricUIManager.getFabricUIManager (#37796) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/37796 Changelog: [internal] `global.nativeFabricUIManager` is defined as a host object in https://github.com/facebook/react-native/blob/5cc8ceeae210f2f23ef18ded7b7b614682f0d67b/packages/react-native/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp and every time a property of that object is accessed, the value is recreated by the host object (like in https://github.com/facebook/react-native/blob/5cc8ceeae210f2f23ef18ded7b7b614682f0d67b/packages/react-native/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp#L179). This is problematic because we're continuously creating copies of those methods every time we access them. As a quick solution (until we migrate the whole native implementation to a regular object with eagerly defined properties or to a TurboModule), this creates a proxy object in JavaScript to cache all properties. Reviewed By: rshest Differential Revision: D46592010 fbshipit-source-id: 38f233becd2ca130fa331d61f99ba54fbf706e13 --- .../Libraries/ReactNative/FabricUIManager.js | 64 ++++++++++++++++- .../__tests__/FabricUIManager-test.js | 68 +++++++++++++++++++ 2 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 packages/react-native/Libraries/ReactNative/__tests__/FabricUIManager-test.js diff --git a/packages/react-native/Libraries/ReactNative/FabricUIManager.js b/packages/react-native/Libraries/ReactNative/FabricUIManager.js index 68372d8b44e..dbdd96cae7f 100644 --- a/packages/react-native/Libraries/ReactNative/FabricUIManager.js +++ b/packages/react-native/Libraries/ReactNative/FabricUIManager.js @@ -20,6 +20,8 @@ import type { } from '../Renderer/shims/ReactNativeTypes'; import type {RootTag} from '../Types/RootTagTypes'; +import defineLazyObjectProperty from '../Utilities/defineLazyObjectProperty'; + export type NodeSet = Array; export type NodeProps = {...}; export type Spec = {| @@ -91,9 +93,69 @@ export type Spec = {| ) => ?[/* scrollLeft: */ number, /* scrollTop: */ number], |}; +let nativeFabricUIManagerProxy: ?Spec; + +// This is a list of all the methods in global.nativeFabricUIManager that we'll +// cache in JavaScript, as the current implementation of the binding +// creates a new host function every time methods are accessed. +const CACHED_PROPERTIES = [ + 'createNode', + 'cloneNode', + 'cloneNodeWithNewChildren', + 'cloneNodeWithNewProps', + 'cloneNodeWithNewChildrenAndProps', + 'createChildSet', + 'appendChild', + 'appendChildToSet', + 'completeRoot', + 'measure', + 'measureInWindow', + 'measureLayout', + 'configureNextLayoutAnimation', + 'sendAccessibilityEvent', + 'findShadowNodeByTag_DEPRECATED', + 'setNativeProps', + 'dispatchCommand', + 'getParentNode', + 'getChildNodes', + 'isConnected', + 'compareDocumentPosition', + 'getTextContent', + 'getBoundingClientRect', + 'getOffset', + 'getScrollPosition', +]; + // This is exposed as a getter because apps using the legacy renderer AND // Fabric can define the binding lazily. If we evaluated the global and cached // it in the module we might be caching an `undefined` value before it is set. export function getFabricUIManager(): ?Spec { - return global.nativeFabricUIManager; + if ( + nativeFabricUIManagerProxy == null && + global.nativeFabricUIManager != null + ) { + nativeFabricUIManagerProxy = createProxyWithCachedProperties( + global.nativeFabricUIManager, + CACHED_PROPERTIES, + ); + } + return nativeFabricUIManagerProxy; +} + +/** + * + * Returns an object that caches the specified properties the first time they + * are accessed, and falls back to the original object for other properties. + */ +function createProxyWithCachedProperties( + implementation: Spec, + propertiesToCache: $ReadOnlyArray, +): Spec { + const proxy = Object.create(implementation); + for (const propertyName of propertiesToCache) { + defineLazyObjectProperty(proxy, propertyName, { + get: () => implementation[propertyName], + }); + } + return proxy; } diff --git a/packages/react-native/Libraries/ReactNative/__tests__/FabricUIManager-test.js b/packages/react-native/Libraries/ReactNative/__tests__/FabricUIManager-test.js new file mode 100644 index 00000000000..e9d1d49c291 --- /dev/null +++ b/packages/react-native/Libraries/ReactNative/__tests__/FabricUIManager-test.js @@ -0,0 +1,68 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow strict-local + * @format + * @oncall react_native + */ + +// flowlint unsafe-getters-setters:off + +describe('FabricUIManager', () => { + let getFabricUIManager; + + beforeEach(() => { + jest.resetModules(); + delete global.nativeFabricUIManager; + getFabricUIManager = require('../FabricUIManager').getFabricUIManager; + }); + + describe('getFabricUIManager', () => { + it('should return undefined if the global binding is not set', () => { + expect(getFabricUIManager()).toBeUndefined(); + }); + + it('should return an object with the same properties as the global binding', () => { + const createNode = jest.fn(); + const customProp = 'some prop'; + global.nativeFabricUIManager = { + createNode, + customProp, + }; + const fabricUIManager = getFabricUIManager(); + + expect(fabricUIManager).toEqual(expect.any(Object)); + expect(fabricUIManager?.createNode).toBe(createNode); + // $FlowExpectedError[prop-missing] + expect(fabricUIManager?.customProp).toBe(customProp); + }); + + it('should only access the cached properties of global binding once', () => { + let incrementingProp = 0; + global.nativeFabricUIManager = { + get createNode() { + return jest.fn(); + }, + get incrementingProp() { + return incrementingProp++; + }, + }; + + const fabricUIManager = getFabricUIManager(); + + expect(fabricUIManager).toEqual(expect.any(Object)); + const firstCreateNode = fabricUIManager?.createNode; + const secondCreateNode = fabricUIManager?.createNode; + // In the original object, the getter creates a new function every time. + expect(firstCreateNode).toBe(secondCreateNode); + + // $FlowExpectedError[prop-missing] + expect(fabricUIManager?.incrementingProp).toBe(0); + // $FlowExpectedError[prop-missing] + expect(fabricUIManager?.incrementingProp).toBe(1); + }); + }); +});