From a8fce2421a7c47861b8601d17a6cc75163a8dbb2 Mon Sep 17 00:00:00 2001 From: Ara Adkins Date: Thu, 20 May 2021 08:29:22 +0100 Subject: [PATCH] Remove the EPB GIL for GraalPython (#1747) --- .gitignore | 2 +- .../epb/node/CoercePrimitiveNode.java | 71 +++++++++++++++++++ .../epb/node/ContextRewrapExceptionNode.java | 9 +++ .../epb/node/ContextRewrapNode.java | 8 +++ .../interpreter/epb/node/ForeignEvalNode.java | 4 +- .../interpreter/epb/node/PyForeignNode.java | 4 +- .../controlflow/caseexpr/ArrayBranchNode.java | 11 +++ .../caseexpr/BooleanBranchNode.java | 2 + .../BooleanConstructorBranchNode.java | 2 + test/Tests/src/Semantic/Js_Interop_Spec.enso | 41 ++++++++++- .../src/Semantic/Python_Interop_Spec.enso | 45 +++++++++++- test/Tests/src/Semantic/R_Interop_Spec.enso | 40 +++++++++++ 12 files changed, 232 insertions(+), 7 deletions(-) create mode 100644 engine/runtime/src/main/java/org/enso/interpreter/epb/node/CoercePrimitiveNode.java diff --git a/.gitignore b/.gitignore index abc9077d310..3ffb12852e0 100644 --- a/.gitignore +++ b/.gitignore @@ -77,7 +77,7 @@ distribution/std-lib/docs-js ## Benchmark Reports ## ####################### -bench-report.xml +bench-report*.xml ############## ## Binaries ## diff --git a/engine/runtime/src/main/java/org/enso/interpreter/epb/node/CoercePrimitiveNode.java b/engine/runtime/src/main/java/org/enso/interpreter/epb/node/CoercePrimitiveNode.java new file mode 100644 index 00000000000..2025308adc2 --- /dev/null +++ b/engine/runtime/src/main/java/org/enso/interpreter/epb/node/CoercePrimitiveNode.java @@ -0,0 +1,71 @@ +package org.enso.interpreter.epb.node; + +import com.oracle.truffle.api.dsl.Fallback; +import com.oracle.truffle.api.dsl.GenerateUncached; +import com.oracle.truffle.api.dsl.ReportPolymorphism; +import com.oracle.truffle.api.dsl.Specialization; +import com.oracle.truffle.api.interop.InteropLibrary; +import com.oracle.truffle.api.interop.UnsupportedMessageException; +import com.oracle.truffle.api.library.CachedLibrary; +import com.oracle.truffle.api.nodes.Node; + +@GenerateUncached +@ReportPolymorphism +public abstract class CoercePrimitiveNode extends Node { + + /** + * Create a new node responsible for coercing primitive values to Enso primitives at the polyglot + * boundary. + * + * @return a new primitive coercion node + */ + public static CoercePrimitiveNode build() { + return CoercePrimitiveNodeGen.create(); + } + + /** + * Converts an object from a polyglot representation into an equivalent representation as an Enso + * primitive. + * + * @param value the polyglot value to perform coercion on + * @return {@code value} coerced to an Enso primitive where applicable + */ + public abstract Object execute(Object value); + + @Specialization(guards = "bools.isBoolean(bool)") + boolean doBoolean(Object bool, @CachedLibrary(limit = "5") InteropLibrary bools) { + try { + return bools.asBoolean(bool); + } catch (UnsupportedMessageException e) { + throw new IllegalStateException("Impossible, `bool` already checked to be a boolean"); + } + } + + @Specialization(guards = {"numbers.isNumber(integer)", "numbers.fitsInLong(integer)"}) + long doInteger(Object integer, @CachedLibrary(limit = "5") InteropLibrary numbers) { + try { + return numbers.asLong(integer); + } catch (UnsupportedMessageException e) { + throw new IllegalStateException("Impossible, `integer` is checked to be a long"); + } + } + + @Specialization( + guards = { + "numbers.isNumber(decimal)", + "!numbers.fitsInLong(decimal)", + "numbers.fitsInDouble(decimal)" + }) + double doDecimal(Object decimal, @CachedLibrary(limit = "5") InteropLibrary numbers) { + try { + return numbers.asDouble(decimal); + } catch (UnsupportedMessageException e) { + throw new IllegalStateException("Impossible, `decimal` is checked to be a long"); + } + } + + @Fallback + Object doNonPrimitive(Object value) { + return value; + } +} diff --git a/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ContextRewrapExceptionNode.java b/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ContextRewrapExceptionNode.java index f821ff25abc..ccadd7dc28a 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ContextRewrapExceptionNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ContextRewrapExceptionNode.java @@ -13,6 +13,15 @@ import org.enso.interpreter.epb.runtime.PolyglotProxy; @GenerateUncached @ReportPolymorphism public abstract class ContextRewrapExceptionNode extends Node { + + /** Create a new context rewrap exception node. + * + * @return a new context rewrap exception node + */ + public static ContextRewrapExceptionNode build() { + return ContextRewrapExceptionNodeGen.create(); + } + /** * Wraps a value originating from {@code origin} into a value valid in {@code target}. This method * is allowed to use interop library on {@code value} and therefore must be called with {@code diff --git a/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ContextRewrapNode.java b/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ContextRewrapNode.java index 39a57a8d3d8..43b5e6c8fc0 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ContextRewrapNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ContextRewrapNode.java @@ -15,6 +15,14 @@ import org.enso.interpreter.epb.runtime.PolyglotProxy; @ReportPolymorphism public abstract class ContextRewrapNode extends Node { + /** Create a new context rewrap node. + * + * @return a new context rewrap node. + */ + static ContextRewrapNode build() { + return ContextRewrapNodeGen.create(); + } + /** * Wraps a value originating from {@code origin} into a value valid in {@code target}. This method * is allowed to use interop library on {@code value} and therefore must be called with {@code diff --git a/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ForeignEvalNode.java b/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ForeignEvalNode.java index 03a5d35d26f..a951384c11e 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ForeignEvalNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/epb/node/ForeignEvalNode.java @@ -22,9 +22,9 @@ import org.enso.interpreter.epb.runtime.GuardedTruffleContext; public abstract class ForeignEvalNode extends RootNode { private final EpbParser.Result code; private @Child ForeignFunctionCallNode foreign; - private @Child ContextRewrapNode rewrapNode = ContextRewrapNodeGen.create(); + private @Child ContextRewrapNode rewrapNode = ContextRewrapNode.build(); private @Child ContextRewrapExceptionNode rewrapExceptionNode = - ContextRewrapExceptionNodeGen.create(); + ContextRewrapExceptionNode.build(); private @CompilerDirectives.CompilationFinal AbstractTruffleException parseError; private final String[] argNames; diff --git a/engine/runtime/src/main/java/org/enso/interpreter/epb/node/PyForeignNode.java b/engine/runtime/src/main/java/org/enso/interpreter/epb/node/PyForeignNode.java index eb7439712e8..24cb3ea38b9 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/epb/node/PyForeignNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/epb/node/PyForeignNode.java @@ -11,13 +11,15 @@ import com.oracle.truffle.api.library.CachedLibrary; @NodeField(name = "foreignFunction", type = Object.class) public abstract class PyForeignNode extends ForeignFunctionCallNode { + private @Child CoercePrimitiveNode coercePrimitiveNode = CoercePrimitiveNode.build(); + abstract Object getForeignFunction(); @Specialization public Object doExecute( Object[] arguments, @CachedLibrary("foreignFunction") InteropLibrary interopLibrary) { try { - return interopLibrary.execute(getForeignFunction(), arguments); + return coercePrimitiveNode.execute(interopLibrary.execute(getForeignFunction(), arguments)); } catch (UnsupportedMessageException | UnsupportedTypeException | ArityException e) { throw new IllegalStateException("Python parser returned a malformed object", e); } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/ArrayBranchNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/ArrayBranchNode.java index 0e28a440b02..a39edcf3cf7 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/ArrayBranchNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/ArrayBranchNode.java @@ -4,6 +4,8 @@ import com.oracle.truffle.api.RootCallTarget; import com.oracle.truffle.api.dsl.Fallback; import com.oracle.truffle.api.dsl.Specialization; import com.oracle.truffle.api.frame.VirtualFrame; +import com.oracle.truffle.api.interop.InteropLibrary; +import com.oracle.truffle.api.library.CachedLibrary; import com.oracle.truffle.api.nodes.NodeInfo; import com.oracle.truffle.api.profiles.ConditionProfile; import org.enso.interpreter.runtime.callable.atom.Atom; @@ -43,6 +45,15 @@ public abstract class ArrayBranchNode extends BranchNode { accept(frame, state, new Object[0]); } + @Specialization(guards = "arrays.hasArrayElements(target)") + void doPolyglotArray( + VirtualFrame frame, + Object state, + Object target, + @CachedLibrary(limit = "5") InteropLibrary arrays) { + accept(frame, state, new Object[0]); + } + @Fallback void doFallback(VirtualFrame frame, Object state, Object target) {} } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/BooleanBranchNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/BooleanBranchNode.java index 957f556b365..fccff2f0f44 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/BooleanBranchNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/BooleanBranchNode.java @@ -4,6 +4,8 @@ import com.oracle.truffle.api.RootCallTarget; import com.oracle.truffle.api.dsl.Fallback; import com.oracle.truffle.api.dsl.Specialization; import com.oracle.truffle.api.frame.VirtualFrame; +import com.oracle.truffle.api.interop.InteropLibrary; +import com.oracle.truffle.api.library.CachedLibrary; import com.oracle.truffle.api.nodes.NodeInfo; import com.oracle.truffle.api.profiles.ConditionProfile; diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/BooleanConstructorBranchNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/BooleanConstructorBranchNode.java index 74e618ad463..b849d32f670 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/BooleanConstructorBranchNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/controlflow/caseexpr/BooleanConstructorBranchNode.java @@ -4,6 +4,8 @@ import com.oracle.truffle.api.RootCallTarget; import com.oracle.truffle.api.dsl.Fallback; import com.oracle.truffle.api.dsl.Specialization; import com.oracle.truffle.api.frame.VirtualFrame; +import com.oracle.truffle.api.interop.InteropLibrary; +import com.oracle.truffle.api.library.CachedLibrary; import com.oracle.truffle.api.nodes.NodeInfo; import com.oracle.truffle.api.profiles.ConditionProfile; import org.enso.interpreter.runtime.builtin.Bool; diff --git a/test/Tests/src/Semantic/Js_Interop_Spec.enso b/test/Tests/src/Semantic/Js_Interop_Spec.enso index 31010050238..c3d414391fa 100644 --- a/test/Tests/src/Semantic/Js_Interop_Spec.enso +++ b/test/Tests/src/Semantic/Js_Interop_Spec.enso @@ -48,6 +48,12 @@ foreign js make_str str = """ foreign js make_int = """ return 10 +foreign js make_true = """ + return true + +foreign js make_false = """ + return false + foreign js make_double = """ return 10.5 @@ -93,6 +99,33 @@ spec = Test.group "Polyglot JS" <| _ -> False t.should_be_true + Test.specify "should make JS booleans type pattern-matchable" <| + bool = here.make_true + t = case bool of + True -> True + _ -> False + t.should_be_true + bool_2 = here.make_false + f = case bool_2 of + False -> True + _ -> False + f.should_be_true + c = case bool of + Boolean -> True + _ -> False + c.should_be_true + c_2 = case bool_2 of + Boolean -> True + _ -> False + c_2.should_be_true + + Test.specify "should make JS arrays type pattern-matchable as arrays" <| + arr = here.make_array + r = case arr of + Array -> True + _ -> False + r.should_be_true + Test.specify "should make JS numbers type pattern-matchable" <| int_match = case here.make_int of Integer -> True @@ -133,7 +166,13 @@ spec = Test.group "Polyglot JS" <| array.index . should_equal 0 length_pending_msg = ".length cannot currently be called due to a bug in GraalJS. Fixed in 21.1.0." - Test.specify "allow access to the length property of a JS array in Enso" pending=length_pending_msg <| + Test.specify "allow access to the length property of a JS array in Enso" <| array = here.make_array array.length . should_equal 3 + Test.specify "should perform maths with mixed numbers" <| + js_num = here.make_int + enso_num = 10 + (enso_num + js_num) . should_equal 20 + (js_num - enso_num) . should_equal 0 + diff --git a/test/Tests/src/Semantic/Python_Interop_Spec.enso b/test/Tests/src/Semantic/Python_Interop_Spec.enso index 2aa9dcf15d8..873d996cf86 100644 --- a/test/Tests/src/Semantic/Python_Interop_Spec.enso +++ b/test/Tests/src/Semantic/Python_Interop_Spec.enso @@ -55,6 +55,15 @@ foreign python make_int = """ foreign python make_double = """ return 10.5 +foreign python make_true = """ + return True + +foreign python make_false = """ + return False + +foreign python make_null = """ + return None + foreign python does_not_parse = """ if? cxcc 531 6 @@ -90,8 +99,34 @@ spec = _ -> False t.should_be_true - pending_pat = "Python numbers are currently not being converted." - Test.specify "should make Python numbers type pattern-matchable" pending=pending_pat <| + Test.specify "should make Python booleans type pattern-matchable" <| + bool = here.make_true + t = case bool of + True -> True + _ -> False + t.should_be_true + bool_2 = here.make_false + f = case bool_2 of + False -> True + _ -> False + f.should_be_true + c = case bool of + Boolean -> True + _ -> False + c.should_be_true + c_2 = case bool_2 of + Boolean -> True + _ -> False + c_2.should_be_true + + Test.specify "should make Python lists type pattern-matchable as arrays" <| + arr = here.make_array + r = case arr of + Array -> True + _ -> False + r.should_be_true + + Test.specify "should make Python numbers type pattern-matchable" <| int_match = case here.make_int of Integer -> True int_match.should_be_true @@ -121,3 +156,9 @@ spec = err = Panic.recover here.does_not_parse . catch err.cause.args.at 0 . should .contain 'invalid syntax' + Test.specify "should perform maths with mixed numbers" <| + py_num = here.make_int + enso_num = 10 + (enso_num + py_num) . should_equal 20 + (py_num - enso_num) . should_equal 0 + diff --git a/test/Tests/src/Semantic/R_Interop_Spec.enso b/test/Tests/src/Semantic/R_Interop_Spec.enso index 7c730d4c877..555fdddc3e8 100644 --- a/test/Tests/src/Semantic/R_Interop_Spec.enso +++ b/test/Tests/src/Semantic/R_Interop_Spec.enso @@ -44,6 +44,12 @@ foreign r make_int = """ foreign r make_double = """ 10.5 +foreign r make_true = """ + TRUE + +foreign r make_false = """ + FALSE + spec = pending = if Polyglot.is_language_installed "R" then Nothing else """ Can't run R tests, R is not installed. @@ -76,6 +82,33 @@ spec = _ -> False t.should_be_true + Test.specify "should make R booleans type pattern-matchable" <| + bool = here.make_true + t = case bool of + True -> True + _ -> False + t.should_be_true + bool_2 = here.make_false + f = case bool_2 of + False -> True + _ -> False + f.should_be_true + c = case bool of + Boolean -> True + _ -> False + c.should_be_true + c_2 = case bool_2 of + Boolean -> True + _ -> False + c_2.should_be_true + + Test.specify "should make R arrays type pattern-matchable as arrays" <| + arr = here.make_array + r = case arr of + Array -> True + _ -> False + r.should_be_true + Test.specify "should make R numbers type pattern-matchable" <| int_match = case here.make_int of Integer -> True @@ -105,3 +138,10 @@ spec = Test.specify "should properly report parse errors" <| err = Panic.recover here.does_not_parse err.catch.to_display_text.should .contain 'parse exception' + + Test.specify "should perform maths with mixed numbers" <| + r_num = here.make_int + enso_num = 10 + (enso_num + r_num) . should_equal 20 + (r_num - enso_num) . should_equal 0 +