From 34be5dd6ec030b226cb4601bec577dc17d16c79e Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Wed, 19 Oct 2022 11:51:21 +0200 Subject: [PATCH 1/7] Normalize index in Vector.at in builtin method The main culprit of a Vector slowdown (when compared to Array) was the normalization of the index when accessing the elements. Turns out that the Graal was very persistent on **not** inlining that particular fragment and that was degrading the results in benchmarks. Being unable to force it to do it (looks like a combination of thunk execution and another layer of indirection) we resorted to just moving the normalization to the builtin method. That makes Array and Vector perform roughly the same. --- .../Base/0.0.0-dev/src/Data/Vector.enso | 3 +- .../builtin/immutable/AtVectorNode.java | 33 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) create mode 100644 engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java diff --git a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso index e59f1186e68f..56dbe36dfb3b 100644 --- a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso +++ b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso @@ -147,8 +147,7 @@ type Vector a [1, 2, 3].at -1 == 3 at : Integer -> Any ! Index_Out_Of_Bounds_Error at self index = - actual_index = if index < 0 then self.length + index else index - Panic.catch Invalid_Array_Index_Error_Data (self.unsafe_at actual_index) _-> + Panic.catch Invalid_Array_Index_Error_Data (self.at_builtin index) _-> Error.throw (Index_Out_Of_Bounds_Error_Data index self.length) ## ADVANCED diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java new file mode 100644 index 000000000000..76be2fff3d8d --- /dev/null +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java @@ -0,0 +1,33 @@ +package org.enso.interpreter.node.expression.builtin.immutable; + +import com.oracle.truffle.api.interop.InteropLibrary; +import com.oracle.truffle.api.interop.InvalidArrayIndexException; +import com.oracle.truffle.api.interop.UnsupportedMessageException; +import com.oracle.truffle.api.nodes.Node; +import org.enso.interpreter.dsl.BuiltinMethod; +import org.enso.interpreter.node.expression.builtin.interop.syntax.HostValueToEnsoNode; +import org.enso.interpreter.runtime.Context; +import org.enso.interpreter.runtime.data.Vector; +import org.enso.interpreter.runtime.error.PanicException; + +@BuiltinMethod( + type = "Vector", + name = "at_builtin", + description = "Returns an element of Vector at the specified index.") +public class AtVectorNode extends Node { + private @Child InteropLibrary interop = InteropLibrary.getFactory().createDispatched(3); + private @Child HostValueToEnsoNode toEnso = HostValueToEnsoNode.build(); + + Object execute(Vector self, long index) { + try { + long actualIndex = index < 0 ? index + self.length(interop) : index; + return self.readArrayElement(actualIndex, interop, toEnso); + } catch (InvalidArrayIndexException e) { + Context ctx = Context.get(this); + throw new PanicException( + ctx.getBuiltins().error().makeInvalidArrayIndexError(self, index), this); + } catch (UnsupportedMessageException e) { + throw new PanicException(e.getMessage(), this); + } + } +} From 7b72011af47aa23bfc24401c7ebb177ce58020d4 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Wed, 19 Oct 2022 17:29:54 +0200 Subject: [PATCH 2/7] Unify {Vector|Array}.at, Vector.unsafe_at begone Vector.at and Array.at should both support negative indices and both return a dataflow error on invalid index. --- .../Base/0.0.0-dev/src/Data/Array.enso | 17 +++--- .../Base/0.0.0-dev/src/Data/Vector.enso | 52 +++++++------------ .../Base/0.0.0-dev/src/Error/Common.enso | 1 + .../builtin/error/IndexOutOfBoundsError.java | 14 +++++ .../builtin/immutable/AtVectorNode.java | 14 +++-- .../builtin/mutable/ArrayAtNode.java | 18 +++++-- .../interpreter/runtime/builtin/Error.java | 6 +++ test/Tests/src/Data/Array_Spec.enso | 5 +- 8 files changed, 78 insertions(+), 49 deletions(-) create mode 100644 engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/IndexOutOfBoundsError.java diff --git a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso index b75c39fde639..403352698238 100644 --- a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso +++ b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Array.enso @@ -7,20 +7,23 @@ import Standard.Base.Meta ## The type of primitive mutable arrays. @Builtin_Type type Array - ## Gets the element at index in the array this. + ## Gets an element from the array at a specified index (0-based). Arguments: - - index: The index to get the element from. - - ? Safety - If index < 0 or index >= self.length, then this operation will result - in an Invalid_Array_Index_Error exception. + - index: The index to get the element from. The index is + also allowed be negative, then the elements are indexed from the back + of the array, i.e. -1 will correspond to the last element. > Example Get the element at index 1. [1,2,3].to_array.at 1 - at : Integer -> Any + + > Example + Get the last element of an array. + + [1,2,3].to_array.at -1 + at : Integer -> Any ! Index_Out_Of_Bounds_Error at self index = @Builtin_Method "Array.at" ## Gets the length of the array this. diff --git a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso index 56dbe36dfb3b..1bbe9340a24a 100644 --- a/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso +++ b/distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso @@ -146,19 +146,7 @@ type Vector a [1, 2, 3].at -1 == 3 at : Integer -> Any ! Index_Out_Of_Bounds_Error - at self index = - Panic.catch Invalid_Array_Index_Error_Data (self.at_builtin index) _-> - Error.throw (Index_Out_Of_Bounds_Error_Data index self.length) - - ## ADVANCED - UNSTABLE - - An unsafe variant of the `at` operation. It only allows non-negative - indices and will panic with a raw Java exception on out-of-bounds access. - Thus it should only be used when the access is guaranteed to be within - bounds or with additional error handling. - unsafe_at : Integer -> Any - unsafe_at self index = @Builtin_Method "Vector.unsafe_at" + at self index = @Builtin_Method "Vector.at" ## Combines all the elements of the vector, by iteratively applying the passed function with next elements of the vector. @@ -178,7 +166,7 @@ type Vector a [0, 1, 2] . fold 0 (+) fold : Any -> (Any -> Any -> Any) -> Any fold self init function = - f = acc -> ix -> function acc (self.unsafe_at ix) + f = acc -> ix -> function acc (self.at ix) 0.up_to self.length . fold init f ## Combines all the elements of the vector, by iteratively applying the @@ -212,9 +200,9 @@ type Vector a reduce : (Any -> Any -> Any) -> Any ! Empty_Error reduce self function = case self.not_empty of - True -> if self.length == 1 then self.unsafe_at 0 else + True -> if self.length == 1 then self.at 0 else f = acc -> ix -> function acc (self.at ix) - 1.up_to self.length . fold (self.unsafe_at 0) f + 1.up_to self.length . fold (self.at 0) f False -> Error.throw Empty_Error ## Computes the sum of the values in the vector. @@ -247,7 +235,7 @@ type Vector a [1, 2, 3, 4, 5].exists (> 3) exists : (Any -> Boolean) -> Boolean exists self predicate = - 0.up_to self.length . exists (idx -> (predicate (self.unsafe_at idx))) + 0.up_to self.length . exists (idx -> (predicate (self.at idx))) ## Returns the first element of the vector that satisfies the predicate or if no elements of the vector satisfy the predicate, it throws nothing. @@ -266,7 +254,7 @@ type Vector a len = self.length go idx = if (idx >= len) then Error.throw Nothing else - elem = self.unsafe_at idx + elem = self.at idx if predicate elem then elem else @Tail_Call go idx+1 go 0 @@ -434,7 +422,7 @@ type Vector a [1, 2, 3] . map +1 map : (Any -> Any) -> Vector Any map self function = - new self.length i-> function (self.unsafe_at i) + new self.length i-> function (self.at i) ## Applies a function to each element of the vector, returning the vector that contains all results concatenated. @@ -482,7 +470,7 @@ type Vector a [1, 2, 3].map_with_index (+) map_with_index : (Integer -> Any -> Any) -> Vector Any - map_with_index self function = new self.length i-> function i (self.unsafe_at i) + map_with_index self function = new self.length i-> function i (self.at i) ## Applies a function to each element of the vector. @@ -499,7 +487,7 @@ type Vector a each : (Any -> Any) -> Nothing each self f = 0.up_to self.length . each ix-> - f (self.unsafe_at ix) + f (self.at ix) ## Applies a function to each element of the vector. @@ -519,7 +507,7 @@ type Vector a each_with_index : (Integer -> Any -> Any) -> Nothing each_with_index self f = 0.up_to self.length . each ix-> - f ix (self.unsafe_at ix) + f ix (self.at ix) ## Reverses the vector, returning a vector with the same elements, but in the opposite order. @@ -529,7 +517,7 @@ type Vector a [1, 2].reverse reverse : Vector Any - reverse self = new self.length (i -> self.unsafe_at (self.length - (1 + i))) + reverse self = new self.length (i -> self.at (self.length - (1 + i))) ## Generates a human-readable text representation of the vector. @@ -578,7 +566,7 @@ type Vector a == : Vector -> Boolean == self that = case that of _ : Vector -> - eq_at i = self.unsafe_at i == that.unsafe_at i + eq_at i = self.at i == that.at i if self.length == that.length then 0.up_to self.length . all eq_at else False _ : Array -> eq_at i = self.at i == that.at i @@ -642,8 +630,8 @@ type Vector a join : Text -> Text -> Text -> Text join self separator="" prefix="" suffix="" = if self.is_empty then prefix+suffix else - if self.length == 1 then prefix + self.unsafe_at 0 + suffix else - prefix + self.unsafe_at 0 + (1.up_to self.length . fold "" acc-> i-> acc + separator + self.unsafe_at i) + suffix + if self.length == 1 then prefix + self.at 0 + suffix else + prefix + self.at 0 + (1.up_to self.length . fold "" acc-> i-> acc + separator + self.at i) + suffix ## PRIVATE Creates a new vector with the skipping elements until `start` and then @@ -715,7 +703,7 @@ type Vector a zip : Vector Any -> (Any -> Any -> Any) -> Vector Any zip self that function=[_,_] = len = Math.min self.length that.length - new len i-> function (self.unsafe_at i) (that.unsafe_at i) + new len i-> function (self.at i) (that.at i) ## Extend `self` vector to the length of `n` appending elements `elem` to the end. @@ -758,7 +746,7 @@ type Vector a [1, 2, 3, 4].head head : Any ! Empty_Error - head self = if self.length >= 1 then self.unsafe_at 0 else Error.throw Empty_Error + head self = if self.length >= 1 then self.at 0 else Error.throw Empty_Error ## Get all elements in the vector except the first. @@ -787,7 +775,7 @@ type Vector a [1, 2, 3, 4].last last : Vector ! Empty_Error - last self = if self.length >= 1 then self.unsafe_at (self.length-1) else + last self = if self.length >= 1 then self.at (self.length-1) else Error.throw Empty_Error ## Get the first element from the vector, or an `Empty_Error` if the vector @@ -1132,7 +1120,7 @@ slice_ranges vector ranges = if ranges.length == 0 then [] else if ranges.length != 1 then slice_many_ranges vector ranges else case ranges.first of - _ : Integer -> [vector.unsafe_at ranges.first] + _ : Integer -> [vector.at ranges.first] Range_Data start end step -> case step == 1 of True -> vector.slice start end False -> slice_many_ranges vector ranges @@ -1146,11 +1134,11 @@ slice_many_ranges vector ranges = builder = new_builder new_length ranges.each descriptor-> case descriptor of _ : Integer -> - builder.append (vector.unsafe_at descriptor) + builder.append (vector.at descriptor) Range_Data start end step -> case step == 1 of True -> builder.append_vector_range vector start end False -> descriptor.each ix-> - builder.append (vector.unsafe_at ix) + builder.append (vector.at ix) builder.to_vector diff --git a/distribution/lib/Standard/Base/0.0.0-dev/src/Error/Common.enso b/distribution/lib/Standard/Base/0.0.0-dev/src/Error/Common.enso index e34d61d0ee23..b1cbdb726693 100644 --- a/distribution/lib/Standard/Base/0.0.0-dev/src/Error/Common.enso +++ b/distribution/lib/Standard/Base/0.0.0-dev/src/Error/Common.enso @@ -218,6 +218,7 @@ from project.Error.Common.Index_Out_Of_Bounds_Error export all Arguments: - index: The requested index. - length: The length of the collection. +@Builtin_Type type Index_Out_Of_Bounds_Error Index_Out_Of_Bounds_Error_Data index length diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/IndexOutOfBoundsError.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/IndexOutOfBoundsError.java new file mode 100644 index 000000000000..58b7d13051a3 --- /dev/null +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/IndexOutOfBoundsError.java @@ -0,0 +1,14 @@ +package org.enso.interpreter.node.expression.builtin.error; + +import org.enso.interpreter.dsl.BuiltinType; +import org.enso.interpreter.node.expression.builtin.UniquelyConstructibleBuiltin; + +import java.util.List; + +@BuiltinType +public class IndexOutOfBoundsError extends UniquelyConstructibleBuiltin { + @Override + protected List getConstructorParamNames() { + return List.of("index", "length"); + } +} diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java index 76be2fff3d8d..efbaee8bc4fc 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java @@ -1,5 +1,6 @@ package org.enso.interpreter.node.expression.builtin.immutable; +import com.oracle.truffle.api.CompilerDirectives; import com.oracle.truffle.api.interop.InteropLibrary; import com.oracle.truffle.api.interop.InvalidArrayIndexException; import com.oracle.truffle.api.interop.UnsupportedMessageException; @@ -8,11 +9,12 @@ import org.enso.interpreter.node.expression.builtin.interop.syntax.HostValueToEnsoNode; import org.enso.interpreter.runtime.Context; import org.enso.interpreter.runtime.data.Vector; +import org.enso.interpreter.runtime.error.DataflowError; import org.enso.interpreter.runtime.error.PanicException; @BuiltinMethod( type = "Vector", - name = "at_builtin", + name = "at", description = "Returns an element of Vector at the specified index.") public class AtVectorNode extends Node { private @Child InteropLibrary interop = InteropLibrary.getFactory().createDispatched(3); @@ -24,9 +26,15 @@ Object execute(Vector self, long index) { return self.readArrayElement(actualIndex, interop, toEnso); } catch (InvalidArrayIndexException e) { Context ctx = Context.get(this); - throw new PanicException( - ctx.getBuiltins().error().makeInvalidArrayIndexError(self, index), this); + try { + return DataflowError.withoutTrace( + ctx.getBuiltins().error().makeIndexOutOfBoundsError(index, self.length(interop)), this); + } catch (UnsupportedMessageException ex) { + CompilerDirectives.transferToInterpreter(); + throw new PanicException(e.getMessage(), this); + } } catch (UnsupportedMessageException e) { + CompilerDirectives.transferToInterpreter(); throw new PanicException(e.getMessage(), this); } } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java index 3fd469974464..6d0486c98279 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java @@ -9,6 +9,7 @@ import org.enso.interpreter.node.expression.builtin.interop.syntax.HostValueToEnsoNode; import org.enso.interpreter.runtime.Context; import org.enso.interpreter.runtime.builtin.Builtins; +import org.enso.interpreter.runtime.error.DataflowError; import org.enso.interpreter.runtime.error.PanicException; @BuiltinMethod(type = "Array", name = "at", description = "Get element of a polyglot array") @@ -16,17 +17,24 @@ public class ArrayAtNode extends Node { @Child InteropLibrary iop = InteropLibrary.getFactory().createDispatched(3); @Child HostValueToEnsoNode convert = HostValueToEnsoNode.build(); - Object execute(Object self, long at) { + Object execute(Object self, long index) { try { - var r = iop.readArrayElement(self, at); + long actualIndex = index < 0 ? index + iop.getArraySize(self) : index; + var r = iop.readArrayElement(self, actualIndex); return convert.execute(r); } catch (UnsupportedMessageException ex) { CompilerDirectives.transferToInterpreter(); throw new IllegalStateException(ex); - } catch (InvalidArrayIndexException ex) { + } catch (InvalidArrayIndexException e) { Context ctx = Context.get(this); - Builtins builtins = ctx.getBuiltins(); - throw new PanicException(builtins.error().makeInvalidArrayIndexError(self, at), this); + try { + return DataflowError.withoutTrace( + ctx.getBuiltins().error().makeIndexOutOfBoundsError(index, iop.getArraySize(self)), + this); + } catch (UnsupportedMessageException ex) { + CompilerDirectives.transferToInterpreter(); + throw new PanicException(ex.getMessage(), this); + } } } } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Error.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Error.java index 635973102002..bca2c12addee 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Error.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Error.java @@ -20,6 +20,7 @@ public class Error { private final SyntaxError syntaxError; private final TypeError typeError; private final CompileError compileError; + private final IndexOutOfBoundsError indexOutOfBoundsError; private final InexhaustivePatternMatchError inexhaustivePatternMatchError; private final UninitializedState uninitializedState; private final NoSuchMethodError noSuchMethodError; @@ -50,6 +51,7 @@ public Error(Builtins builtins, Context context) { syntaxError = builtins.getBuiltinType(SyntaxError.class); typeError = builtins.getBuiltinType(TypeError.class); compileError = builtins.getBuiltinType(CompileError.class); + indexOutOfBoundsError = builtins.getBuiltinType(IndexOutOfBoundsError.class); inexhaustivePatternMatchError = builtins.getBuiltinType(InexhaustivePatternMatchError.class); uninitializedState = builtins.getBuiltinType(UninitializedState.class); noSuchMethodError = builtins.getBuiltinType(NoSuchMethodError.class); @@ -76,6 +78,10 @@ public Atom makeCompileError(Object message) { return compileError.newInstance(message); } + public Atom makeIndexOutOfBoundsError(long index, long length) { + return indexOutOfBoundsError.newInstance(index, length); + } + public Atom makeInexhaustivePatternMatchError(Object message) { return inexhaustivePatternMatchError.newInstance(message); } diff --git a/test/Tests/src/Data/Array_Spec.enso b/test/Tests/src/Data/Array_Spec.enso index 0cba5617c6cb..49b94447a743 100644 --- a/test/Tests/src/Data/Array_Spec.enso +++ b/test/Tests/src/Data/Array_Spec.enso @@ -26,11 +26,12 @@ test_arrays array_from_vector = arr = array_from_vector [1, 2, 3] arr.at 0 . should_equal 1 arr.at 2 . should_equal 3 + arr.at -1 . should_equal 3 Test.specify "should panic on out of bounds access" <| arr = array_from_vector [1, 2, 3] - Test.expect_panic_with (arr.at -1) Invalid_Array_Index_Error_Data - Test.expect_panic_with (arr.at 3) Invalid_Array_Index_Error_Data + (arr.at -4) . should_fail_with Index_Out_Of_Bounds_Error_Data + (arr.at 3) . should_fail_with Index_Out_Of_Bounds_Error_Data spec = Test.group "Enso Arrays" <| From f47683c6eca98857ecc9e7435689d45e091a323b Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Wed, 19 Oct 2022 17:36:48 +0200 Subject: [PATCH 3/7] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d91baeb56e2f..ecda2e305529 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -395,6 +395,7 @@ - [By-type pattern matching][3742] - [Fix performance of method calls on polyglot arrays][3781] - [Missing foreign language generates proper Enso error][3798] +- [Made Vector performance to be on par with Array][3811] [3227]: https://github.com/enso-org/enso/pull/3227 [3248]: https://github.com/enso-org/enso/pull/3248 @@ -449,6 +450,7 @@ [3742]: https://github.com/enso-org/enso/pull/3742 [3781]: https://github.com/enso-org/enso/pull/3781 [3798]: https://github.com/enso-org/enso/pull/3798 +[3811]: https://github.com/enso-org/enso/pull/3811 # Enso 2.0.0-alpha.18 (2021-10-12) From 8bc12fe368676baa316867063ebace969a1acc47 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Wed, 19 Oct 2022 17:44:44 +0200 Subject: [PATCH 4/7] nit --- .../node/expression/builtin/immutable/AtVectorNode.java | 4 ++-- .../node/expression/builtin/mutable/ArrayAtNode.java | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java index efbaee8bc4fc..7b451c810c76 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java @@ -31,11 +31,11 @@ Object execute(Vector self, long index) { ctx.getBuiltins().error().makeIndexOutOfBoundsError(index, self.length(interop)), this); } catch (UnsupportedMessageException ex) { CompilerDirectives.transferToInterpreter(); - throw new PanicException(e.getMessage(), this); + throw new IllegalStateException(ex); } } catch (UnsupportedMessageException e) { CompilerDirectives.transferToInterpreter(); - throw new PanicException(e.getMessage(), this); + throw new IllegalStateException(e); } } } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java index 6d0486c98279..08e0b7c8f775 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java @@ -22,9 +22,6 @@ Object execute(Object self, long index) { long actualIndex = index < 0 ? index + iop.getArraySize(self) : index; var r = iop.readArrayElement(self, actualIndex); return convert.execute(r); - } catch (UnsupportedMessageException ex) { - CompilerDirectives.transferToInterpreter(); - throw new IllegalStateException(ex); } catch (InvalidArrayIndexException e) { Context ctx = Context.get(this); try { @@ -33,8 +30,11 @@ Object execute(Object self, long index) { this); } catch (UnsupportedMessageException ex) { CompilerDirectives.transferToInterpreter(); - throw new PanicException(ex.getMessage(), this); + throw new IllegalStateException(ex); } + } catch (UnsupportedMessageException ex) { + CompilerDirectives.transferToInterpreter(); + throw new IllegalStateException(ex); } } } From 535f12b9cb1cc999953a5b2e1b1be0ecc59f3935 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Wed, 19 Oct 2022 17:57:08 +0200 Subject: [PATCH 5/7] Remove obsolete node --- .../builtin/immutable/UnsafeAtVectorNode.java | 32 ------------------- 1 file changed, 32 deletions(-) delete mode 100644 engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/UnsafeAtVectorNode.java diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/UnsafeAtVectorNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/UnsafeAtVectorNode.java deleted file mode 100644 index 6e9cbd659f9b..000000000000 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/UnsafeAtVectorNode.java +++ /dev/null @@ -1,32 +0,0 @@ -package org.enso.interpreter.node.expression.builtin.immutable; - -import com.oracle.truffle.api.interop.InteropLibrary; -import com.oracle.truffle.api.interop.InvalidArrayIndexException; -import com.oracle.truffle.api.interop.UnsupportedMessageException; -import com.oracle.truffle.api.nodes.Node; -import org.enso.interpreter.dsl.BuiltinMethod; -import org.enso.interpreter.node.expression.builtin.interop.syntax.HostValueToEnsoNode; -import org.enso.interpreter.runtime.Context; -import org.enso.interpreter.runtime.data.Vector; -import org.enso.interpreter.runtime.error.PanicException; - -@BuiltinMethod( - type = "Vector", - name = "unsafe_at", - description = "Returns an element of Vector at the specified index.") -public class UnsafeAtVectorNode extends Node { - private @Child InteropLibrary interop = InteropLibrary.getFactory().createDispatched(3); - private @Child HostValueToEnsoNode toEnso = HostValueToEnsoNode.build(); - - Object execute(Vector self, long index) { - try { - return self.readArrayElement(index, interop, toEnso); - } catch (InvalidArrayIndexException e) { - Context ctx = Context.get(this); - throw new PanicException( - ctx.getBuiltins().error().makeInvalidArrayIndexError(self, index), this); - } catch (UnsupportedMessageException e) { - throw new PanicException(e.getMessage(), this); - } - } -} From 168f5c3e7b9cfba254bed1fc02e840331a1dcbe7 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Wed, 19 Oct 2022 18:30:33 +0200 Subject: [PATCH 6/7] Provide hints about performance debugging --- docs/runtime-guide.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/docs/runtime-guide.md b/docs/runtime-guide.md index b636ed74531e..23c34e1167fd 100644 --- a/docs/runtime-guide.md +++ b/docs/runtime-guide.md @@ -94,6 +94,29 @@ order: 7 `truffle-api`, you've done something wrong. Similarly, if you ever import anything from `org.graalvm.polyglot` in the language code, you're doing something wrong. +10. Avoid + [deoptimizations](https://www.graalvm.org/22.2/graalvm-as-a-platform/language-implementation-framework/Optimizing/#debugging-deoptimizations). + Understanding IGV graphs can be a very time-consuming and complex process. + Sometimes it is sufficient to only look at the compilation traces to + discover repeated or unnecessary deoptimizations which can significantly + affect overall performance of your program. You can tell runner to generate + compilation traces via additional options: + ``` + JAVA_OPTS="-Dpolygot.engine.TracePerformanceWarnings=all -Dpolyglot.engine.TraceTransferToInterpreter=true -Dpolyglot.engine.TraceDeoptimizeFrame=true -Dpolyglot.engine.TraceCompilation=true -Dpolyglot.engine.TraceCompilationDetails=true" + ``` + Make sure you print trace logs by using `--log-level TRACE`. +11. Occasionally a piece of code runs slower than we anticipated. Analyzing + Truffle inlining traces may reveal locations that one thought would be + inlined but Truffle decided otherwise. Rewriting such locations to builtin + methods or more inliner-friendly representation can significantly improve + the performance. You can tell runner to generate inlining traces via + additional options: + ``` + JAVA_OPTS="-Dpolyglot.engine.TraceInlining=true -Dpolyglot.engine.TraceInliningDetails=true" + ``` + Make sure you print trace logs by using `--log-level TRACE`. See + [documentation](https://www.graalvm.org/22.2/graalvm-as-a-platform/language-implementation-framework/Inlining/#call-tree-states) + for the explanation of inlining decisions. ## Code & Internal Documentation Map From baee63cddfeae06d30918aa3311f2b31ba80bc53 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Thu, 20 Oct 2022 10:27:35 +0200 Subject: [PATCH 7/7] PR review --- .../builtin/immutable/AtVectorNode.java | 26 ++++++++--------- .../builtin/mutable/ArrayAtNode.java | 28 +++++++++---------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java index 7b451c810c76..7ccdb5e7720b 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java @@ -10,7 +10,6 @@ import org.enso.interpreter.runtime.Context; import org.enso.interpreter.runtime.data.Vector; import org.enso.interpreter.runtime.error.DataflowError; -import org.enso.interpreter.runtime.error.PanicException; @BuiltinMethod( type = "Vector", @@ -18,24 +17,25 @@ description = "Returns an element of Vector at the specified index.") public class AtVectorNode extends Node { private @Child InteropLibrary interop = InteropLibrary.getFactory().createDispatched(3); - private @Child HostValueToEnsoNode toEnso = HostValueToEnsoNode.build(); + private @Child HostValueToEnsoNode convert = HostValueToEnsoNode.build(); Object execute(Vector self, long index) { try { - long actualIndex = index < 0 ? index + self.length(interop) : index; - return self.readArrayElement(actualIndex, interop, toEnso); - } catch (InvalidArrayIndexException e) { - Context ctx = Context.get(this); - try { - return DataflowError.withoutTrace( - ctx.getBuiltins().error().makeIndexOutOfBoundsError(index, self.length(interop)), this); - } catch (UnsupportedMessageException ex) { - CompilerDirectives.transferToInterpreter(); - throw new IllegalStateException(ex); - } + return readElement(self, index); } catch (UnsupportedMessageException e) { CompilerDirectives.transferToInterpreter(); throw new IllegalStateException(e); } } + + private Object readElement(Vector self, long index) throws UnsupportedMessageException { + try { + long actualIndex = index < 0 ? index + self.length(interop) : index; + return self.readArrayElement(actualIndex, interop, convert); + } catch (InvalidArrayIndexException e) { + Context ctx = Context.get(this); + return DataflowError.withoutTrace( + ctx.getBuiltins().error().makeIndexOutOfBoundsError(index, self.length(interop)), this); + } + } } diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java index 08e0b7c8f775..cb52be9d1354 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/mutable/ArrayAtNode.java @@ -8,9 +8,7 @@ import org.enso.interpreter.dsl.BuiltinMethod; import org.enso.interpreter.node.expression.builtin.interop.syntax.HostValueToEnsoNode; import org.enso.interpreter.runtime.Context; -import org.enso.interpreter.runtime.builtin.Builtins; import org.enso.interpreter.runtime.error.DataflowError; -import org.enso.interpreter.runtime.error.PanicException; @BuiltinMethod(type = "Array", name = "at", description = "Get element of a polyglot array") public class ArrayAtNode extends Node { @@ -19,22 +17,22 @@ public class ArrayAtNode extends Node { Object execute(Object self, long index) { try { - long actualIndex = index < 0 ? index + iop.getArraySize(self) : index; - var r = iop.readArrayElement(self, actualIndex); - return convert.execute(r); - } catch (InvalidArrayIndexException e) { - Context ctx = Context.get(this); - try { - return DataflowError.withoutTrace( - ctx.getBuiltins().error().makeIndexOutOfBoundsError(index, iop.getArraySize(self)), - this); - } catch (UnsupportedMessageException ex) { - CompilerDirectives.transferToInterpreter(); - throw new IllegalStateException(ex); - } + return readElement(self, index); } catch (UnsupportedMessageException ex) { CompilerDirectives.transferToInterpreter(); throw new IllegalStateException(ex); } } + + private Object readElement(Object self, long index) throws UnsupportedMessageException { + try { + long actualIndex = index < 0 ? index + iop.getArraySize(self) : index; + var element = iop.readArrayElement(self, actualIndex); + return convert.execute(element); + } catch (InvalidArrayIndexException e) { + Context ctx = Context.get(this); + return DataflowError.withoutTrace( + ctx.getBuiltins().error().makeIndexOutOfBoundsError(index, iop.getArraySize(self)), this); + } + } }