Summary:
Release Notes: None
Previous PRs fixed these issues, but didn't necessarily add the test case from the issue. This PR adds the exact test cases from the issues.
Closes#2358Closes#2398
Pull Request resolved: https://github.com/facebook/prepack/pull/2410
Differential Revision: D9259118
Pulled By: cblappert
fbshipit-source-id: 3eb347044865a11e2ce5262b2338b7b6e1958c60
Summary:
Release notes: none
Fixes#2393. I created the test as a React test because I couldn't see any way of making a serializer test fail if there was a warning without making many other tests fail or without adding another `// some config` to the test (which I believe we said we would not add anymore as there's already too many). React tests will always fail on side-effect warnings, so this takes better advantage of this.
Pull Request resolved: https://github.com/facebook/prepack/pull/2394
Differential Revision: D9249714
Pulled By: trueadm
fbshipit-source-id: 0e11d13e575eca3b6bd75e437eb083b9294a3aee
Summary:
Release Notes: None
Previously, `tryGetOptimizedFunctionRoot` would only get the root successfully if the root was one of the optimized functions or the parent of one of the residual functions. Now we look at the optimized functions as well as any of their parents. Added 2 test cases.
Fixes#2399
Pull Request resolved: https://github.com/facebook/prepack/pull/2401
Differential Revision: D9242153
Pulled By: cblappert
fbshipit-source-id: 9bdffce23d5dd19d9ce5a6b09e1ee9647ca6e5cb
Summary:
Release notes: none
This fixes a bug with nested optimized functions referencing nested residual functions and makes our internal bundle compile.
The issue was that we weren't checking if the function we get from `tryGetOptimizedFunctionRoot` was defined inside another optimized function, like we do already 5-6 lines up in the other if statement. This is an important thing, as the next line will result in `undefined` being returned. When `undefined` is returned, we use the `MainGenerator` rather than the `OptimizedFunction` generator, which means all declared values look at the wrong body.
Pull Request resolved: https://github.com/facebook/prepack/pull/2397
Differential Revision: D9223735
Pulled By: trueadm
fbshipit-source-id: 8e1cb1cc1b201b1ae1a804bc61a4bdc8790a3eea
Summary:
Release notes: Progress in support for nested optimized functions
This fixes#2392.
Control flow of nested optimized functions is independent of
their parents. This change reflects that.
Added regression tests.
Pull Request resolved: https://github.com/facebook/prepack/pull/2395
Differential Revision: D9204607
Pulled By: NTillmann
fbshipit-source-id: 1c75ff7493871a4abc3c36dc63f00663c5f6e31b
Summary:
Release Notes: None
When optimizing a function fails, we can usually continue to emit a correct program while ignoring that call to `__optimize`, thus most of these errors should be `Warning`s and not `FatalError`s.
A few times (like when we have multiple sets of `Effects` for one optimized function), we can sensibly continue Prepacking code, but our output may be incorrect. In these cases, we can issue a `RecoverableError` instead of a `FatalError`
Pull Request resolved: https://github.com/facebook/prepack/pull/2371
Differential Revision: D9186240
Pulled By: cblappert
fbshipit-source-id: c30fc63fb713a3bf504cc1ec4f02410a8566cc42
Summary:
When we create a joined descriptor in an abstract object we need to preserve any attributes of that property that were already there. To do that, we just need to read the first descriptor of the elements since all elements must have the same attributes (or we fatal).
Pull Request resolved: https://github.com/facebook/prepack/pull/2390
Differential Revision: D9193129
Pulled By: sebmarkbage
fbshipit-source-id: 32db3e88ae65de9a09ff6de72ed5b91ec02cf090
Summary:
Attempting to fix#1547.
**Summary**:
I started work with supporting the `return` keyword in switch statements. I came to the method of "pushing" the return up through the `switch` statement by throwing the `ReturnCompletion`.
Next, I worked on the code snippet provided in #1547, which required supporting `ForkedAbruptCompletion`. I again came to the conclusion of just throwing the `Completion`, but I am not positive this is the correct way. Any guidance here would be appreciated.
Then it was adding support for `ThrowCompletion`, which ended up being the same method.
Since `continue` can only be used within a loop, I made a small test using static variables all known at optimization time, and prepack correctly evaluated the function with no additional changes necessary. However, using a variable not known at optimization time yielded an error with tag PP0037.
*Note*: I left the `if (r instanceof PossiblyNormalCompletion) {...}` throw untouched in-case I am missing what some of the possible completion types are that might trigger this (other than the ones I have touched).
**Tests**:
1. Simple Return
```javascript
function f(x, c) {
switch (x) {
default: return 42;
}
}
```
```javascript
var _0 = function (x, c) {
return 42;
};
```
2. Cases Return
```javascript
function f(x, c) {
switch (x) {
case 0: return 12;
case 1: return 24;
default: return 42;
}
}
```
```javascript
var _0 = function (x, c) {
var _9 = 0 === x;
var _6 = 1 === x;
var _3 = _6 ? 24 : 42;
var _1 = _9 ? 12 : _3;
return _1;
};
```
3. Task Snippet
```javascript
(function () {
function f(x, c) {
switch (x) {
case 0: if (c) return 42; else return 99;
case 1: return 23;
}
}
__optimize(f);
global.inspect = function() { return f(0, 1); }
})();
```
```javascript
(function () {
var _$0 = this;
var _1 = function (x, c) {
var _D = 0 === x;
var _3 = c ? 42 : 99;
var _A = 1 === x;
var _7 = _A ? 23 : void 0;
var _2 = _D ? _3 : _7;
return _2;
};
var _0 = function () {
return _1(0, 1);
};
_$0.inspect = _0;
}).call(this);
```
4. Throw Support
```javascript
function f(x, c) {
switch (x) {
case 0: throw 12;
case 1: throw 24;
default: throw 42;
}
}
```
```javascript
var _0 = function (x, c) {
var _1 = 0 === x;
if (_1) {
throw 12;
} else {
var _5 = 1 === x;
if (_5) {
throw 24;
} else {
throw 42;
}
}
};
```
5. Continue Support (optimization-time known variables)
```javascript
function f() {
let counter = 0;
for (let i = 0; i < 5; i++) {
switch (i) {
case 0: counter++; break;
case 1: counter += 2; break;
case 2: counter += 3; break;
case 3: continue;
default: return counter;
}
}
}
```
```javascript
var _0 = function () {
return 6;
};
```
6. Continue Support (unknown variable):
```javascript
function f(max) {
let counter = 0;
for (let i = 0; i < max; i++) {
switch (i) {
case 0: counter++; break;
case 1: counter += 2; break;
case 2: counter += 3; break;
case 3: continue;
default: return counter;
}
}
}
```
```
In stdin(3:23) FatalError PP0037: failed to recover from a for/while loop bail-out due to unsupported logic in loop body (https://github.com/facebook/prepack/wiki/PP0037)
Prepack failed, reporting 1 fatal error.
```
Pull Request resolved: https://github.com/facebook/prepack/pull/2309
Reviewed By: hermanventer
Differential Revision: D9169333
Pulled By: zjijz
fbshipit-source-id: 3a9b2738679fcd6dac8fff9881b7464c85243723
Summary:
Release notes: none
Fixes a `defaultProps` bug and also improves bloat slightly by ensuring we state that the default props helper can be omitted if the value it mutates is never used. Furthermore, added `isPure` to the binary expression temporal, so if the value created from it never gets used, then we don't emit the action.
Pull Request resolved: https://github.com/facebook/prepack/pull/2391
Differential Revision: D9179550
Pulled By: trueadm
fbshipit-source-id: b5671a725e347be3876a04b927eef99977757bd8
Summary:
Resolves#2186. Embeds the value `__empty` in array and object literals for properties that are conditionally set, instead of handling such values via assignments and deletes. Raises an exception if there is a cycle in object or array creation (InstantRender does not support these).
Pull Request resolved: https://github.com/facebook/prepack/pull/2364
Differential Revision: D9169335
Pulled By: sb98052
fbshipit-source-id: f83d85677b30f10f3c548349e93ce792fc6c1ca0
Summary:
Release Notes: None
We weren't properly adding leaked bindings to the optimized function's FunctionInstance, so state didn't get cleaned up properly between passes of the serializer.
There was also a bug in emitter invariants. Loosening the invariant causes the test cases to pass.
Also the invariant seems to have been incorrect in the first place. I suspect it is just the direct negation of [this line](393624f00f/src/serializer/Emitter.js (L103)).
Added slightly minimized repros from the issue.
I am not quite sure what we are trying to test with the last line of this invariant. I think what we're trying to test with the last line of this invariant is: If we're emitting to an additional function, it must be the case that referencedDeclaredValues has `val`. Here the test also passes when `referencedDeclaredValues.has(val)` is `false` which seems wrong to me.
I'm also dubious that top level optimized functions don't consider the MainGenerator as their parent generator, as this means that any derived ids declared in the MainGenerator shouldn't be considered declared in optimized functions (was able to repro that in conditions2 test after tightening invariants). For the sake of unblocking React Compiler, I'll look into that in a future PR.
Solves #2359.
Pull Request resolved: https://github.com/facebook/prepack/pull/2382
Differential Revision: D9158973
Pulled By: cblappert
fbshipit-source-id: a6915d0f125708739022d8576d5f007107e76b6b
Summary:
Release notes: none
Follow up to https://github.com/facebook/prepack/pull/2346. This PR cleans up all the React specific nested optimized function logic, removing most of the old legacy code in favor of the simpler/cleaner approach.
This also introduces a `bubbleSideEffectReports` parameter to `evaluatePure` for specific case where we don't want side-effects to be reported to parent `evaluatePure` calls; necessary for the React reconciler to not throw a `UnsupportedSideEffect` which will cause the reconciliation process to hard-fail (instead nested optimize functions bail-out to be inline abstract function calls like before, and we do not try and optimize the nested function).
Lastly, we now throw an `SideEffect` object when evaluating a nested optimized function and detecting a side-effect. There's no point in continuing evaluation of a given side-effectful function, so this speeds things up a bit by terminating execution.
Pull Request resolved: https://github.com/facebook/prepack/pull/2348
Differential Revision: D9138290
Pulled By: trueadm
fbshipit-source-id: 6d468ea0430be0c0190ff9817b2872df368c325d
Summary:
Release notes: None
- Make formatting of error locations more useful.
- Factored out redundant code that initialize ECMAScriptSource functions
(and as part of that possibly a small improvement in locality: record
`uniqueOrderedTag` only the first time a function is encountered,
not effectively the last time)_
- Made PP1003 recoverable (not sure what's fatal about it)
- Deleted dead `functionExpressions` property
Pull Request resolved: https://github.com/facebook/prepack/pull/2352
Differential Revision: D9136199
Pulled By: NTillmann
fbshipit-source-id: 279efcc97310ccdb11907b74f4f166851b816819
Summary:
Release Notes: None
Fixes#2266.
Looking at the `__optimize` code, what we want is for every call to `__optimize` to optimize that function, even in conditonal contexts. In this case, it makes no sense to store the functions to optimize in a special global because we don't care about the applying/reverting of effects. It simplifies the code significantly to be able to store these values in the realm instead.
The only case where this would matter is if effects containing an __optimize call get created then discarded (I am unsure if there are any legitimate cases of this in Prepack at the moment, but I created a CompilerDiagnostic just in case).
Pull Request resolved: https://github.com/facebook/prepack/pull/2303
Differential Revision: D9134968
Pulled By: cblappert
fbshipit-source-id: 040f7ccc24f928e6b8daa068521d3848caffc4d2
Summary:
Release Notes: None
Now bindingAssignments will have their containing optimized function visit them.
Fixes#2335 and test case 3 of #2252
Test output currently fails lint, once #2339 lands and I rebase past it, that'll go away.
Pull Request resolved: https://github.com/facebook/prepack/pull/2344
Reviewed By: trueadm
Differential Revision: D9133987
Pulled By: cblappert
fbshipit-source-id: 243fc56a6ff7552c801762e2668ea5e4607599fd
Summary:
Release notes: None
- Remove some dead code from test runner.
- Simplify test runner by removing option that's only used once for no good reason.
Pull Request resolved: https://github.com/facebook/prepack/pull/2350
Differential Revision: D9125266
Pulled By: NTillmann
fbshipit-source-id: 04eb9c17d8a3e7b2f93c246652691759e8a797c8
Summary:
Release notes: unknown abstract arrays with numeric properties now have their map methods auto optimize as nested optimized functions
This PR introduces automatic collection of nested optimized functions for abstract array methods (for now just `Array.prototype.map` and `Array.from`). It also re-works part of the React nested optimized function logic for unknown arrays so that the same approach is used for both optimized functions and React component trees. If the nested optimized function has side-effects when evaluating it (i.e. it's not a "pure function") then we don't try and treat it as an nested optimized function and fallback to havocing the function as we did before.
In the interests of keeping this PR small: **this PR is one of many** to fully add support, support edge-cases and remove legacy implementations. I plan on following up to this PR with another that gets rid of a lot of duplicate logic in the React reconciler to do with nested optimized functions and unify it to a single place.
Pull Request resolved: https://github.com/facebook/prepack/pull/2346
Differential Revision: D9109608
Pulled By: trueadm
fbshipit-source-id: 77e7fcd5f514caf14607a9a48bb8ff149fe221b2
Summary:
Release notes: none
In some places we have type information but don't fully use the benefits it brings and instead go down a non-optimal path. This PR addresses two fairly simple cases where this occurs:
- when we have a method call on an abstract value that has a primitive type, where the name of the method matches that of one of the methods on the primitive prototype, then use that method.
- when we have a type of "array", use all the logic for unknown abstract arrays rather than generate an abstract value with a type of array (which has no optimized paths, unlike unknown arrays).
- adds temporals for `toString` and other built-ins because of existing logic around joining them and the fact that they can throw if called on `undefined`, it makes them unsafe. Previous we rarely went down these code paths to create template abstracts, because we guarded in pure scope for abstract base values in `CallExpression`.
Pull Request resolved: https://github.com/facebook/prepack/pull/2321
Differential Revision: D9081455
Pulled By: trueadm
fbshipit-source-id: 5be2ec87eb582d50f7b4f72ef08debee8977b0e2
Summary:
Release Notes: None
The original issue here was that `nested` is defined inside of `fn2` which is a non-optimized function called by `fn` (an optimized function). That caused Prepack to not detect that `nested` was nested in `fn2`.
The fix is to use `CreatedObjects` to test for nesting instead of the environment lookup. The environment lookup fails because `nested` is evaluated with `fn2`'s effects applied but _not in `fn2`'s environment_.
This PR also adds a command I use frequently to test a single failing `test-runner` test as well as a way to skip lint because some tests can't pass lint.
Addresses the first test case of #2337.
Pull Request resolved: https://github.com/facebook/prepack/pull/2339
Differential Revision: D9074916
Pulled By: cblappert
fbshipit-source-id: 720003b965d9a9a6842d512ea41cd6402361342e
Summary:
Release notes: none
Changes invariant to accept SymbolValue as well as strings. I found to be hitting this invariant in the internal bundle. I'll put up a regression test next week as this is a tricky one to repro it seems.
Pull Request resolved: https://github.com/facebook/prepack/pull/2333
Differential Revision: D9046722
Pulled By: trueadm
fbshipit-source-id: 1fc43f17c8e4e637aeaa3156319b6ae4995c7dad
Summary:
Fixes#2322. The React Compiler was discarding keys for components that were inlined.
Consider:
```jsx
const React = require("react");
__evaluatePureFunction(() => {
function Lambda(props) {
return [<div key="0" />, <Omega key="1" />, <Omega key="2" />];
}
function Omega(props) {
return <div />;
}
__optimizeReactComponentTree(Lambda);
module.exports = Lambda;
});
```
```jsx
(function() {
var _2 = function(props, context) {
_4 === void 0 && $f_0();
return [_4, _7, _7]; // <------------------------- `_7` has no `key`!
};
var $f_0 = function() {
_4 = <div key="0" />;
_7 = <div />;
};
var _4;
var _7;
module.exports = _2;
})();
```
---
Cases where this can be an issue:
- First render mode and we need to hydrate instances based on keys (will this be a thing?)
- Compiled component is re-rendered and a list has a different order. List item components are expensive.
- We inline components with state.
Of course, it’s possible this may be a non-issue.
My solution for this was to wrap the inlined React Element in a `<React.Fragment>` with a key. So my input above becomes:
```jsx
[
<div key="0" />,
<React.Fragment key="1"><div /></React.Fragment>,
<React.Fragment key="2"><div /></React.Fragment>,
]
```
Cloning inlined elements and adding keys is a fragile operation since the element may already have a key or the element may be some other React node like a portal. I opted for wrapping in `<React.Fragment>`. Note that this also means we can hoist `<div />` in the example above.
Thoughts on adding an optimization which clones the element and adds `key={x}` in cases where this is possible? `<React.Fragment>` seems correct in all cases, but cloning with `key={x}` when possible seems like it might be faster.
Also happy to hear other suggestions for maintaining the key on inlined elements.
This has no impact on our internal bundle.
Pull Request resolved: https://github.com/facebook/prepack/pull/2324
Differential Revision: D9034942
Pulled By: calebmer
fbshipit-source-id: 2cdeec611a2c1f67059fa093684b5b51e0bbe809
Summary:
In case the havoced object has a setter on it.
This is fixes this particular bug but it's also useful to be able to pass the correct receiver to properties.js rather than unwrapping it like we do now. This will be evident in a follow up PR.
This also lets emitPropertyAssignment deal with abstract values which is a common pattern and will become more common with widened objects.
Pull Request resolved: https://github.com/facebook/prepack/pull/2338
Differential Revision: D9035746
Pulled By: sebmarkbage
fbshipit-source-id: 2abb1a3eb047de1739dec94259a803c4c45e416d
Summary:
Coercing a symbol to a number or primitive implicitly always throws an error. We generated fatal compiler errors. Instead, this should just be treated as a throw.
This is neat because that means other serializers doesn't have to deal with the expressions involving symbols.
Pull Request resolved: https://github.com/facebook/prepack/pull/2300
Differential Revision: D9035734
Pulled By: sebmarkbage
fbshipit-source-id: 60c0d089e63b7b36b6757dab0c9928c19514c5d6
Summary:
Follow up to #2219
This also includes two relevant bonus fixes: 2aa1ad35a4 and e96ac8c0c1
Pull Request resolved: https://github.com/facebook/prepack/pull/2329
Differential Revision: D9033950
Pulled By: sebmarkbage
fbshipit-source-id: b28b608b7449f0bbd7781c7d6aab5717d638d904
Summary:
This solves the remaining issues in #2300.
If a binary expression can't be resolved as pure (such as when applied to values of two different types), we can instead try conditions separately and join the effects.
If the left value is conditional, we try each possible value separately until we hit a non-conditional value. Then we try any conditional right values.
Pull Request resolved: https://github.com/facebook/prepack/pull/2332
Differential Revision: D9032487
Pulled By: sebmarkbage
fbshipit-source-id: 0f89a680ad0b710e90c1dd7a4c127c2247527c99
Summary:
This pull request breaks out the first part of #2185, which deduplicates object creation when leaking is unconditional to begin with. No changes are made to how leaking works. Only to how leak information is used by the serializer and visitor.
Pull Request resolved: https://github.com/facebook/prepack/pull/2302
Differential Revision: D8997109
Pulled By: sb98052
fbshipit-source-id: e0ea2e7ea574a9f2be71b70be289831c7f797d9d
Summary:
After adding classes to my fuzzer (which does not use first render mode) I found #2316. This PR fixes#2316 and a related issue I found while working on that. (Each fix is in a separate commit.) The second issue I found is scarier since the compiler passes but we get invalid output.
In the following example I observed an invariant violation:
```js
const React = require("react");
__evaluatePureFunction(() => {
function Tau(props) {
return React.createElement(
"div",
null,
React.createElement(Epsilon, {
a: props.z,
}),
React.createElement(Zeta, {
p: props.h,
})
);
}
class Epsilon extends React.Component {
constructor(props) {
super(props);
this.state = {};
}
render() {
return React.createElement(Zeta, { p: this.props.a });
}
}
function Zeta(props) {
return props.p ? null : React.createElement("foobar", null);
}
__optimizeReactComponentTree(Tau);
module.exports = Tau;
});
```
```
=== serialized but not visited values
=== visited but not serialized values
undefined, hash: 792057514635681
referenced by 1 scopes
=>_resolveAbstractConditionalValue alternate(#75)=>ReactAdditionalFunctionEffects(#80)
Invariant Violation: serialized 26 of 27
This is likely a bug in Prepack, not your code. Feel free to open an issue on GitHub.
at invariant (/Users/calebmer/prepack/src/invariant.js:18:15)
at ResidualHeapSerializer.serialize (/Users/calebmer/prepack/src/serializer/ResidualHeapSerializer.js:2465:17)
at statistics.referenceCounts.measure (/Users/calebmer/prepack/src/serializer/serializer.js:259:15)
at PerformanceTracker.measure (/Users/calebmer/prepack/src/statistics.js💯14)
at ast (/Users/calebmer/prepack/src/serializer/serializer.js:238:38)
at statistics.total.measure (/Users/calebmer/prepack/src/serializer/serializer.js:169:17)
at PerformanceTracker.measure (/Users/calebmer/prepack/src/statistics.js💯14)
at Serializer.init (/Users/calebmer/prepack/src/serializer/serializer.js:136:35)
at prepackSources (/Users/calebmer/prepack/src/prepack-standalone.js:66:33)
at compileSource (/Users/calebmer/prepack/scripts/debug-fb-www.js:92:18)
```
Somehow we were visiting `undefined`, but clearly we weren’t serializing it given the source code. Here’s what was happening:
- We’d visit an additional function calling `withCleanEquivalenceSet`.
- The additional function would enqueue an action which visited `<foobar>`.
- Later, we’d execute the `<foobar>` visiting action outside our `withCleanEquivalenceSet` with our default equivalence set.
- The same thing happens with our new root from our `Epsilon` class.
- Except now some effects have been applied that set the `type` for our `<foobar>` React element in our React equivalence set to `undefined`. Since when we created `<foobar>` we modified its `type` property. (Recorded in `modifiedProperties`.)
- Now our `<Zeta>` in `<Tau>` and our `<Zeta>` in `<Epsilon>` share the _exact same_ `<foobar>` thanks to our equivalence set.
- But `<foobar>` has a `type` of `undefined` thanks to the effects we applied. We should be creating a new `<foobar>` since we are in a new optimized function.
- ***Boom!*** We visit `undefined`, but don’t serialize it since the same effects aren’t applied when we serialize.
This test case caught an important flaw in our visiting logic, but it only manifested as an invariant under these very specific conditions. Which is a little scary. In a large example, like our internal bundle, we would of course serialize some `undefined` value but we would have still visited `undefined` instead of the proper type, _and_ we may consider two elements to be equivalent when we shouldn’t since their components may render independently. This issue (I presume) can also affect we bail out on since they create new trees inside the root tree.
While debugging and fixing this issue, I found another with incorrect/suboptimal output that passes Prepack and passes eslint. Given the following input:
```js
require("react");
__evaluatePureFunction(() => {
const React = require("react");
function Tau(props) {
return React.createElement(
"a",
null,
React.createElement("b", null),
React.createElement(Epsilon, null),
React.createElement("c", null)
);
}
class Epsilon extends React.Component {
constructor(props) {
super(props);
this.state = {};
}
render() {
return React.createElement("d", null);
}
}
__optimizeReactComponentTree(Tau);
module.exports = { Tau, Epsilon };
});
```
We get this output:
```js
(function () {
var _$0 = require("react").Component;
var _3 = function (props, context) {
_6 === void 0 && $f_0();
_9 === void 0 && $f_1();
var _8 = <_B />;
var _4 = <a>{_6}{_8}{_9}</a>;
return _4;
};
var _B = class extends _$0 {
constructor(props) {
super(props);
this.state = {};
}
render() {
return _E; // <--------------------------- Incorrect. `_B` may be rendered outside of `_3`.
}
};
var $f_0 = function () {
_6 = <b />;
_E = <d />;
};
var _6;
var _E;
var $f_1 = function () {
_9 = <c />;
};
var _9;
var _0 = {
Tau: _3,
Epsilon: _B
};
module.exports = _0;
})();
```
This happened because the React serializer’s `_lazilyHoistedNodes` logic was implemented in a way that doesn’t play well with the interleaving almost random order of the serializer invocation of React lazily hoisted nodes logic.
Pull Request resolved: https://github.com/facebook/prepack/pull/2318
Differential Revision: D8992253
Pulled By: calebmer
fbshipit-source-id: 4a75e5768ffb7887c3a8afa2a0f3f59e7eac266d
Summary:
I was seeing `throwIfNotConcrete` a lot for `String()` calls on abstract values in #2297. The fix is relatively quick.
Pull Request resolved: https://github.com/facebook/prepack/pull/2310
Differential Revision: D8960761
Pulled By: calebmer
fbshipit-source-id: f378092cb3e36ac4026404a9e185abe9a768f577
Summary:
Release Notes: None
PR solved by hermanventer's recent PR, just adding another test case closer to the one in the issue.
Resolves#2248
Pull Request resolved: https://github.com/facebook/prepack/pull/2272
Differential Revision: D8938294
Pulled By: cblappert
fbshipit-source-id: 18e4fd3b8913400b98242eff06f011ed0140459b
Summary:
Release notes: Enhanced dead code elimination for optimized functions
This resolves#2276: Visiting+emitting modified bindings now participates
in the fixed point computation, checking for whether the modified binding
is actually getting visited for other reasons.
The previously existing code that did CSE by mutating the generator entry
got removed, as it wouldn't work exactly like that anymore (can't return value
because actual analysis gets postponed for fixed point computation). Filed #2286
to keep track of that and more needed changes around CSE for values stored in locations
participating in effects tracking. --- Turns out that we didn't have a test anyway
that would detect whether binding CSE happens or not.
Added regression test.
Pull Request resolved: https://github.com/facebook/prepack/pull/2287
Differential Revision: D8902345
Pulled By: NTillmann
fbshipit-source-id: 6ba3fb9755312956829050f0278dee2d85d6a261
Summary:
This change allows modeling of optimized function arguments in different way than regular environment modeling. The main point of this modeling is being able to determine what getter should be used at this particular GraphQL property access at compile time. To achieve this Prepack should know the shape of arguments used for optimizing functions and should be able to infer the shape of value when it is needed.
Shape information is attached to AbstractValue (to pass it around) and on every property access a new AbstractValue is returned with shape information specific to this property.
As an output of this process, every member access to modeled values is replaced by function call like prop_string(obj, "key").
Structure of the model can be found in `ShapeInformation.js`.
Pull Request resolved: https://github.com/facebook/prepack/pull/2215
Reviewed By: NTillmann
Differential Revision: D8874743
Pulled By: hotsnr
fbshipit-source-id: 9e1b2254ef54986229be7d1195c1586b95d9a4be
Summary:
Release notes: None
IsCall used to fail on abstract values even if they have a set of values in their domain.
This in turn would make `typeof` not work on abstract values.
This makes it work, and adds a regression test.
Pull Request resolved: https://github.com/facebook/prepack/pull/2268
Differential Revision: D8878641
Pulled By: NTillmann
fbshipit-source-id: f6dda513786eaf3f02bd13d5c6474c2f20fb0082
Summary:
Release note: none
Generators that end up in joined effects, need to disappear from the generators in the forked completions from which they have been extracted. Also, they need to be applied to the generator containing the timeline in which the fork occurs, before the fork occurs.
With some luck, this may fix a lot of issues.
Pull Request resolved: https://github.com/facebook/prepack/pull/2274
Differential Revision: D8878320
Pulled By: hermanventer
fbshipit-source-id: a6cd41bf9e82bc4e362bda32a1e32c5fc90298cf
Summary:
Fixes#1867 and fixes#2058 which are issues blocking the React compiler.
Test case from #1867 copied below:
```js
(function() {
function fn(arg) {
if (arg.foo) {
if (arg.bar) {
return 42;
}
}
}
if (global.__optimize) {
__optimize(fn);
}
global.inspect = function() {
return JSON.stringify([
fn(null),
fn({}),
fn({foo: true}),
]);
};
})();
```
The fix is almost entirely in `joinOrForkResults` where I remove the call to `updatePossiblyNormalCompletionWithConditionalSimpleNormalCompletion` and replace it with a new `PossiblyNormalCompletion`. Here’s my understanding of the issue and the previous strategy. Hopefully hermanventer can correct me if I’m wrong or explain why it needs to be the way it was.
In our program above when we are joining the `arg.foo` consequent with its alternate we have the following completion for the `arg.foo` consequent:
- `PossiblyNormalCompletion` (join condition is `arg.bar`)
- `ReturnCompletion(42)` (value of `42`)
- `SimpleNormalCompletion(empty)` (there is no `else`)
We are joining this with an alternate of `SimpleNormalCompletion` since there is no `else` on the `arg.foo` if-statement. Before we would try and “sneak into” the two branches and add an abstract conditional to the result. However, the implementation was limited so we we only updated `SimpleNormalCompletion`. This meant we ended up changing our above completion to:
- `PossiblyNormalCompletion` (join condition is `arg.bar`)
- `ReturnCompletion(42)`
- `SimpleNormalCompletion(arg.foo ? empty : empty)`
The only change being the second `SimpleNormalCompletion` is now `arg.foo ? empty : empty`. This would serialize into roughly:
```js
function f() {
return _$1 ? 42 : undefined;
}
```
Where `_$1` is not declared! Since we were serializing a `PossiblyNormalCompletion` with a join condition of `arg.bar` and the `arg.foo ? empty : empty` abstract conditional simplified to just `empty` which means we never emit `arg.foo` so never emit `arg.bar` which has a dependency on `arg.foo`.
My strategy was to nest the `PossiblyNormalCompletion` in another `PossiblyNormalCompletion`. So the final completion for the code sample at the top of this PR is:
- `PossiblyNormalCompletion` (join condition is `arg.foo`)
- consequent: `PossiblyNormalCompletion` (join condition is `arg.bar`)
- consequent: `ReturnCompletion(42)`
- alternate: `SimpleNormalCompletion(empty)`
- alternate: `SimpleNormalCompletion(empty)`
I’m confused as to why this was not done in the first place and instead an update strategy was used.
I then wrote a test (now in `test/serializer/optimized-functions/NestedConditions.js`) to enumerate a bunch of nested conditional cases. The rest of the fixes come from there. Notably the refactor of `replacePossiblyNormalCompletionWithForkedAbruptCompletion` which did not produce the same output when `consequent` and `alternate` were flipped.
Pull Request resolved: https://github.com/facebook/prepack/pull/2255
Differential Revision: D8865130
Pulled By: calebmer
fbshipit-source-id: 4964acc11b6403b9956cf29decefb1971340516d
Summary:
Release notes: none
When we check to do the temporal `Object.assign` merge optimization, we should properly account for any "prefix" args that occur before "to" and the "suffix" args. Test case added that came up in testing.
Pull Request resolved: https://github.com/facebook/prepack/pull/2261
Differential Revision: D8874714
Pulled By: trueadm
fbshipit-source-id: 59869f556c1300424c5a1b59d3df1fdd139b41fe
Summary:
Release note: none
This fixes a problem reported as a comment in PR #2258.
The underlying problem was that Realm.evaluateForEffects would just append the normal effects, captured since the last non complete join, into the saved partially normal completion that becomes the result of the returned Effects. This is not quite right because applying the resulting Effects should restore the complete set of normal effects that were in force when evaluateForEffects completed.
Pull Request resolved: https://github.com/facebook/prepack/pull/2271
Differential Revision: D8870763
Pulled By: hermanventer
fbshipit-source-id: 027924f2b250174393c60d90c2feefdb5a55eec3
Summary:
Release note: none
The main part of this PR is to ensure that completions and the effects they complete are always 1-1.
Along the way some bugs got fixed#2241.
This also includes a little side project: limiting the console spew when doing "yarn test-serializer --fast", which is something I do all the time and I'm much happier this way.
Pull Request resolved: https://github.com/facebook/prepack/pull/2258
Differential Revision: D8864448
Pulled By: hermanventer
fbshipit-source-id: a7f257a7e07211ecd6069b84330aa3305609c5d2
Summary:
Since I'm adding a new experiment I figured I'd delete an equivalent sized one.
Last year I added an option that runs the Prepack program by invoking Node.js JS runtime which lets us prepack the whole module system and initialization. It's essentially a packager with perfect Node.js module resolution semantics. It did this by modeling Node's native environment as Prepack bindings.
This PR removes that whole option.
There's a few reasons why I don't think that worked out as a good idea.
- It's not solving a real need. It is hard to keep different module systems in tact. There is always something in the ecosystem that breaks down and using the canonical one solves that. However, in practice, if there is a need for bundling the ecosystem itself adapts to the toolchain. So it's not actually that hard to bundle up a CLI even with Webpack, even if it's strictly not 100% compatible, by tweaking a few downstream depenencies.
- Running the resulting bundle is tricky. The resulting bundle includes the JS parts of Node. This overlaps with what Node.js adds at runtime so it runs it twice. The ideal is actually to build a custom distribution of Node.js but this is generally overkill for what people want.
- Bindings change a lot. While Node.js's API notoriously doesn't change much. The internals do change a lot. By picking the API boundary in the middle of the internals of Node.js, it risks changing with any version. While technically observable changes, nobody else relies on these details. If this option was worth its weight, someone could probably maintain it but so far that has not been the case so we had to disable this option in CI to upgrade Node.
However, going forward I think there are alternative approaches we can explore.
- First class module system. This is something we really need at some point. A first class module system would be able to load Node.js module files from disk and package them up while excluding others. It doesn't have to be literally Node.js's module system. Close enough is ok. Especially as standards compliant ECMAScript modules get more popular. This lets us target compiling output that runs after Node's initialization.
- By introducing havocing and membranes in the boundaries, it becomes possible to initialize Node.js modules without actually knowing the internal of the boundaries.
- We've started optimizing residual functions which is much more interesting. However, this requires that code puts some constraints on how it works with its environment. It's not designed to be fully backwards compatible. That's probably a good thing but that also means that we can put constraints on the modules being Prepacked.
This removes the ability to prepack Prepack itself which is unfortunate but already wasn't being tested. To speed up Prepack itself, the [LLVM backend](https://github.com/facebook/prepack/pull/2264) seems much more useful if it can ever work on Prepack itself.
Pull Request resolved: https://github.com/facebook/prepack/pull/2267
Differential Revision: D8863788
Pulled By: sebmarkbage
fbshipit-source-id: d777ec9a95c8523b3386cfad553d9f691ec59074
Summary:
This fixes an issue where get on abstract top val didn't consider Receiver.
This also illustrates two common patterns of getters that are not pure. One is reading a mutable property known by Prepack and one is lazily initializing a shared mutable property.
I believe we'll want to continue supporting these patterns since they almost always work anyway. Note that supporting these don't add much negative consequence. We can still eliminate unused getters. Most of the time these patterns doesn't happen where the prototype is unknown or havoced.
Usually the getter is abstract because the Receiver itself is unknown or havoced (not its prototype). So havocing even more doesn't have any further negative downside.
Pull Request resolved: https://github.com/facebook/prepack/pull/2257
Differential Revision: D8862827
Pulled By: sebmarkbage
fbshipit-source-id: 7e6e98f8b85b6fceaa5131136cc6163d94f5d289
Summary:
Release notes: upgrades Prepack to use Babel 7.0.0-beta.53
This is a big PR that updates all of Prepack to Babel 7. Babylon is now `babel/parser` and pretty much all of the the previous Babel packages are now located in scoped packages. I had to make a bunch of changes around Jest/Flow/Webpack to get this all working. The build times of building Prepack itself seem considerably faster (easily twice as fast locally). I followed most of the Babel 6 -> 7 upgrade guide from the Babel site in terms of changing nodes and type definitions to match the new ones.
Pull Request resolved: https://github.com/facebook/prepack/pull/2256
Differential Revision: D8850583
Pulled By: trueadm
fbshipit-source-id: 2d2aaec25c6a1ccd1ec0c08c5e7e2a71f78ac2d8
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.
5f7256f17e/src/react/ReactEquivalenceSet.js (L233-L234)
Both `getEquivalentPropertyValue`…
5f7256f17e/src/react/ReactEquivalenceSet.js (L255)
…and `getValue`…
5f7256f17e/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: https://github.com/facebook/prepack/pull/2243
Reviewed By: trueadm
Differential Revision: D8811463
Pulled By: calebmer
fbshipit-source-id: 4f30a75e96c6592456f5b21ee9c2ee3e40b56d81
Summary:
Release note: none
When joining a SimpleNormalCompletion with a PossiblyNormalCompletion, do not move the join condition to a leaf position.
Pull Request resolved: https://github.com/facebook/prepack/pull/2250
Differential Revision: D8838821
Pulled By: hermanventer
fbshipit-source-id: 6e102307af31e2ff2f3fbc7ec7f5ad516df5f7a5
Summary:
Release notes: stop abstract loops from getting stuck in infinite loops when body contains a return, throw or break completion
Fixes https://github.com/facebook/prepack/issues/2053. This PR adds logic to "for loops" where they might get stuck in an infinite loop when there is no incrementor but there is a return, throw or break completion in the loop body. We check this via a new function `andfailIfContainsBreakOrReturnOrThrowCompletion` and only do this after `100` iterations of evaluating the loop body. I was thinking of making the iteration count configurable but wanted feedback on the approach first. I tried `1000` iterations, but it actually ended up taking about 2 minutes to evaluate the test case :/
Pull Request resolved: https://github.com/facebook/prepack/pull/2247
Differential Revision: D8827425
Pulled By: trueadm
fbshipit-source-id: d05f539e2c8a6dce15b7f23c1b76e89087437738
Summary:
Release notes: Object.assign should no longer lose values when snapshotting in certain cases
Fixes https://github.com/facebook/prepack/issues/2240. This PR does a few things:
- Improves the value of Object.assign when snapshots are involved. Previously, the `removeProperties` property passed to `getSnapshot` was being called in all cases, now it only does it when it makes sense – i.e. the next "source" is partial. I've explained this in comments and broken this logic out into its own function.
- Removed the duplicate Object.assign logic for `react/utils`, we no longer need this as the fallback route never gets triggered anymore and it's just dead code.
- Updated the dead code tests with assertions to ensure we check the $ variable matches the Object.assign and also updated a few tests that were affected by this PR (as the Object.assign variable changed).
Pull Request resolved: https://github.com/facebook/prepack/pull/2246
Differential Revision: D8827443
Pulled By: trueadm
fbshipit-source-id: ec600baf7d08916b6c62f0aeaa09d739fba4415b
Summary:
Release note: none
This just removes code that became redundant because of earlier PRs. To put it another way: ErasedAbruptCompletion is now gone!!!
Pull Request resolved: https://github.com/facebook/prepack/pull/2237
Differential Revision: D8811164
Pulled By: hermanventer
fbshipit-source-id: c7ca973f5ab6f3f7b5d4d78f70e2a3590cbe52e6
Summary:
This lets the simplifier do it work and allows intermediate objects to be dead code eliminated.
If this looks good, I'll do $SetPartial and $Delete too.
Pull Request resolved: https://github.com/facebook/prepack/pull/2219
Differential Revision: D8809146
Pulled By: sebmarkbage
fbshipit-source-id: 9fbfa87f5079b60f234ce4c1b2c745ac72a6b2af