Transitive materialization for Array operators #2456

sb98052 · 2018-08-21T20:17:29Z

This PR implements a step of the way to getting leaked value analysis working for optimized Array operators. It is desirable to leak as little as possible, so that the operators can be specialized to take into account values in the environment in which they run. In the beginning, we are focusing on the narrow range of scenarios in which this is possible. We will start by enforcing the assumptions that we rely on, and make sure that the code that we generate is correct. Once we have correct code, we will start progressively relaxing the assumptions to increase coverage. The overall plan can be found here: #2452.

More specifically, this PR transitively materializes objects reachable via reads to bindings in the optimized function. This is necessary to snapshot the contents of those objects at specialization time.

Fixes #2405.

trueadm · 2018-08-22T10:49:53Z

This looks good. I noticed quite a lot of tests were failing though and after looking into them, it you are materializing intrinsic objects – in this case the React global is one of those that is failing because we are. We shouldn't materialize intrinsic objects at all as far as I'm aware.

Another area I noticed was getLeakedFunctionInfo needs to support BoundFunctionValues:

https://github.com/facebook/prepack/blob/master/src/utils/leak.js#L93

trueadm

Made a few comments on some things that aren't quite right/missing as to why the current React tests on CI are failing.

trueadm

Looks good to me now. Although I found it somewhat harder/longer to review as you squashed your recent changes so I had to read through everything again. :/

sb98052 · 2018-08-23T13:15:06Z

I squashed because I moved a fair bit of code around while proofreading the code, so not sure if a diff would have been useful. The main changes:

Fixed the issue with intrinsics being materialized:
https://github.com/facebook/prepack/pull/2456/files#diff-7c5d0dd678054fc89e7eb35f66582c28R692
Supported bound functions and fixed a few other issues by not treating the main optimized function specially:
https://github.com/facebook/prepack/pull/2456/files#diff-7c5d0dd678054fc89e7eb35f66582c28R609
Fixed some cases in which certain values would not be get traversed.
Added a reduced version of your repro as a test caes (MaterializeObject0). More of these on their way.

sb98052 · 2018-08-23T14:36:35Z

Added a few more tests. Verified that they fail without this changeset, and succeed with it. I will add more tests covering other materialization paths in the future. For now, removing the WIP tag.

NTillmann · 2018-08-23T17:58:09Z

src/values/ArrayValue.js

+    // are used to optimize the nested optimized function. We compute the set of
+    // objects that are transitively reachable from read bindings and materialize them.
+
+    Materialize.materializeObjectsTransitive(realm, func);


What about materializing the abstractArrayValue?

Have an invariant that abstractArrayValue has already leaked?

Yes, the index properties of widened arrays are not backed by locations, so no materialization is needed. I'll add an invariant that fails if this every changes.

NTillmann · 2018-08-23T17:59:03Z

src/types.js

@@ -385,7 +385,8 @@ export type LeakType = {
 };

 export type MaterializeType = {
-  materialize(realm: Realm, object: ObjectValue): void,
+  materializeObject(realm: Realm, object: ObjectValue): void,


Why even expose the single-object materialization? Doesn't seem to be needed, and nobody should call that, right?

Eventually it should go away. Right now, it is dangling out of the leaking implementation.

NTillmann · 2018-08-23T17:59:39Z

src/values/ArrayValue.js

    // Check if effects were pure then add them
    if (abstractArrayValue.nestedOptimizedFunctionEffects === undefined) {
      abstractArrayValue.nestedOptimizedFunctionEffects = new Map();
    }
    abstractArrayValue.nestedOptimizedFunctionEffects.set(funcToModel, effects);
    realm.collectedNestedOptimizedFunctionEffects.set(funcToModel, effects);
  }
+  return;


This return doesn't do anything.

NTillmann · 2018-08-23T18:15:17Z

src/utils/leak.js

+        // #2446
+        if (found && binding.value !== undefined) {
+          if (binding.hasLeaked) {
+            notSupportedForTransitiveMaterialization();


This should be fine (at least the way things are right now), there's simply nothing TODO. Add a comment to this end.

Done. There was a case I wanted to cover, it's now checked in as a test.

NTillmann · 2018-08-23T18:18:10Z

src/utils/leak.js

+      traverse.cache.clear();
+
+      // If the function is not pure, we should not be here
+      invariant(functionInfo.unboundWrites.size === 0);


This is not true. There an assumption based on a dynamic analysis that this should not happen, but here we are doing a static traversal where we might indeed find unbound writes.

True. We ought to use the interpreter to compute the set of non-local bindings: #2478

NTillmann · 2018-08-23T18:18:39Z

src/utils/leak.js

+      let code = func.$ECMAScriptCode;
+      invariant(code != null);
+
+      traverse(


This thing is expensive. We should globally cache the result.

Using the interpreter should eliminate this #2478.

NTillmann · 2018-08-23T18:20:29Z

src/utils/leak.js

+
+      // TODO: Add items to nonLocalReadBindings in passing
+      let nonLocalReadBindings = nonLocalReadBindingsOfFunction(fn);
+      computeFromBindings(fn, nonLocalReadBindings);


What if there are any nonLocalReadBindings left at the end? Shouldn't happen in strict mode, but otherwise there might be some global variable access. What does that mean?

I have added an invariant to ensure that it empties out after rewinding down the scopes, and added an issue #2484 to deal with global environments.

NTillmann · 2018-08-23T18:23:00Z

src/utils/leak.js

+    }
+
+    function computeFromObjectPropertiesWithComputedNames(absVal: AbstractValue): void {
+      if (absVal.kind === "widened property") return;


Why is it okay to just ignore those abstract value kinds here?

My assumption was that these lead to intrinsic values, which do not need materialization, but I have just dropped the exceptions. This call now fails, and will be dealt with as part of #2484.

NTillmann · 2018-08-23T18:26:04Z

src/utils/leak.js

+      } else if (value instanceof AbstractValue) {
+        ifNotVisited(value, computeFromAbstractValue);
+      } else if (
+        value.isIntrinsic() &&


NTillmann · 2018-08-23T18:27:30Z

src/utils/leak.js

+    }
+    function computeFromObjectValue(value: Value): void {
+      invariant(value instanceof ObjectValue);
+      if (!objectsToMaterialize.has(value)) objectsToMaterialize.add(value);


What about arrays and functions which are objects too?

We shouldn't have to add values that have already leaked (and have thus been materialized and all environment interactions go through generator entries).

#2 - Lumped into the larger, #2485.
#1 - The function is handled by line 804, which visits the object properties of a function. We now have an invariant that insures that the array value is intrinsic, and so property accesses are at best temporal.

I have added a test for the function-object case.

But how does that function-object case work when we don't add function values to the objects to materialize?

Could you elaborate what you mean by "don't add function values to the objects to materialize?" An example would help. The main outer function (i.e. the array op) is passed to computeFromFunctionValue which in turn explores its properties.

NTillmann

LGTM. Lots of little things to consider in going forward...

sb98052 · 2018-08-23T21:51:57Z

Thanks. I'll create issues for the points that are not already lumped into future work. Some others, I will resolve before landing.

NTillmann · 2018-08-24T15:43:59Z

src/values/ArrayValue.js

+    // We assume that we do not have to materialize widened arrays because they are intrinsic.
+    // If somebody changes the underlying design in a major way, then materialization could be
+    // needed, and this check will fail.
+    invariant(abstractArrayValue.isIntrinsic());


This invariant now appears twice in the code.

facebook-github-bot