From 6c440beecc90b2b18cfe7211dba81b9efd0d01d3 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Thu, 20 Oct 2022 14:50:44 +0200 Subject: [PATCH] Move logic calculating the index in Vector.at to a builtin method to make the performance of Vector to be on par with Array (#3811) 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. Moved all handling of invalid index into the builtin as well, simplifying the Enso implementation. This also meant that `Vector.unsafe_at` is now obsolete. Additionally, added support for negative indices in Array, to behave in the same way as for Vector. # Important Notes Note that this workaround only addresses this particular perf issue. I'm pretty sure we will have more of such scenarios. Before the change `averageOverVector` benchmark averaged around `0.033 ms/op` now it does consistently `0.016 ms/op`, similarly to `averageOverArray`. --- CHANGELOG.md | 2 + .../Base/0.0.0-dev/src/Data/Array.enso | 17 +++--- .../Base/0.0.0-dev/src/Data/Vector.enso | 53 +++++++------------ .../Base/0.0.0-dev/src/Error/Common.enso | 1 + docs/runtime-guide.md | 23 ++++++++ .../builtin/error/IndexOutOfBoundsError.java | 14 +++++ ...afeAtVectorNode.java => AtVectorNode.java} | 27 ++++++---- .../builtin/mutable/ArrayAtNode.java | 22 +++++--- .../interpreter/runtime/builtin/Error.java | 6 +++ test/Tests/src/Data/Array_Spec.enso | 5 +- 10 files changed, 111 insertions(+), 59 deletions(-) create mode 100644 engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/error/IndexOutOfBoundsError.java rename engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/{UnsafeAtVectorNode.java => AtVectorNode.java} (54%) diff --git a/CHANGELOG.md b/CHANGELOG.md index fcdd328105c..083050adf2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -399,6 +399,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 @@ -453,6 +454,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) 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 b75c39fde63..40335269823 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 e59f1186e68..1bbe9340a24 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,20 +146,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) _-> - 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. @@ -179,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 @@ -213,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. @@ -248,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. @@ -267,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 @@ -435,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. @@ -483,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. @@ -500,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. @@ -520,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. @@ -530,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. @@ -579,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 @@ -643,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 @@ -716,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. @@ -759,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. @@ -788,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 @@ -1133,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 @@ -1147,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 39c98bcd529..b16451ba2fb 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/docs/runtime-guide.md b/docs/runtime-guide.md index b636ed74531..23c34e1167f 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 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 00000000000..58b7d13051a --- /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/UnsafeAtVectorNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java similarity index 54% rename from engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/UnsafeAtVectorNode.java rename to engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java index 6e9cbd659f9..7ccdb5e7720 100644 --- 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/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,25 +9,33 @@ 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; +import org.enso.interpreter.runtime.error.DataflowError; @BuiltinMethod( type = "Vector", - name = "unsafe_at", + name = "at", description = "Returns an element of Vector at the specified index.") -public class UnsafeAtVectorNode extends Node { +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 { - return self.readArrayElement(index, interop, toEnso); + 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); - throw new PanicException( - ctx.getBuiltins().error().makeInvalidArrayIndexError(self, index), this); - } catch (UnsupportedMessageException e) { - throw new PanicException(e.getMessage(), 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 3fd46997446..cb52be9d135 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,25 +8,31 @@ 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.builtin.Builtins; -import org.enso.interpreter.runtime.error.PanicException; +import org.enso.interpreter.runtime.error.DataflowError; @BuiltinMethod(type = "Array", name = "at", description = "Get element of a polyglot array") 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); - return convert.execute(r); + return readElement(self, index); } catch (UnsupportedMessageException ex) { CompilerDirectives.transferToInterpreter(); throw new IllegalStateException(ex); - } catch (InvalidArrayIndexException 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); - Builtins builtins = ctx.getBuiltins(); - throw new PanicException(builtins.error().makeInvalidArrayIndexError(self, at), this); + return DataflowError.withoutTrace( + ctx.getBuiltins().error().makeIndexOutOfBoundsError(index, iop.getArraySize(self)), 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 63597310200..bca2c12adde 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 class Error { 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 class Error { 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 0cba5617c6c..49b94447a74 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" <|