Enable and constrain optimized array operator support for InstantRender #2547

sb98052 · 2018-09-13T18:45:35Z

This change ensures that the code generated via optimized array loop operators is correct in the InstantRender setting. It enables the optimization of such functions when the --instantRender option is set. Materialization is prevented by ensuring that post-optimization, objects that are reachable from the function are not mutated. Recoverable errors are issued. We also degrade all InstantRender bailouts to recoverable errors, to facilitate debugging. We add a constraint that optimized functions may not be reused.

Resolves #2451 #2448

facebook-github-bot

NTillmann has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

NTillmann

Looks good. Please make sure that the internal test produces reasonable results before landing.

hermanventer · 2018-09-13T20:55:33Z

src/realm.js

+  number of unsupported scenarios. We group all associated compiler diagnostics here. */
+
+  instantRenderBailout(message: string, loc: ?BabelNodeSourceLocation) {
+    let error = new CompilerDiagnostic(message, loc, "PP0039", "RecoverableError");


We need an actual Wiki page for this. I am not thrilled by this, however. It may be more specific than PP0001, but it is still vague and not actionable. Each call to this routine should instead have its own error code along with a wiki page that gives concrete advice about it.

hermanventer · 2018-09-13T20:59:13Z

src/utils/leak.js

+      // user upgrades the warning to an error.
+      handleMaterialization = o => {
+        let diagnosticCode = "PP0044";
+        if (realm.userChangedDiagnosticToError(diagnosticCode)) {


It is far too subtle to change the semantics of the abstract interpreter based on whether or not a warning was upgraded to an error and then subsequently ignored.

Ok, I'll find another way to do this.

hermanventer · 2018-09-13T21:00:53Z

src/utils/leak.js

+          diagnosticCode,
+          "Warning"
+        );
+        realm.handleError(error);


Never ignore the result of handleError and then carry on. You may only ignore it if you throw a FatalError immediately afterwards.

This I've fixed.

error

Dropped the approach that opened a path to adding policies in Prepack.

NTillmann · 2018-09-14T20:58:47Z

Looks fine to me; except that I expect to run into issues when the everything including the module table is getting marked as final. But we can deal with it when we get there.

sb98052 · 2018-09-17T14:46:13Z

Looks fine to me; except that I expect to run into issues when the everything including the module table is getting marked as final. But we can deal with it when we get there.

@NTillmann Note that accessing such objects lead to recoverable errors for Instant Render.

facebook-github-bot

sb98052 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

informative and recoverable

facebook-github-bot

sb98052 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

hermanventer · 2018-09-17T20:37:48Z

src/methods/properties.js

+    );
+  } else {
+    let error = new CompilerDiagnostic(
+      "Mutating a final object, or an object with unknown properties, after some of those " +


Not your doing, but I hate it when terms related to internal implementation details of Prepack leak out via error messages. Moreover, the text of this message is very obscure and may mislead a programmer who has to make sense of it.

Even worse than that, the wiki page for this error gives an outdated example. Prepack already snapshots and does the right thing for the example. I'm none too sure if there are other cases. If there are none, the whole concept of final objects should be deleted. Otherwise, the wiki page should be updated with examples that cover ALL of the other cases.

Is there an issue for this?

hermanventer · 2018-09-17T20:40:40Z

src/realm.js

+  number of unsupported scenarios. We group all associated compiler diagnostics here. */
+  instantRenderBailout(message: string, loc: ?BabelNodeSourceLocation) {
+    if (loc === undefined) loc = this.currentLocation;
+    let error = new CompilerDiagnostic(message, loc, "PP0039", "RecoverableError");


As I've said before, we should not use the same error code for different error messages. At best we can parameterize a single message. Given that, this abstraction is a trap and should be removed.

hermanventer · 2018-09-17T20:41:52Z

src/methods/properties.js

+  // We can't continue because this object is already in its final state
+  if (realm.instantRender.enabled) {
+    realm.instantRenderBailout(
+      "Object mutations that require materialization are currently not supported by InstantRender",


Materialization is an internal concept and should not leak into error messages. The message is also very obscure. If you can't explain why this is not allowed in clear terms along with a wiki page that has a clarifying example, you are not quite done thinking about this problem.

hermanventer · 2018-09-17T20:46:03Z

src/serializer/ResidualHeapSerializer.js

@@ -1348,14 +1348,10 @@ export class ResidualHeapSerializer {
          to the __empty built-in */
          if (instantRenderMode) {
            if (this.emitter.getReasonToWaitForDependencies(elemVal)) {
-              let error = new CompilerDiagnostic(
+              this.realm.instantRenderBailout(
                "InstantRender does not yet support cyclical arrays or objects",


This error is totally unrelated to the previous call to instantRenderBailout and a perfect example of the trap that I want to remove.

hermanventer · 2018-09-17T20:49:47Z

src/values/ArrayValue.js

@@ -43,6 +43,11 @@ function evaluatePossibleNestedOptimizedFunctionsAndStoreEffects(
      thisValue = func.$BoundThis;
    }
    invariant(funcToModel instanceof ECMAScriptSourceFunctionValue);
+
+    if (realm.instantRender.enabled && realm.collectedNestedOptimizedFunctionEffects.has(funcToModel)) {
+      realm.instantRenderBailout("Array operators may only be optimized once", funcToModel.expressionLocation);


What does this mean? Give an example in a wiki page.

hermanventer · 2018-09-17T20:50:38Z

src/values/ArrayValue.js

@@ -59,6 +64,12 @@ function evaluatePossibleNestedOptimizedFunctionsAndStoreEffects(
      // the default behaviour and leaked the nested functions so any bindings
      // within the function properly leak and materialize.
      if (e instanceof NestedOptimizedFunctionSideEffect) {
+        if (realm.instantRender.enabled) {
+          realm.instantRenderBailout(
+            "InstantRender does not support impure array operators",


What is an impure array operator? Give a motivating example in a wiki page.

hermanventer · 2018-09-17T20:53:53Z

src/serializer/ResidualHeapSerializer.js

-                "PP0039",
-                "FatalError"
+              this.realm.instantRenderBailout(
+                "InstantRender does not yet support cyclical arrays or objects",


Why is this no longer a fatal error? Is it likely that the code generated in this case will do the right thing? If so, I want to see a regression test that does the right thing in this case.

Summary: In support of #2547. Sometimes, it is more desirable (less costly, better for debugging) to flag a warning for a possibly anomalous condition, than to assume that it is anomalous and enforce it as an error. This change exposes warnings that a user upgrades to errors, so that they can then be enforced as such. Pull Request resolved: #2548 Differential Revision: D9884632 Pulled By: sb98052 fbshipit-source-id: c47b57a21aa047b0f3fa2672508f8fb23c04942a

sb98052 requested review from trueadm and NTillmann September 13, 2018 18:45

facebook-github-bot added the CLA Signed label Sep 13, 2018

sb98052 mentioned this pull request Sep 13, 2018

Return correct recovery code for FatalError #2548

Closed

sb98052 force-pushed the irops branch from 70b865c to d797b98 Compare September 13, 2018 19:51

facebook-github-bot reviewed Sep 13, 2018

View reviewed changes

NTillmann approved these changes Sep 13, 2018

View reviewed changes

hermanventer previously requested changes Sep 13, 2018

View reviewed changes

Sapan Bhatia added 4 commits September 13, 2018 21:20

Return correct recovery code for FatalError

238e345

Support check to see if the user has upgraded a given diagnostic to an

2e701dd

error

Support check to see if the user has upgraded a given diagnostic to an

ab0d4c4

error

Enable and constrain optimized array operator support for InstantRender

bceb9ef

sb98052 force-pushed the irops branch from 19a1719 to 3b9f292 Compare September 14, 2018 16:34

sb98052 force-pushed the irops branch from 3b9f292 to eb2f153 Compare September 14, 2018 16:42

facebook-github-bot reviewed Sep 17, 2018

View reviewed changes

Alternative approach to preventing materialization and making the error

010fd4f

informative and recoverable

sb98052 force-pushed the irops branch from eb2f153 to 010fd4f Compare September 17, 2018 19:24

facebook-github-bot reviewed Sep 17, 2018

View reviewed changes

hermanventer suggested changes Sep 17, 2018

View reviewed changes

facebook-github-bot closed this in 2ad7ac2 Sep 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable and constrain optimized array operator support for InstantRender #2547

Enable and constrain optimized array operator support for InstantRender #2547

sb98052 commented Sep 13, 2018 •

edited

Loading

facebook-github-bot left a comment

NTillmann left a comment

hermanventer Sep 13, 2018

hermanventer Sep 13, 2018

sb98052 Sep 14, 2018

hermanventer Sep 13, 2018

sb98052 Sep 14, 2018

NTillmann commented Sep 14, 2018 •

edited

Loading

sb98052 commented Sep 17, 2018

facebook-github-bot left a comment

facebook-github-bot left a comment

hermanventer Sep 17, 2018

hermanventer Sep 17, 2018

hermanventer Sep 17, 2018

hermanventer Sep 17, 2018

hermanventer Sep 17, 2018

hermanventer Sep 17, 2018

hermanventer Sep 17, 2018

Enable and constrain optimized array operator support for InstantRender #2547

Enable and constrain optimized array operator support for InstantRender #2547

Conversation

sb98052 commented Sep 13, 2018 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

NTillmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTillmann commented Sep 14, 2018 • edited Loading

sb98052 commented Sep 17, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sb98052 commented Sep 13, 2018 •

edited

Loading

NTillmann commented Sep 14, 2018 •

edited

Loading