Use ArraySlice to slice a Vector #3724

jdunkerley · 2022-09-20T15:49:01Z

[ci no changelog needed]

Pull Request Description

Use an ArraySlice to slice Vector.
Avoids memory copying for the slice function.

Important Notes

Test	Ref	New
New Vector	71.9	71.0
Append Single	26.0	27.7
Append Large	15.1	14.9
Sum	156.4	165.8
Drop First 20 and Sum	171.2	165.3
Drop Last 20 and Sum	170.7	163.0
Filter	76.9	76.9
Filter With Index	166.3	168.3
Partition	278.5	273.8
Partition With Index	392.0	393.7
Each	101.9	102.7

Note: the performance of New and Append has got slower from previous tests.

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run ide build and ./run ide watch.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/ArraySlice.java

radeusgd

Looks good to me overall, just a few small comments and a question on implementation that would be good if can be answered before merge.

radeusgd · 2022-09-22T10:44:25Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/ArraySlice.java

+      if (CompilerDirectives.inInterpreter()) {
+        if (!InteropLibrary.getUncached().hasArrayElements(storage)) {
+          throw new IllegalStateException("Vector needs array-like delegate, but got: " + storage);
+        }
+      }


I think I don't understand why this check is performed only in the interpreter. Won't this affect semantics depending on optimization levels?

Anyway, if left as-is I think it would be beneficial to add a short comment explaining why this is only checked in interpreter.

Unless there is some obvious reason that every Truffle developer should understand immediately, I think such an explanation will help future developers understand this decision.

We could do the check as assert. Asserts are ignored when Truffle does its PE compilation. We just need to make sure we run our tests with -ea.

Why can't the check be there always?

radeusgd · 2022-09-22T17:22:04Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Vector.java

+  @Builtin.Specialize
+  @Builtin.WrapException(from = UnsupportedMessageException.class, to = PanicException.class)
+  public final Vector slice(long start, long end, InteropLibrary interop)
+      throws UnsupportedMessageException {


In what circumstances is UnsupportedMessageException thrown?

Only if the backing storage no longer can answer the interop.getArraySize call - shouldn't happen.

test/Benchmarks/src/Vector.enso

test/Tests/src/Data/Vector_Spec.enso

JaroslavTulach

Not bad. If that works, it can go in. There is VectorBenchmarks.java - consider adding a sliced check in there.

JaroslavTulach · 2022-09-23T04:26:57Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/ArraySlice.java

+      if (CompilerDirectives.inInterpreter()) {
+        if (!InteropLibrary.getUncached().hasArrayElements(storage)) {
+          throw new IllegalStateException("Vector needs array-like delegate, but got: " + storage);
+        }
+      }


We could do the check as assert. Asserts are ignored when Truffle does its PE compilation. We just need to make sure we run our tests with -ea.

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/ArraySlice.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Vector.java

test/Tests/src/Data/Vector_Spec.enso

Parameters of type InteropLibrary are currently correctly generated only when annotated with `Builtin.Specialized`. Should eventually get rid of this limitation and merge the logic into both variants.

Each builtin `makeFunction` method has to be currently registered for reflection. Adding registration of `SliceVectorMethodGen` to make following example work ag ```bash enso$ sbt engine-runner-native/buildNativeImage enso$ ./runner --run engine/runner-native/src/test/resources/Factorial.enso 6 ``` # Important Notes Regression caused by #3724 - Once the above code is executed in the CI, we'll discover such breakages before integration.

jdunkerley requested a review from hubertp September 20, 2022 15:49

JaroslavTulach reviewed Sep 21, 2022

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso Outdated Show resolved Hide resolved

JaroslavTulach reviewed Sep 21, 2022

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso Outdated Show resolved Hide resolved

JaroslavTulach reviewed Sep 21, 2022

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/ArraySlice.java Show resolved Hide resolved

jdunkerley force-pushed the wip/jd/vector-slice-182880080 branch from 13973fb to 19bbb0c Compare September 21, 2022 16:18

jdunkerley marked this pull request as ready for review September 21, 2022 16:37

jdunkerley requested review from radeusgd and 4e6 as code owners September 21, 2022 16:37

hubertp approved these changes Sep 22, 2022

View reviewed changes

jdunkerley force-pushed the wip/jd/vector-slice-182880080 branch 2 times, most recently from abfc064 to 151c749 Compare September 22, 2022 16:24

radeusgd approved these changes Sep 22, 2022

View reviewed changes

JaroslavTulach approved these changes Sep 23, 2022

View reviewed changes

jdunkerley and others added 9 commits September 23, 2022 09:16

Fix benchmark.

25e7ff9

First pass at ArraySlice.

34c9589

Java Format.

1a3eb5d

Making slice return a Vector.

eb68daf

Java format.

421e7b6

InteropLibrary parameter is added in specialized

134ffdc

Parameters of type InteropLibrary are currently correctly generated only when annotated with `Builtin.Specialized`. Should eventually get rid of this limitation and merge the logic into both variants.

Fix built in name.

cfc681c

Few slice tests.

b680ad7

end not length!

0df33f0

jdunkerley force-pushed the wip/jd/vector-slice-182880080 branch from 151c749 to 0df33f0 Compare September 23, 2022 08:54

jdunkerley added 3 commits September 23, 2022 10:16

PR comments.

dd3e9ab

Slice benchmark.

5181e7f

Fix new benchmark.

2d8df00

jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Sep 23, 2022

Merge branch 'develop' into wip/jd/vector-slice-182880080

a4b88d6

mergify bot merged commit a3de3c6 into develop Sep 23, 2022

mergify bot deleted the wip/jd/vector-slice-182880080 branch September 23, 2022 15:13

JaroslavTulach mentioned this pull request Sep 26, 2022

Registering SliceVectorMethodGen for reflection #3737

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ArraySlice to slice a Vector #3724

Use ArraySlice to slice a Vector #3724

jdunkerley commented Sep 20, 2022 •

edited

Loading

radeusgd left a comment

radeusgd Sep 22, 2022

radeusgd Sep 22, 2022

JaroslavTulach Sep 23, 2022

radeusgd Sep 23, 2022

radeusgd Sep 22, 2022

jdunkerley Sep 23, 2022

JaroslavTulach left a comment

JaroslavTulach Sep 23, 2022

Use ArraySlice to slice a Vector #3724

Use ArraySlice to slice a Vector #3724

Conversation

jdunkerley commented Sep 20, 2022 • edited Loading

Pull Request Description

Important Notes

Checklist

radeusgd left a comment

Choose a reason for hiding this comment

radeusgd Sep 22, 2022

Choose a reason for hiding this comment

radeusgd Sep 22, 2022

Choose a reason for hiding this comment

JaroslavTulach Sep 23, 2022

Choose a reason for hiding this comment

radeusgd Sep 23, 2022

Choose a reason for hiding this comment

radeusgd Sep 22, 2022

Choose a reason for hiding this comment

jdunkerley Sep 23, 2022

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Sep 23, 2022

Choose a reason for hiding this comment

jdunkerley commented Sep 20, 2022 •

edited

Loading