RN (fiber) avoids the overhead of bridge calls if there's no update. (#…

…10505) This is an expensive no-op for Android, and causes an unnecessary view invalidation for certain components (eg RCTTextInput) on iOS.
facebook · Aug 22, 2017 · 6ab2869 · 6ab2869
1 parent 640eb70
commit 6ab2869
Show file tree

Hide file tree

Showing 5 changed files with 121 additions and 22 deletions.
diff --git a/src/renderers/native/NativeMethodsMixin.js b/src/renderers/native/NativeMethodsMixin.js
@@ -183,11 +183,16 @@ function setNativePropsFiber(componentOrHandle: any, nativeProps: Object) {
     viewConfig.validAttributes,
   );
 
-  UIManager.updateView(
-    maybeInstance._nativeTag,
-    viewConfig.uiViewClassName,
-    updatePayload,
-  );
+  // Avoid the overhead of bridge calls if there's no update.
+  // This is an expensive no-op for Android, and causes an unnecessary
+  // view invalidation for certain components (eg RCTTextInput) on iOS.
+  if (updatePayload != null) {
+    UIManager.updateView(
+      maybeInstance._nativeTag,
+      viewConfig.uiViewClassName,
+      updatePayload,
+    );
+  }
 }
 
 // TODO (bvaughn) Remove this once ReactNativeStack is dropped.
@@ -238,7 +243,9 @@ function setNativePropsStack(componentOrHandle: any, nativeProps: Object) {
     viewConfig.validAttributes,
   );
 
-  UIManager.updateView(tag, viewConfig.uiViewClassName, updatePayload);
+  if (updatePayload) {
+    UIManager.updateView(tag, viewConfig.uiViewClassName, updatePayload);
+  }
 }
 
 // Switching based on fiber vs stack to avoid a lot of inline checks at runtime.

diff --git a/src/renderers/native/ReactNativeComponent.js b/src/renderers/native/ReactNativeComponent.js
@@ -174,11 +174,16 @@ function setNativePropsFiber(componentOrHandle: any, nativeProps: Object) {
     viewConfig.validAttributes,
   );
 
-  UIManager.updateView(
-    maybeInstance._nativeTag,
-    viewConfig.uiViewClassName,
-    updatePayload,
-  );
+  // Avoid the overhead of bridge calls if there's no update.
+  // This is an expensive no-op for Android, and causes an unnecessary
+  // view invalidation for certain components (eg RCTTextInput) on iOS.
+  if (updatePayload != null) {
+    UIManager.updateView(
+      maybeInstance._nativeTag,
+      viewConfig.uiViewClassName,
+      updatePayload,
+    );
+  }
 }
 
 // TODO (bvaughn) Remove this once ReactNativeStack is dropped.
@@ -225,7 +230,9 @@ function setNativePropsStack(componentOrHandle: any, nativeProps: Object) {
     viewConfig.validAttributes,
   );
 
-  UIManager.updateView(tag, viewConfig.uiViewClassName, updatePayload);
+  if (updatePayload != null) {
+    UIManager.updateView(tag, viewConfig.uiViewClassName, updatePayload);
+  }
 }
 
 // Switching based on fiber vs stack to avoid a lot of inline checks at runtime.

diff --git a/src/renderers/native/ReactNativeFiberHostComponent.js b/src/renderers/native/ReactNativeFiberHostComponent.js
@@ -90,11 +90,16 @@ class ReactNativeFiberHostComponent {
       this.viewConfig.validAttributes,
     );
 
-    UIManager.updateView(
-      this._nativeTag,
-      this.viewConfig.uiViewClassName,
-      updatePayload,
-    );
+    // Avoid the overhead of bridge calls if there's no update.
+    // This is an expensive no-op for Android, and causes an unnecessary
+    // view invalidation for certain components (eg RCTTextInput) on iOS.
+    if (updatePayload != null) {
+      UIManager.updateView(
+        this._nativeTag,
+        this.viewConfig.uiViewClassName,
+        updatePayload,
+      );
+    }
   }
 }
 

