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" <|