If a function is referenced before it's natural insertion point,

we cannot apply the optimization that emits "var f = ...bind(...); " calls. Added positive and negative test. While at it, reviewed some places where code emission was delayed for no reason.
facebookarchive · Apr 26, 2017 · 0154392 · 0154392
1 parent b18dc89
commit 0154392
Show file tree

Hide file tree

Showing 5 changed files with 95 additions and 58 deletions.
diff --git a/scripts/test-runner.js b/scripts/test-runner.js
@@ -120,13 +120,13 @@ function runTest(name, code) {
   } else {
     let expected, actual;
     let codeIterations = [];
-    let marker = "// does not contain:";
-    let to_find = "";
-    let find_pos = -1;
-    if (code.includes(marker)) {
-      let start_pos = code.indexOf(marker);
-      to_find = code.substring(start_pos + marker.length, code.indexOf("\n", start_pos));
-      find_pos = start_pos + marker.length + to_find.length;
+    let markersToFind = [];
+    for (let [positive, marker] of [[true, "// does contain:"], [false, "// does not contain:"]]) {
+      if (code.includes(marker)) {
+        let i = code.indexOf(marker);
+        let value = code.substring(i + marker.length, code.indexOf("\n", i));
+        markersToFind.push({ positive, value, start: i + marker.length });
+      }
     }
     let unique = 27277;
     let oldUniqueSuffix = "";
@@ -147,10 +147,15 @@ function runTest(name, code) {
         }
         let newCode = serialized.code;
         codeIterations.push(newCode);
-        if (find_pos !== -1 && newCode.indexOf(to_find, find_pos) !== -1) {
-          console.log(chalk.red("Output contains forbidden string: " + to_find));
-          break;
+        let markersIssue = false;
+        for (let { positive, value, start } of markersToFind) {
+          let found = newCode.indexOf(value, start) !== -1;
+          if (found !== positive) {
+            console.log(chalk.red(`Output ${positive ? "does not contain" : "contains"} forbidden string: ${value}`));
+            markersIssue = true;
+          }
         }
+        if (markersIssue) break;
         actual = exec(newCode);
         if (expected !== actual) {
           console.log(chalk.red("Output mismatch!"));

diff --git a/src/serializer/serializer.js b/src/serializer/serializer.js
@@ -97,6 +97,7 @@ export class Serializer {
     this.factoryNameGenerator = this.preludeGenerator.createNameGenerator("$_");
     this.requireReturns = new Map();
     this.statistics = new SerializerStatistics();
+    this.firstFunctionUsages = new Map();
   }
 
   globalReasons: {
@@ -132,6 +133,7 @@ export class Serializer {
   requireReturns: Map<number | string, BabelNodeExpression>;
   options: SerializerOptions;
   statistics: SerializerStatistics;
+  firstFunctionUsages: Map<FunctionValue, BodyReference>;
 
   _getBodyReference() {
     return new BodyReference(this.body, this.body.length);
@@ -330,10 +332,9 @@ export class Serializer {
           },
           mightHaveBeenDeleted);
       };
-      let delayReason = this._shouldDelayValue(descValue) || mightHaveBeenDeleted;
-      if (delayReason) {
-        // handle self recursion
-        this._delay(delayReason, [descValue], serializeFunc, mightHaveBeenDeleted);
+      invariant(!this._shouldDelayValues([descValue, val]), "precondition of _emitProperty");
+      if (mightHaveBeenDeleted) {
+        this._delay(true, [], serializeFunc);
       } else {
         serializeFunc();
       }
@@ -373,28 +374,28 @@ export class Serializer {
         this.body.push(declar);
         this.descriptors.set(descriptorsKey, descriptorId);
       }
+      invariant(descriptorId !== undefined);
 
       for (let descKey of valKeys) {
         if (descKey in desc) {
           let descValue = desc[descKey] || this.realm.intrinsics.undefined;
           invariant(descValue instanceof Value);
-          this._eagerOrDelay([descValue], () => {
-            invariant(descriptorId !== undefined);
-            this.body.push(t.expressionStatement(t.assignmentExpression(
-              "=",
-              t.memberExpression(descriptorId, t.identifier(descKey)),
-              this.serializeValue(
-                descValue,
-                reasons.concat(`Referred to in the object ${name} for the key ${((key: any): BabelNodeIdentifier).name || ((key: any): BabelNodeStringLiteral).value} in the descriptor property ${descKey}`)
-              )
-            )));
-          });
+          invariant(!this._shouldDelayValues([descValue]), "precondition of _emitProperty");
+          this.body.push(t.expressionStatement(t.assignmentExpression(
+            "=",
+            t.memberExpression(descriptorId, t.identifier(descKey)),
+            this.serializeValue(
+              descValue,
+              reasons.concat(`Referred to in the object ${name} for the key ${((key: any): BabelNodeIdentifier).name || ((key: any): BabelNodeStringLiteral).value} in the descriptor property ${descKey}`)
+            )
+          )));
         }
       }
 
       let keyRaw = key;
       if (t.isIdentifier(keyRaw)) keyRaw = t.stringLiteral(((keyRaw: any): BabelNodeIdentifier).name);
 
+      invariant(!this._shouldDelayValues([val]), "precondition of _emitProperty");
       let uid = this._getValIdForReference(val);
       this.body.push(t.expressionStatement(t.callExpression(
         this.preludeGenerator.memoizeReference("Object.defineProperty"),
@@ -424,7 +425,6 @@ export class Serializer {
         // Increment ref count one more time to ensure that this object will be assigned a unique id.
         // This ensures that only once instance is created across all possible residual function invocations.
         this._incrementValToRefCount(value);
-
       }
     }
     return serializedBinding;
@@ -558,8 +558,10 @@ export class Serializer {
         delayReason = this._shouldDelayValue(arg);
         if (delayReason) return delayReason;
       }
-    } else if (val instanceof FunctionValue) return false;
-    else if (val instanceof AbstractValue) {
+    } else if (val instanceof FunctionValue) {
+      if (!this.firstFunctionUsages.has(val)) this.firstFunctionUsages.set(val, this._getBodyReference());
+      return false;
+    } else if (val instanceof AbstractValue) {
       if (val.hasIdentifier() && !this.declaredDerivedIds.has(val.getIdentifier())) return val.getIdentifier();
       for (let arg of val.args) {
         delayReason = this._shouldDelayValue(arg);
@@ -635,7 +637,7 @@ export class Serializer {
               let delayReason = this._shouldDelayValue(elemVal) || mightHaveBeenDeleted;
               if (delayReason) {
                 // handle self recursion
-                this._delay(delayReason, [elemVal], () => {
+                this._delay(delayReason, [elemVal, val], () => {
                   this._assignProperty(
                     () => t.memberExpression(this._getValIdForReference(val), t.numericLiteral(i), true),
                     () => {
@@ -732,7 +734,13 @@ export class Serializer {
       serializedBindings,
       functionValue: val,
     };
-    let delayed = 0;
+    let delayed = 1;
+    let undelay = () => {
+      if (--delayed === 0) {
+        instance.insertionPoint = this._getBodyReference();
+        this.functionInstances.push(instance);
+      }
+    };
     for (let innerName in functionInfo.names) {
       let referencedValues = [];
       let serializeBindingFunc;
@@ -762,32 +770,18 @@ export class Serializer {
           invariant(false);
         }
       }
-      let delayReason = this._shouldDelayValues(referencedValues);
-      let serialize = () => {
+      delayed++;
+      this._eagerOrDelay(referencedValues, () => {
         let serializedBinding = serializeBindingFunc();
         invariant(serializedBinding);
         serializedBindings[innerName] = serializedBinding;
         invariant(functionInfo);
         if (functionInfo.modified[innerName]) serializedBinding.modified = true;
-      };
-      if (delayReason) {
-        delayed++;
-        this._delay(delayReason, referencedValues, () => {
-          serialize();
-          if (--delayed === 0) {
-            instance.bodyReference = this._getBodyReference();
-            this.functionInstances.push(instance);
-          }
-        });
-      } else {
-        serialize();
-      }
+        undelay();
+      });
     }
 
-    if (delayed === 0) {
-      instance.bodyReference = this._getBodyReference();
-      this.functionInstances.push(instance);
-    }
+    undelay();
     functionInfo.instances.push(instance);
 
     this.addProperties(name, val, false, reasons);
@@ -816,7 +810,7 @@ export class Serializer {
     let func = this._isPrototype(val);
     if (func !== undefined) {
       let serializedFunction = this.serializeValue(func, reasons.concat(`Constructor of object ${name}`));
-      this.addProperties(name, val, false, reasons, val.properties);
+      this.addProperties(name, val, false, reasons);
       return t.memberExpression(serializedFunction, t.identifier("prototype"));
     }
 
@@ -836,7 +830,7 @@ export class Serializer {
         let delayReason = this._shouldDelayValue(propValue) || mightHaveBeenDeleted;
         if (delayReason) {
           // self recursion
-          this._delay(delayReason, [propValue], () => {
+          this._delay(delayReason, [propValue, val], () => {
             this._assignProperty(
               () => t.memberExpression(this._getValIdForReference(val), keyNode, t.isStringLiteral(keyNode)),
               () => {
@@ -1074,11 +1068,13 @@ export class Serializer {
         //
 
         for (let instance of instances) {
-          let { functionValue, serializedBindings } = instance;
+          let { functionValue, serializedBindings, insertionPoint } = instance;
           let id = this._getValIdForReference(functionValue);
           let flatArgs: Array<BabelNodeExpression> = factoryNames.map((name) => serializedBindings[name].serializedValue);
           let node;
-          if (usesThis) {
+          let firstUsage = this.firstFunctionUsages.get(functionValue);
+          invariant(insertionPoint !== undefined);
+          if (usesThis || firstUsage !== undefined && !firstUsage.isNotEarlierThan(insertionPoint)) {
             let callArgs: Array<BabelNodeExpression | BabelNodeSpreadElement> = [t.thisExpression()];
             for (let flatArg of flatArgs) callArgs.push(flatArg);
             for (let param of params) {
@@ -1110,10 +1106,9 @@ export class Serializer {
     for (let instance of this.functionInstances.reverse()) {
       let functionBody = functionBodies.get(instance);
       invariant(functionBody !== undefined);
-      let bodyReference = instance.bodyReference;
-      invariant(bodyReference instanceof BodyReference);
-      invariant(bodyReference.index >= 0);
-      Array.prototype.splice.apply(bodyReference.body, ([bodyReference.index, 0]: Array<any>).concat((functionBody: Array<any>)));
+      let insertionPoint = instance.insertionPoint;
+      invariant(insertionPoint instanceof BodyReference);
+      Array.prototype.splice.apply(insertionPoint.body, ([insertionPoint.index, 0]: Array<any>).concat((functionBody: Array<any>)));
     }
 
     if (requireStatistics.replaced > 0 && !this.collectValToRefCountOnly) {

diff --git a/src/serializer/types.js b/src/serializer/types.js
@@ -12,13 +12,14 @@
 import { FunctionValue, Value } from "../values/index.js";
 import type { BabelNodeExpression, BabelNodeStatement } from "babel-types";
 import { Completion } from "../completions.js";
+import invariant from "../invariant.js";
 
 export type TryQuery<T> = (f: () => T, onCompletion: T | (Completion => T), logCompletion: boolean) => T;
 
 export type FunctionInstance = {
   serializedBindings: SerializedBindings;
   functionValue: FunctionValue;
-  bodyReference?: BodyReference;
+  insertionPoint?: BodyReference;
 };
 
 export type Names = { [key: string]: true };
@@ -46,9 +47,13 @@ export function AreSameSerializedBindings(x: SerializedBinding, y: SerializedBin
 
 export class BodyReference {
   constructor(body: Array<BabelNodeStatement>, index: number) {
+    invariant(index >= 0);
     this.body = body;
     this.index = index;
   }
+  isNotEarlierThan(other: BodyReference): boolean {
+    return this.body === other.body && this.index >= other.index;
+  }
   body: Array<BabelNodeStatement>;
   index: number;
 }

diff --git a/test/serializer/basic/Bind.js b/test/serializer/basic/Bind.js
@@ -0,0 +1,10 @@
+// does not contain: bind
+(function() {
+function f() {
+  return function() { /* This comment makes this function too big to be inlined */ return 42; }
+}
+
+var f1 = f();
+var f2 = f();
+inspect = function() { return f1() + f2(); }
+})();
diff --git a/test/serializer/basic/BoundFunctionCreationOrder.js b/test/serializer/basic/BoundFunctionCreationOrder.js
@@ -0,0 +1,22 @@
+(function() {
+
+    function wrap(obj) {
+        function A() {
+            return obj.hello();
+        }
+        function B() {
+            return obj.world();
+        }
+        A.B = B;
+        return A;
+    }
+
+    let fooObj = {};
+    let fooFn = wrap(fooObj);
+    let barFn = wrap(fooObj);
+    fooObj.bar = barFn;
+
+    global.foo = fooFn;
+
+    global.inspect = function() { return true; }
+})();