Optimize ReactEquivalenceSet (#2243)

Summary:
I generated the following program (now added to test cases) and while I would expect its evaluation in Prepack to terminate reasonably quickly, I did not see the program terminate after five minutes of execution:

https://gist.github.com/calebmer/2bf1d84a5b7849fa732bce69811df10b

After this change that same program’s Prepack evaluation terminates in about two seconds. This change also saves about 2.8s on the evaluation of our internal bundle which is about 3% of the total time.

What was happening? Why did my program fail to terminate execution in five minutes? Why was the internal bundle relatively ok compared to my extreme case?

In my program, I would [expect the component `Mu`](https://gist.github.com/calebmer/2bf1d84a5b7849fa732bce69811df10b#file-program-js-L289-L291) to be inlined about 60 times. Which means there should only be about 60 calls to `ReactElementSet#add` for each of `Mu`’s `div`s. However, after some basic instrumentation I observed way over ten thousand visits to these React elements.

This pair of method calls happens a couple times in `ReactEquivalenceSet` and friends.

https://github.com/facebook/prepack/blob/5f7256f17ed1bca84ed2f8c80a72d9251a32fc43/src/react/ReactEquivalenceSet.js#L233-L234

Both `getEquivalentPropertyValue`…

https://github.com/facebook/prepack/blob/5f7256f17ed1bca84ed2f8c80a72d9251a32fc43/src/react/ReactEquivalenceSet.js#L255

…and `getValue`…

https://github.com/facebook/prepack/blob/5f7256f17ed1bca84ed2f8c80a72d9251a32fc43/src/react/ReactEquivalenceSet.js#L104

…end up calling `ReactElementSet#add` with the same React element. Then `ReactElementSet#add` ends up recursing into children. So the root element is added 2 times, so the root’s children are added 4 times each, so each child of those elements are added 8 times each, and so on. So if a leaf element is `n` deep in the tree it will be added `2^n` times. The most nested `Mu` child was 9 elements deep. `Mu` appeared 60 times at many levels of nesting.

I’m not entirely sure why my generated case was so much worse then our internal bundle. My two best guesses are: lots of repeated components or deeper nesting?

My fix was to move the `getValue` call into `getEquivalentPropertyValue`. This way if `getEquivalentPropertyValue` already finds an equivalent value we can short-circuit and not run `getValue` which will perform the same operation.
Pull Request resolved: #2243

Reviewed By: trueadm

Differential Revision: D8811463

Pulled By: calebmer

fbshipit-source-id: 4f30a75e96c6592456f5b21ee9c2ee3e40b56d81

.gitignore

@@ -3,6 +3,7 @@
 /node_modules
 /lib
 npm-debug.log
+yarn-error.log
 .DS_Store
 build/
 .idea/

src/react/ReactElementSet.js

@@ -29,23 +29,19 @@ export class ReactElementSet {
 
     // type
     currentMap = reactEquivalenceSet.getKey("type", currentMap, visitedValues);
-    let type = reactEquivalenceSet.getEquivalentPropertyValue(reactElement, "type");
-    let result = reactEquivalenceSet.getValue(type, currentMap, visitedValues);
+    let result = reactEquivalenceSet.getEquivalentPropertyValue(reactElement, "type", currentMap, visitedValues);
     currentMap = result.map;
     // key
     currentMap = reactEquivalenceSet.getKey("key", currentMap, visitedValues);
-    let key = reactEquivalenceSet.getEquivalentPropertyValue(reactElement, "key");
-    result = reactEquivalenceSet.getValue(key, currentMap, visitedValues);
+    result = reactEquivalenceSet.getEquivalentPropertyValue(reactElement, "key", currentMap, visitedValues);
     currentMap = result.map;
     // ref
     currentMap = reactEquivalenceSet.getKey("ref", currentMap, visitedValues);
-    let ref = reactEquivalenceSet.getEquivalentPropertyValue(reactElement, "ref");
-    result = reactEquivalenceSet.getValue(ref, currentMap, visitedValues);
+    result = reactEquivalenceSet.getEquivalentPropertyValue(reactElement, "ref", currentMap, visitedValues);
     currentMap = result.map;
     // props
     currentMap = reactEquivalenceSet.getKey("props", currentMap, visitedValues);
-    let props = reactEquivalenceSet.getEquivalentPropertyValue(reactElement, "props");
-    result = reactEquivalenceSet.getValue(props, currentMap, visitedValues);
+    result = reactEquivalenceSet.getEquivalentPropertyValue(reactElement, "props", currentMap, visitedValues);
 
     if (result.value === null) {
       result.value = reactElement;

src/react/ReactEquivalenceSet.js

@@ -79,7 +79,7 @@ export class ReactEquivalenceSet {
     return ((map.get(key): any): ReactSetValueMap);
   }
 
-  getValue(val: ReactSetValueMapKey, map: ReactSetValueMap, visitedValues: Set<Value>): ReactSetNode {
+  _getValue(val: ReactSetValueMapKey, map: ReactSetValueMap, visitedValues: Set<Value>): ReactSetNode {
     if (val instanceof StringValue || val instanceof NumberValue) {
       val = val.value;
     } else if (val instanceof AbstractValue) {
@@ -108,14 +108,13 @@ export class ReactEquivalenceSet {
 
     for (let [propName] of object.properties) {
       currentMap = this.getKey(propName, currentMap, visitedValues);
-      let prop = this.getEquivalentPropertyValue(object, propName);
-      result = this.getValue(prop, currentMap, visitedValues);
+      result = this.getEquivalentPropertyValue(object, propName, currentMap, visitedValues);
       currentMap = result.map;
     }
     for (let [symbol] of object.symbols) {
       currentMap = this.getKey(symbol, currentMap, visitedValues);
       let prop = getProperty(this.realm, object, symbol);
-      result = this.getValue(prop, currentMap, visitedValues);
+      result = this._getValue(prop, currentMap, visitedValues);
       currentMap = result.map;
     }
     let temporalAlias = object.temporalAlias;
@@ -188,7 +187,7 @@ export class ReactEquivalenceSet {
       }
       currentMap = this.getKey(i, (currentMap: any), visitedValues);
       invariant(arg instanceof Value && (equivalenceArg instanceof Value || equivalenceArg === undefined));
-      result = this.getValue(equivalenceArg || arg, currentMap, visitedValues);
+      result = this._getValue(equivalenceArg || arg, currentMap, visitedValues);
       currentMap = result.map;
     }
     invariant(result !== undefined);
@@ -230,8 +229,7 @@ export class ReactEquivalenceSet {
 
     for (let i = 0; i < length; i++) {
       currentMap = this.getKey(i, currentMap, visitedValues);
-      let element = this.getEquivalentPropertyValue(array, "" + i);
-      result = this.getValue(element, currentMap, visitedValues);
+      result = this.getEquivalentPropertyValue(array, "" + i, currentMap, visitedValues);
       currentMap = result.map;
     }
     if (result === undefined) {
@@ -246,30 +244,34 @@ export class ReactEquivalenceSet {
     return result.value;
   }
 
-  getEquivalentPropertyValue(object: ObjectValue, propName: string): Value {
+  getEquivalentPropertyValue(
+    object: ObjectValue,
+    propName: string,
+    map: ReactSetValueMap,
+    visitedValues: Set<Value>
+  ): ReactSetNode {
     let prop = getProperty(this.realm, object, propName);
     let isFinal = object.mightBeFinalObject();
     let equivalentProp;
 
     if (prop instanceof ObjectValue && isReactElement(prop)) {
       equivalentProp = this.residualReactElementVisitor.reactElementEquivalenceSet.add(prop);
-
-      if (prop !== equivalentProp && isFinal) {
-        hardModifyReactObjectPropertyBinding(this.realm, object, propName, equivalentProp);
-      }
     } else if (prop instanceof ObjectValue && isReactPropsObject(prop)) {
       equivalentProp = this.residualReactElementVisitor.reactPropsEquivalenceSet.add(prop);
-
-      if (prop !== equivalentProp && isFinal) {
-        hardModifyReactObjectPropertyBinding(this.realm, object, propName, equivalentProp);
-      }
     } else if (prop instanceof AbstractValue) {
       equivalentProp = this.residualReactElementVisitor.residualHeapVisitor.equivalenceSet.add(prop);
+    }
 
+    if (equivalentProp !== undefined) {
       if (prop !== equivalentProp && isFinal) {
         hardModifyReactObjectPropertyBinding(this.realm, object, propName, equivalentProp);
       }
+      if (!map.has(equivalentProp)) {
+        map.set(equivalentProp, this._createNode());
+      }
+      return ((map.get(equivalentProp): any): ReactSetNode);
+    } else {
+      return this._getValue(prop, map, visitedValues);
     }
-    return equivalentProp || prop;
   }
 }

src/react/ReactPropsSet.js

@@ -30,8 +30,7 @@ export class ReactPropsSet {
 
     for (let [propName] of props.properties) {
       currentMap = reactEquivalenceSet.getKey(propName, currentMap, visitedValues);
-      let prop = reactEquivalenceSet.getEquivalentPropertyValue(props, propName);
-      result = reactEquivalenceSet.getValue(prop, currentMap, visitedValues);
+      result = reactEquivalenceSet.getEquivalentPropertyValue(props, propName, currentMap, visitedValues);
       currentMap = result.map;
     }
     let temporalAlias = props.temporalAlias;

test/react/FunctionalComponents-test.js

@@ -319,3 +319,7 @@ it("Dynamic ReactElement type #4", () => {
 it("Hoist Fragment", () => {
   runTest(__dirname + "/FunctionalComponents/hoist-fragment.js");
 });
+
+it("Pathological case", () => {
+  runTest(__dirname + "/FunctionalComponents/pathological-case.js");
+});