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

@@ -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)
 

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.

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

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
 

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
 

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<String> getConstructorParamNames() {
+    return List.of("index", "length");
+  }
+}

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);
     }
   }
 }

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);
     }
   }
 }

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);
   }

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