diff --git a/src/renderers/native/ReactNativeFiberRenderer.js b/src/renderers/native/ReactNativeFiberRenderer.js
@@ -143,11 +143,16 @@ const NativeRenderer = ReactFiberReconciler({
       viewConfig.validAttributes,
     );
 
-    UIManager.updateView(
-      instance._nativeTag, // reactTag
-      viewConfig.uiViewClassName, // viewName
-      updatePayload, // props
-    );
+    // Avoid the overhead of bridge calls if there's no update.
+    // This is an expensive no-op for Android, and causes an unnecessary
+    // view invalidation for certain components (eg RCTTextInput) on iOS.
+    if (updatePayload != null) {
+      UIManager.updateView(
+        instance._nativeTag, // reactTag
+        viewConfig.uiViewClassName, // viewName
+        updatePayload, // props
+      );
+    }
   },
 
   createInstance(

diff --git a/src/renderers/native/__tests__/ReactNativeMount-test.js b/src/renderers/native/__tests__/ReactNativeMount-test.js
@@ -11,6 +11,7 @@
 
 'use strict';
 
+var PropTypes;
 var React;
 var ReactNative;
 var createReactNativeComponentClass;
@@ -20,6 +21,7 @@ describe('ReactNative', () => {
   beforeEach(() => {
     jest.resetModules();
 
+    PropTypes = require('prop-types');
     React = require('react');
     ReactNative = require('react-native');
     UIManager = require('UIManager');
@@ -60,6 +62,79 @@ describe('ReactNative', () => {
     expect(UIManager.updateView).toBeCalledWith(2, 'View', {foo: 'bar'});
   });
 
+  it('should not call UIManager.updateView after render for properties that have not changed', () => {
+    const Text = createReactNativeComponentClass({
+      validAttributes: {foo: true},
+      uiViewClassName: 'Text',
+    });
+
+    // Context hack is required for RN text rendering in stack.
+    // TODO Remove this from the test when RN stack has been deleted.
+    class Hack extends React.Component {
+      static childContextTypes = {isInAParentText: PropTypes.bool};
+      getChildContext() {
+        return {isInAParentText: true};
+      }
+      render() {
+        return this.props.children;
+      }
+    }
+
+    ReactNative.render(<Hack><Text foo="a">1</Text></Hack>, 11);
+    expect(UIManager.updateView).not.toBeCalled();
+
+    // If no properties have changed, we shouldn't call updateView.
+    ReactNative.render(<Hack><Text foo="a">1</Text></Hack>, 11);
+    expect(UIManager.updateView).not.toBeCalled();
+
+    // Only call updateView for the changed property (and not for text).
+    ReactNative.render(<Hack><Text foo="b">1</Text></Hack>, 11);
+    expect(UIManager.updateView.mock.calls.length).toBe(1);
+
+    // Only call updateView for the changed text (and no other properties).
+    ReactNative.render(<Hack><Text foo="b">2</Text></Hack>, 11);
+    expect(UIManager.updateView.mock.calls.length).toBe(2);
+
+    // Call updateView for both changed text and properties.
+    ReactNative.render(<Hack><Text foo="c">3</Text></Hack>, 11);
+    expect(UIManager.updateView.mock.calls.length).toBe(4);
+  });
+
+  it('should not call UIManager.updateView from setNativeProps for properties that have not changed', () => {
+    const View = createReactNativeComponentClass({
+      validAttributes: {foo: true},
+      uiViewClassName: 'View',
+    });
+
+    class Subclass extends ReactNative.NativeComponent {
+      render() {
+        return <View />;
+      }
+    }
+
+    [View, Subclass].forEach(Component => {
+      UIManager.updateView.mockReset();
+
+      let viewRef;
+      ReactNative.render(
+        <Component
+          foo="bar"
+          ref={ref => {
+            viewRef = ref;
+          }}
+        />,
+        11,
+      );
+      expect(UIManager.updateView).not.toBeCalled();
+
+      viewRef.setNativeProps({});
+      expect(UIManager.updateView).not.toBeCalled();
+
+      viewRef.setNativeProps({foo: 'baz'});
+      expect(UIManager.updateView.mock.calls.length).toBe(1);
+    });
+  });
+
   it('returns the correct instance and calls it in the callback', () => {
     var View = createReactNativeComponentClass({
       validAttributes: {foo: true},