Emit Mutations to Abstract Templates on the AbstractValue instead of the ObjectValue

[sebmarkbage]

Release notes: adds externalTemplate modeling option for template objects

Currently we emit generator entries when we mutate intrinsic objects. When we create abstract objects with a template, we end up giving an intrinsic name to the abstract value, however using the rebuild mechanism we also give a name to the template itself.

This is a bit confusing since now we have two values with the same intrinsic name. This doesn't necessarily lead to bugs by itself but it does generate some weird output.

This PR adds an option to avoid assigning intrinsic names to the template. (I think this should probably just become the default in the future.)

To make mutations work, they are now emitted on the AbstractValue instead of emitting mutations to the template ObjectValue itself. This ensures that the template never leaks out of the AbstractValue wrapper.

My understanding is that this is how this was originally intended to work.

The mechanism that I use is to pass an additional argument "Target" to DefineOwnProperty and Delete. This Value is used as the object of any emitted property mutations. By default it is just the same as the ObjectValue but for AbstractObjectValues with templates, it passes the AbstractObjectValue as the Target instead.

For conditionals, we don't pass the abstract value since we can do better by unwrapping it and referring to each arg.

The primary motivation of this change is to be able to use templates without known identity in a follow up. Such as objects created in loops or residual optimized functions. In that case it is important that we don't leak the underlying template and only ever refer to the abstract value.

This change also omits emitting assignments to array.length when the array is intrinsic since that is implied - when setting numeric properties. This is done automatically by the other assignment. Before this change, we used to unnecessarily add two assignments for all array assignments.

[sebmarkbage]

We use abstract values in many different ways that isn't just defining the surrounding host environment. Those have subtle differences that can't be expressed using the __abstract() helper. E.g. arguments passed to optimized functions are abstract values with intrinsic names. Those objects can be intrinsic objects, whose properties refer to objects that are not intrinsic.

E.g. before this API there's no way to express this:

let notIntrinsic = {};
let intrinsic = __abstract({ nestedObject: notIntrinsic }, "x", { externalTemplate: true });
intrinsic.nestedObject = 12;

While it is not so useful for modeling the host environment, it is useful to be able to write tests for these cases since there are cases where such abstract values do happen in optimized functions etc.

In my next PR, I explicitly want to be able to use the same template in two different abstract values to represent abstract objects with different identity - such as objects created by loops or optimized function. For that, I do need the same template to be able to be shared.

More over this will also happen with loops because the object created in the loop does escape, then we need to use an abstract value to refer to particular instances of that object. So the template and the abstract value can co-exist.

I could wait to write any tests until I have every piece in place and then write a full integration test using loops, but that's very hard to work with compared to an isolated unit test.

I don't like the name externalTemplate. Better ideas? I think we should just drop this

[hermanventer]

For the loop case, when would two different intrinsic abstract objects need to share the same template?

If the abstract value is not intrinsic, there is already no impediment to sharing the template.

It does seem sensible to just drop externalTemplate for now.

[sebmarkbage]

I opened a PR that shows how I intend to use this mechanism: https://github.com/facebook/prepack/pull/2543

[sebmarkbage]

Dropping the externalTemplate option right now means dropping rebuildNestedProperties from the __abstract helper. https://github.com/facebook/prepack/blob/master/src/realm.js#L1620

This is a potential breaking change since our internal modeling code may depend on nested objects being assigned intrinsic names automagically. So I'd like to change that code first and then drop the option in a follow up when it's safe.

[hermanventer]

I don't understand why dropping the externalTemplate option requires dropping rebuildNestedProperties. I also don't understand why #2543 requires you to have two distinct intrinsic abstract objects that share the same template object.

[sebmarkbage]

If I use rebuildNestedProperties then the template is also assigned the same intrinsic name so my test case here wouldn’t have failed before.

If I use it with the same template in two abstracts, then it causes an invariant.

So the reason I need two abstract values is the case mentioned in #2543.

It’s possible to get access to multiple instances from the object value created inside the loop body.

let a = obj.x;
let b = obj.y;
a.someProperty = 1;
result = b.someProperty;

These may share the same identity or not. So I don’t know if a === b and consequently I don’t know if b.someProperty will have been updated or not.

The simplest way to model this is to have all of them share the same template.

[sebmarkbage]

The __abstract helper lets me write unit tests for this without invoking the whole loop logic too. https://github.com/sebmarkbage/prepack/blob/9a7a69f7c8dbd4f704d2a69f0c7da3f809c00ae9/test/serializer/abstract/WidenedIdentity4.js

However these templates I suspect will be helpful for modeling return values too. We need a way to model that a function always returns the same object or different instances every time or different instances but those instances may be the same as the return value from another function.

In that case you’d share the template as the return value of two different functions.

[hermanventer]

I am deeply uncomfortable with this. We talked about how to use two templates for one abstract object value as a way to indicate that the identity of the abstract object is not known and possibly not unique.

Going from there to having two abstract objects that share the same template object is not obvious to me.

[sebmarkbage]

If I read out two properties from an array created in a loop body:

let a = array[0];
let b = array[1];
a.x = 1;

How do I represent that I want to mutate whatever is in slot 0?

There needs to be a new abstract value derived every time I extract a new value from a widened property. That already happens.

If I don’t share the template there needs to be some other thing that is shared since they might also be the same object.

[sebmarkbage]

An abstract value with two templates already means something else. It means that it can possibly be one of those two exact identities. Eg a conditional object and the code generated even compares the identities.

Regardless it needs some differentiator that is different than just two templates because the deep mechanism of the object model (such as the special cases in ArrayValue, deep inside ValuesDomain and many other places doesn’t have knowledge of the set of template). So you need some kind of flag that travels with the object or a different type of class.

However once you have that there is little need to keep two objects around that are always identical. It’s just unnecessary memory.

And regardless we need multiple abstract values as mentioned above.

[hermanventer]

We did indeed discuss the need for a flag. Perhaps there is no need for multiple templates in that case. We'll see.

I don't recall us discussing the scenarios you allude to above and in #2543. These are not supported by a simple widened object abstraction and we should take some time to hash out what can be done about them and how to do it.

At this stage I am deeply skeptical that letting two abstract objects that may have distinct identities share the same template object, can possibly be a safe solution. Let's put this PR on ice and follow up with an depth face to face discussion about the underlying problem.

[sebmarkbage]

I have a sequence of other PRs that all build on this strategy so we should do this soon (tomorrow) if so since that is blocking all of the future work.

[sebmarkbage]

I've gone very deep into these in various branches the past few months. So I at least know a bunch of things that will not work or result in a lot of duplicated code. :) This strategy seems simple and covers the needs for I've found so far.

[facebook-github-bot]

Hi @sebmarkbage!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!