Polyglot JS Definitions #1451

kustosz · 2021-02-04T17:02:42Z

Pull Request Description

Introduces a prototype for foreign JS definitions.
These are thread safe.
Performance is not great, currently a JS call seems ~500X slower than an in-enso call – probably because these cannot be easily inlined and require more memory-digging for synchronization. Further investigation will be required in the future.

Important Notes

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the Scala, Java, and Rust style guides.
All documentation and configuration conforms to the markdown and YAML style guides.
All code has been tested where possible.

This reverts commit 4e1caf6.

iamrecursion

Awesome stuff. A few important things are missing, though!

iamrecursion · 2021-02-05T09:02:03Z

engine/runner/src/main/scala/org/enso/runner/ContextFactory.scala

@@ -34,7 +34,8 @@ class ContextFactory {
    strictErrors: Boolean = false
  ): PolyglotContext = {
    val context = Context
-      .newBuilder(LanguageInfo.ID)
+      // TODO: Remove EPB from this list when https://github.com/oracle/graal/pull/3139 is merged


Suggested change

// TODO: Remove EPB from this list when https://github.com/oracle/graal/pull/3139 is merged

// TODO: Remove EPB from this list when https://github.com/oracle/graal/pull/3139 is merged

// and available in our Graal release.

iamrecursion · 2021-02-05T09:21:09Z

engine/runtime/src/main/java/org/enso/interpreter/epb/EpbLanguage.java

+ * {@link com.oracle.truffle.api.TruffleContext}s, one (often referred to as "outer") is allowed to
+ * run Enso, Host Java, and possibly other thread-ready languages. Languages that cannot safely run
+ * in a multithreaded environment are relegated to the other context (referred to as "inner"). The
+ * inner context provides a GIL capacity, ensuring that access to the single-threaded languages is


Suggested change

* inner context provides a GIL capacity, ensuring that access to the single-threaded languages is

* inner context provides a GIL capability, ensuring that access to the single-threaded languages is

iamrecursion · 2021-02-05T09:21:34Z

engine/runtime/src/main/java/org/enso/interpreter/epb/EpbLanguage.java

+ * outer context values need to be specially wrapped before being passed (e.g. as arguments) to the
+ * inner context, and inner context values need rewrapping for use in the outer context. See {@link
+ * org.enso.interpreter.epb.runtime.PolyglotProxy} and {@link
+ * org.enso.interpreter.epb.node.ContextFlipNode} for details of how and when this wrapping is being


Suggested change

* org.enso.interpreter.epb.node.ContextFlipNode} for details of how and when this wrapping is being

* org.enso.interpreter.epb.node.ContextFlipNode} for details of how and when this wrapping is

iamrecursion · 2021-02-05T09:21:49Z

engine/runtime/src/main/java/org/enso/interpreter/epb/EpbLanguage.java

+ * done.
+ *
+ * <p>With the structure outlined above, EPB is the only language that is initialized in both inner
+ * and outer contexts and thus it is very minimal. It's only role is to manage both contexts and


Suggested change

* and outer contexts and thus it is very minimal. It's only role is to manage both contexts and

* and outer contexts and thus it is very minimal. Its only role is to manage both contexts and

iamrecursion · 2021-02-05T09:24:33Z

engine/runtime/src/main/java/org/enso/interpreter/epb/EpbParser.java

