Refactor Attribute Processing (Step 1)

Summary: Concolidate the creation of the "update payload" into
ReactNativeAttributePayload which now has a create
and a diff version. The create version can be used by
both mountComponent and setNativeProps. This means that
diffRawProperties moves into ReactNativeAttributePayload.

Instead of storing previousFlattenedStyle as memoized
state in the component tree, I recalculate it every
time. This allows better use of the generational GC.
However, it is still probably a fairly expensive
technique so I will follow it up with a diff that
walks both nested array trees to do the diffing in a
follow up.

This is the first diff of several steps.

@​public

Reviewed By: @vjeux

Differential Revision: D2440644

fb-gh-sync-id: 1d0da4f6e2bf716f33e119df947c044abb918471

Libraries/ReactIOS/NativeMethodsMixin.js

@@ -13,6 +13,7 @@
 
 var NativeModules = require('NativeModules');
 var RCTUIManager = NativeModules.UIManager;
+var ReactNativeAttributePayload = require('ReactNativeAttributePayload');
 var TextInputState = require('TextInputState');
 
 var findNodeHandle = require('findNodeHandle');
@@ -65,52 +66,15 @@ var NativeMethodsMixin = {
    * next render, they will remain active.
    */
   setNativeProps: function(nativeProps: Object) {
-    // nativeProps contains a style attribute that's going to be flattened
-    // and all the attributes expanded in place. In order to make this
-    // process do as few allocations and copies as possible, we return
-    // one if the other is empty. Only if both have values then we create
-    // a new object and merge.
-    var hasOnlyStyle = true;
-    for (var key in nativeProps) {
-      if (key !== 'style') {
-        hasOnlyStyle = false;
-        break;
-      }
-    }
-
-    var validAttributes = this.viewConfig.validAttributes;
-    var hasProcessedProps = false;
-    var processedProps = {};
-    for (var key in nativeProps) {
-      var process = validAttributes[key] && validAttributes[key].process;
-      if (process) {
-        hasProcessedProps = true;
-        processedProps[key] = process(nativeProps[key]);
-      }
-    }
-
-    var style = precomputeStyle(
-      flattenStyle(processedProps.style || nativeProps.style),
+    var updatePayload = ReactNativeAttributePayload.create(
+      nativeProps,
       this.viewConfig.validAttributes
     );
 
-    var props = null;
-    if (hasOnlyStyle) {
-      props = style;
-    } else {
-      props = nativeProps;
-      if (hasProcessedProps) {
-        props = mergeFast(props, processedProps);
-      }
-      if (style) {
-        props = mergeFast(props, style);
-      }
-    }
-
     RCTUIManager.updateView(
       findNodeHandle(this),
       this.viewConfig.uiViewClassName,
-      props
+      updatePayload
     );
   },
 

Libraries/ReactNative/ReactNativeAttributePayload.js

@@ -6,12 +6,18 @@
  * LICENSE file in the root directory of this source tree. An additional grant
  * of patent rights can be found in the PATENTS file in the same directory.
  *
- * @providesModule diffRawProperties
+ * @providesModule ReactNativeAttributePayload
  * @flow
  */
 'use strict';
 
+var ReactNativeStyleAttributes = require('ReactNativeStyleAttributes');
+
 var deepDiffer = require('deepDiffer');
+var styleDiffer = require('styleDiffer');
+var deepFreezeAndThrowOnMutationInDev = require('deepFreezeAndThrowOnMutationInDev');
+var flattenStyle = require('flattenStyle');
+var precomputeStyle = require('precomputeStyle');
 
 /**
  * diffRawProperties takes two sets of props and a set of valid attributes
@@ -115,4 +121,72 @@ function diffRawProperties(
   return updatePayload;
 }
 
-module.exports = diffRawProperties;
+var ReactNativeAttributePayload = {
+
+  create: function(
+    props : Object,
+    validAttributes : Object
+  ) : ?Object {
+    return ReactNativeAttributePayload.diff({}, props, validAttributes);
+  },
+
+  diff: function(
+    prevProps : Object,
+    nextProps : Object,
+    validAttributes : Object
+  ) : ?Object {
+
+    if (__DEV__) {
+      for (var key in nextProps) {
+        if (nextProps.hasOwnProperty(key) &&
+            nextProps[key] &&
+            validAttributes[key]) {
+          deepFreezeAndThrowOnMutationInDev(nextProps[key]);
+        }
+      }
+    }
+
+    var updatePayload = diffRawProperties(
+      null, // updatePayload
+      prevProps,
+      nextProps,
+      validAttributes
+    );
+
+    for (var key in updatePayload) {
+      var process = validAttributes[key] && validAttributes[key].process;
+      if (process) {
+        updatePayload[key] = process(updatePayload[key]);
+      }
+    }
+
+    // The style property is a deeply nested element which includes numbers
+    // to represent static objects. Most of the time, it doesn't change across
+    // renders, so it's faster to spend the time checking if it is different
+    // before actually doing the expensive flattening operation in order to
+    // compute the diff.
+    if (styleDiffer(nextProps.style, prevProps.style)) {
+      // TODO: Use a cached copy of previousFlattenedStyle, or walk both
+      // props in parallel.
+      var previousFlattenedStyle = precomputeStyle(
+        flattenStyle(prevProps.style),
+        validAttributes
+      );
+      var nextFlattenedStyle = precomputeStyle(
+        flattenStyle(nextProps.style),
+        validAttributes
+      );
+      updatePayload = diffRawProperties(
+        updatePayload,
+        previousFlattenedStyle,
+        nextFlattenedStyle,
+        ReactNativeStyleAttributes
+      );
+    }
+
+    return updatePayload;
+  }
+
+};
+
+module.exports = ReactNativeAttributePayload;

Libraries/ReactNative/ReactNativeBaseComponent.js

@@ -12,17 +12,13 @@
 'use strict';
 
 var NativeMethodsMixin = require('NativeMethodsMixin');
+var ReactNativeAttributePayload = require('ReactNativeAttributePayload');
 var ReactNativeEventEmitter = require('ReactNativeEventEmitter');
 var ReactNativeStyleAttributes = require('ReactNativeStyleAttributes');
 var ReactNativeTagHandles = require('ReactNativeTagHandles');
 var ReactMultiChild = require('ReactMultiChild');
 var RCTUIManager = require('NativeModules').UIManager;
 
-var styleDiffer = require('styleDiffer');
-var deepFreezeAndThrowOnMutationInDev = require('deepFreezeAndThrowOnMutationInDev');
-var diffRawProperties = require('diffRawProperties');
-var flattenStyle = require('flattenStyle');
-var precomputeStyle = require('precomputeStyle');
 var warning = require('warning');
 
 var registrationNames = ReactNativeEventEmitter.registrationNames;
@@ -131,63 +127,6 @@ ReactNativeBaseComponent.Mixin = {
     }
   },
 
-
-  /**
-   * Beware, this function has side effect to store this.previousFlattenedStyle!
-   *
-   * @param {!object} prevProps Previous properties
-   * @param {!object} nextProps Next properties
-   * @param {!object} validAttributes Set of valid attributes and how they
-   *                  should be diffed
-   */
-  computeUpdatedProperties: function(prevProps, nextProps, validAttributes) {
-    if (__DEV__) {
-      for (var key in nextProps) {
-        if (nextProps.hasOwnProperty(key) &&
-            nextProps[key] &&
-            validAttributes[key]) {
-          deepFreezeAndThrowOnMutationInDev(nextProps[key]);
-        }
-      }
-    }
-
-    var updatePayload = diffRawProperties(
-      null, // updatePayload
-      prevProps,
-      nextProps,
-      validAttributes
-    );
-
-    for (var key in updatePayload) {
-      var process = validAttributes[key] && validAttributes[key].process;
-      if (process) {
-        updatePayload[key] = process(updatePayload[key]);
-      }
-    }
-
-    // The style property is a deeply nested element which includes numbers
-    // to represent static objects. Most of the time, it doesn't change across
-    // renders, so it's faster to spend the time checking if it is different
-    // before actually doing the expensive flattening operation in order to
-    // compute the diff.
-    if (styleDiffer(nextProps.style, prevProps.style)) {
-      var nextFlattenedStyle = precomputeStyle(
-        flattenStyle(nextProps.style),
-        this.viewConfig.validAttributes
-      );
-      updatePayload = diffRawProperties(
-        updatePayload,
-        this.previousFlattenedStyle,
-        nextFlattenedStyle,
-        ReactNativeStyleAttributes
-      );
-      this.previousFlattenedStyle = nextFlattenedStyle;
-    }
-
-    return updatePayload;
-  },
-
-
   /**
    * Updates the component's currently mounted representation.
    *
@@ -200,7 +139,7 @@ ReactNativeBaseComponent.Mixin = {
     var prevElement = this._currentElement;
     this._currentElement = nextElement;
 
-    var updatePayload = this.computeUpdatedProperties(
+    var updatePayload = ReactNativeAttributePayload.diff(
       prevElement.props,
       nextElement.props,
       this.viewConfig.validAttributes
@@ -262,10 +201,8 @@ ReactNativeBaseComponent.Mixin = {
 
     var tag = ReactNativeTagHandles.allocateTag();
 
-    this.previousFlattenedStyle = {};
-    var updatePayload = this.computeUpdatedProperties(
-      {}, // previous props
-      this._currentElement.props, // next props
+    var updatePayload = ReactNativeAttributePayload.create(
+      this._currentElement.props,
       this.viewConfig.validAttributes
     );
 

Libraries/ReactNative/__benchmarks__/ReactNativeAttributePayload-benchmark.js

@@ -242,13 +242,7 @@ var variants = {
 
 var validAttributes = require('ReactNativeViewAttributes').UIView;
 
-var ReactNativeBaseComponent = require('ReactNativeBaseComponent');
-ReactNativeBaseComponent.prototype.diff =
-  ReactNativeBaseComponent.prototype.computeUpdatedProperties;
-var Differ = new ReactNativeBaseComponent({
-  validAttributes: validAttributes,
-  uiViewClassName: 'Differ'
-});
+var Differ = require('ReactNativeAttributePayload');
 
 // Runner
 

Libraries/ReactNative/__tests__/ReactNativeAttributePayload-test.js

@@ -3,15 +3,19 @@
  */
 'use strict';
 
-jest.dontMock('diffRawProperties');
+jest.dontMock('ReactNativeAttributePayload');
 jest.dontMock('deepDiffer');
-var diffRawProperties = require('diffRawProperties');
+jest.dontMock('styleDiffer');
+jest.dontMock('precomputeStyle');
+jest.dontMock('flattenStyle');
+var ReactNativeAttributePayload = require('ReactNativeAttributePayload');
 
-describe('diffRawProperties', function() {
+var diff = ReactNativeAttributePayload.diff;
+
+describe('ReactNativeAttributePayload', function() {
 
   it('should work with simple example', () => {
-    expect(diffRawProperties(
-      null,
+    expect(diff(
       {a: 1, c: 3},
       {b: 2, c: 3},
       {a: true, b: true}
@@ -19,8 +23,7 @@ describe('diffRawProperties', function() {
   });
 
   it('should skip fields that are equal', () => {
-    expect(diffRawProperties(
-      null,
+    expect(diff(
       {a: 1, b: 'two', c: true, d: false, e: undefined, f: 0},
       {a: 1, b: 'two', c: true, d: false, e: undefined, f: 0},
       {a: true, b: true, c: true, d: true, e: true, f: true}
@@ -28,8 +31,7 @@ describe('diffRawProperties', function() {
   });
 
   it('should remove fields', () => {
-    expect(diffRawProperties(
-      null,
+    expect(diff(
       {a: 1},
       {},
       {a: true}
@@ -37,32 +39,17 @@ describe('diffRawProperties', function() {
   });
 
   it('should ignore invalid fields', () => {
-    expect(diffRawProperties(
-      null,
+    expect(diff(
       {a: 1},
       {b: 2},
       {}
     )).toEqual(null);
   });
 
-  it('should override the updatePayload argument', () => {
-    var updatePayload = {a: 1};
-    var result = diffRawProperties(
-      updatePayload,
-      {},
-      {b: 2},
-      {b: true}
-    );
-
-    expect(result).toBe(updatePayload);
-    expect(result).toEqual({a: 1, b: 2});
-  });
-
   it('should use the diff attribute', () => {
     var diffA = jest.genMockFunction().mockImpl((a, b) => true);
     var diffB = jest.genMockFunction().mockImpl((a, b) => false);
-    expect(diffRawProperties(
-      null,
+    expect(diff(
       {a: [1], b: [3]},
       {a: [2], b: [4]},
       {a: {diff: diffA}, b: {diff: diffB}}
@@ -74,8 +61,7 @@ describe('diffRawProperties', function() {
   it('should not use the diff attribute on addition/removal', () => {
     var diffA = jest.genMockFunction();
     var diffB = jest.genMockFunction();
-    expect(diffRawProperties(
-      null,
+    expect(diff(
       {a: [1]},
       {b: [2]},
       {a: {diff: diffA}, b: {diff: diffB}}
@@ -85,8 +71,7 @@ describe('diffRawProperties', function() {
   });
 
   it('should do deep diffs of Objects by default', () => {
-    expect(diffRawProperties(
-      null,
+    expect(diff(
       {a: [1], b: {k: [3,4]}, c: {k: [4,4]} },
       {a: [2], b: {k: [3,4]}, c: {k: [4,5]} },
       {a: true, b: true, c: true}
@@ -94,41 +79,35 @@ describe('diffRawProperties', function() {
   });
 
   it('should work with undefined styles', () => {
-    expect(diffRawProperties(
-      null,
-      {a: 1, c: 3},
-      undefined,
-      {a: true, b: true}
-    )).toEqual({a: null});
-    expect(diffRawProperties(
-      null,
-      undefined,
-      {a: 1, c: 3},
-      {a: true, b: true}
-    )).toEqual({a: 1});
-    expect(diffRawProperties(
-      null,
-      undefined,
-      undefined,
-      {a: true, b: true}
+    expect(diff(
+      { style: { a: '#ffffff', opacity: 1 } },
+      { style: undefined },
+      { }
+    )).toEqual({ opacity: null });
+    expect(diff(
+      { style: undefined },
+      { style: { a: '#ffffff', opacity: 1 } },
+      { }
+    )).toEqual({ opacity: 1 });
+    expect(diff(
+      { style: undefined },
+      { style: undefined },
+      { }
     )).toEqual(null);
   });
 
   it('should work with empty styles', () => {
-    expect(diffRawProperties(
-      null,
+    expect(diff(
       {a: 1, c: 3},
       {},
       {a: true, b: true}
     )).toEqual({a: null});
-    expect(diffRawProperties(
-      null,
+    expect(diff(
       {},
       {a: 1, c: 3},
       {a: true, b: true}
     )).toEqual({a: 1});
-    expect(diffRawProperties(
-      null,
+    expect(diff(
       {},
       {},
       {a: true, b: true}
@@ -139,8 +118,7 @@ describe('diffRawProperties', function() {
   it('should convert functions to booleans', () => {
     // Note that if the property changes from one function to another, we don't
     // need to send an update.
-    expect(diffRawProperties(
-      null,
+    expect(diff(
       {a: function() { return 1; }, b: function() { return 2; }, c: 3},
       {b: function() { return 9; }, c: function() { return 3; }, },
       {a: true, b: true, c: true}

Libraries/ReactNative/createReactNativeComponentClass.js

@@ -33,7 +33,6 @@ var createReactNativeComponentClass = function(
 
     this._rootNodeID = null;
     this._renderedChildren = null;
-    this.previousFlattenedStyle = null;
   };
   Constructor.displayName = viewConfig.uiViewClassName;
   Constructor.viewConfig = viewConfig;