+   */
+  public static Result parse(Source source) {
+    String src = source.getCharacters().toString();
+    String[] langAndCode = src.split("#", 2);


Shouldn't you use separator here?

iamrecursion · 2021-02-05T09:25:53Z

engine/runtime/src/main/java/org/enso/interpreter/epb/EpbParser.java

+    if (tag.equals("js")) {
+      return ForeignLanguage.JS;
+    }
+    return null;


It seems like this should delegate to a method on the ForeignLanguage enum for me.

iamrecursion · 2021-02-05T09:27:06Z

engine/runtime/src/main/java/org/enso/interpreter/epb/node/ContextFlipNode.java

+
+@GenerateUncached
+@ReportPolymorphism
+public abstract class ContextFlipNode extends Node {


I'm not sure the name of this node really expresses what it does. ContextProxyNode?

iamrecursion · 2021-02-05T09:45:31Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Polyglot.java

@@ -26,9 +26,9 @@ private void createPolyglot(Language language, ModuleScope scope) {
    scope.registerMethod(polyglot, "execute", ExecuteMethodGen.makeFunction(language));
    scope.registerMethod(polyglot, "invoke", InvokeMethodGen.makeFunction(language));
    scope.registerMethod(polyglot, "new", InstantiateMethodGen.makeFunction(language));
-    scope.registerMethod(polyglot, "eval", EvalMethodGen.makeFunction(language));


You need to remove this from Builtins.enso as well.

iamrecursion · 2021-02-05T09:45:44Z

...main/java/org/enso/interpreter/node/expression/builtin/interop/generic/GetArraySizeNode.java

+    type = "Polyglot",
+    name = "get_array_size",


You need to add this to Builtins.enso.

iamrecursion · 2021-02-05T09:53:40Z

engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala

@@ -5808,6 +5809,10 @@ object IR {
      case object InvalidOperatorName extends Reason {
        override def explanation: String = "Invalid operator name."
      }
+
+      case object InvalidForeignDefinition extends Reason {
+        override def explanation: String = "Invalid foreign definition."


This needs to be more specific. "Invalid foreign definition foo is not a supported foreign language", or something like that.

iamrecursion · 2021-02-08T15:57:03Z

test/Tests/src/Semantic/Js_Interop_Spec.enso

+        t = case str of
+            Text -> True
+            _ -> False
+        t.should_be_true


You can write this more nicely using fail in the False case.

kustosz added 7 commits February 2, 2021 11:22

Revert "undo changes for other languages"

311fdd2

This reverts commit 4e1caf6.

JS progress and thread synchronization

d57666b

default arguments support

a62ab6a

tests, array fixes

9879b1c

refactors and docs inside EPB

49c05ad

refactors

f43342d

syntax

f841201

kustosz added Type: Enhancement p-highest Should be completed ASAP labels Feb 4, 2021

kustosz self-assigned this Feb 4, 2021

kustosz requested review from iamrecursion, radeusgd and wdanilo as code owners February 4, 2021 17:02

Merge branch 'main' into wip/mk/poly_eval

f644016

iamrecursion suggested changes Feb 5, 2021

View reviewed changes

kustosz added 2 commits February 8, 2021 16:16

handle non-text texts

7250a0a

cr feedback

b96c3b3

kustosz requested a review from iamrecursion February 8, 2021 15:41

kustosz added 2 commits February 8, 2021 16:55

more stuff

665a327

remove imports

f04c0d0

iamrecursion approved these changes Feb 8, 2021

View reviewed changes

kustosz added 2 commits February 8, 2021 17:05

fix test

4edf196

Merge branch 'main' into wip/mk/poly_eval

589b320

kustosz merged commit c4a0772 into main Feb 8, 2021

kustosz deleted the wip/mk/poly_eval branch February 8, 2021 17:14

iamrecursion pushed a commit that referenced this pull request Feb 10, 2021

Polyglot JS Definitions (#1451)

25e88cf

iamrecursion pushed a commit that referenced this pull request Feb 11, 2021

Polyglot JS Definitions (#1451)

9565358

iamrecursion pushed a commit that referenced this pull request Feb 11, 2021

Polyglot JS Definitions (#1451)

2aac491

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polyglot JS Definitions #1451

Polyglot JS Definitions #1451

kustosz commented Feb 4, 2021

iamrecursion left a comment

iamrecursion Feb 5, 2021

iamrecursion Feb 5, 2021

iamrecursion Feb 5, 2021

iamrecursion Feb 5, 2021

iamrecursion Feb 5, 2021

iamrecursion Feb 5, 2021

iamrecursion Feb 5, 2021

iamrecursion Feb 5, 2021

iamrecursion Feb 5, 2021

iamrecursion Feb 5, 2021

iamrecursion Feb 8, 2021

	// TODO: Remove EPB from this list when https://github.com/oracle/graal/pull/3139 is merged
	// TODO: Remove EPB from this list when https://github.com/oracle/graal/pull/3139 is merged
	// and available in our Graal release.

	* inner context provides a GIL capacity, ensuring that access to the single-threaded languages is
	* inner context provides a GIL capability, ensuring that access to the single-threaded languages is

	* org.enso.interpreter.epb.node.ContextFlipNode} for details of how and when this wrapping is being
	* org.enso.interpreter.epb.node.ContextFlipNode} for details of how and when this wrapping is

	* and outer contexts and thus it is very minimal. It's only role is to manage both contexts and
	* and outer contexts and thus it is very minimal. Its only role is to manage both contexts and

Polyglot JS Definitions #1451

Polyglot JS Definitions #1451

Conversation

kustosz commented Feb 4, 2021

Pull Request Description

Important Notes

Checklist

iamrecursion 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

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