Encapsulating array-like data and operations into a single package #7544

JaroslavTulach · 2023-08-10T10:23:51Z

Pull Request Description

Fixes #6299 by:

moving all array-like data classes into a single package - Done in 2201637
removing all public methods of Vector & Array - Done
hiding the classes from direct public access (Done in 1dd97a8 & 4c3e383)
offering ArrayLikeLengthNode and ArrayLikeAtNode & co. to access them - Done
simplify all other code by using those nodes - like in f128f8b and 1dd97a8 - Done

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

Make sure all Enso objects subclass EnsoObject and check for that
All code follows the style guides.
All code has been tested:
- Unit tests have been written where possible.
- Benchmarks show no regressions

project/FrgaalJavaCompiler.scala

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/builtins/TypeWithKind.java

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

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/builtins/TypeWithKind.java

project/FrgaalJavaCompiler.scala

…gthNode

JaroslavTulach · 2023-08-11T17:36:29Z

Current state seems to be stable. Let's do a bit of benchmarking. On par, except:

probably ArrayProxyBenchmarks_sumOverVectorBackedByDelegatingProxy
VectorBenchmarks_averageOverSlice
WarningBenchmarks_noWarningsVecSum and randomElementsVecSum
both VectorBenchmarks_averageOverArrayProxy*
maybe AtomBenchmarks_benchSumListMethods
certainly IfVsCaseBenchmarks

Let's delay querying length until really needed: 359a608 and let's try new benchmark - some improvements are visible, but there is still some work to do on:

ArrayProxyBenchmarks_sumOverVectorBackedByDelegatingProxy
WarningBenchmarks_noWarningsVecSum and randomElementsVecSum - improved, but used to be better
VectorBenchmarks_averageOverArrayProxy* - improved, but used to be better
IfVsCaseBenchmarks - improved, but used to be better
AtomBenchmarks_benchSumListMethods - still

Running new benchmarks check based on 4c3e383 - assuming it is no worse we'll need to find out how to speed the ones listed above up...

JaroslavTulach · 2023-08-13T16:45:43Z

6181b10 introduces Vector.EnsoOnly that is likely to speed up the IfVsCase benchmarks. Which has happened... IfVsCase benchmarks are no longer slow. Benchmarks seem to be fine.

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

Akirathan

Good job. I have written some minor comments.

Akirathan · 2023-08-14T08:21:18Z

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

 type Array_Proxy
+    ## PRIVATE
+    Value ignore


What is the reason for this ignored field?

Removing the Value ignore constructor turns Array_Proxy into a singleton and fails:

[error] Test org.enso.interpreter.test.MetaIsATest.typesAreNotInstancesOfThemselves failed: java.lang.AssertionError: [error] Type Array_Proxy shall not be instance of itself expected: [error] at org.enso.interpreter.test.MetaIsATest.typesAreNotInstancesOfThemselves(MetaIsATest.java:238)

I decided to add a constructor to "silent" this tests.

The proper fix is to remove the type Array_Proxy altogether, I think. It is not a type anyway. It is just a place for the new and from_proxy_object methods. However I happily leave this for @jdunkerley, @radeusgd and for another pull request.

distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Array_Like_Helpers.enso

.../java/org/enso/interpreter/node/expression/builtin/immutable/FromArrayBuiltinVectorNode.java

...src/main/java/org/enso/interpreter/node/expression/builtin/immutable/RemoveAtVectorNode.java

Akirathan · 2023-08-14T08:40:57Z

.../main/java/org/enso/interpreter/node/expression/builtin/text/util/TypeToDisplayTextNode.java

+import org.enso.interpreter.node.expression.builtin.meta.TypeOfNode;
+import org.enso.interpreter.runtime.callable.atom.Atom;
+import org.enso.interpreter.runtime.callable.atom.AtomConstructor;
+import org.enso.interpreter.runtime.data.Type;
+import org.enso.interpreter.runtime.type.TypesGen;


minor: Are these imports reordered with sbt enso/javafmt or is it your IDE doing this? I am asking because your setup seems to create inconsistent import ordering - in this file, org.enso imports are before com.oracle.truffle, whereare, in GetStackTraceNode.java it is the other way around.

I have seen a lot of import reordering in your recent PRs. I just want to make sure that we are on the same page and ensure whose setup is the prefered one.

VSCode (with Apache NetBeans based plugin) is configured to clean imports aggressively. For example it expands * imports to individual ones, removes unnecessary imports, etc. However the order is then finally set by javafmtAll.

I have seen a lot of import reordering in your recent PRs.

Yeah, I've noticed that myself as well.

on the same page and ensure whose setup is the preferred one.

javafmtAll is the final check, however it may be too "forgiving". I can change my settings to instruct the IDE to not touch imports automatically.

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/hash/HashMapToVectorNode.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/Array.java

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

JaroslavTulach · 2023-08-14T12:24:20Z

Another benchmark run to check the effect of ArrayBuilder builtin.

hubertp

Mostly fine, apart from the warnings bit

distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Array_Like_Helpers.enso

hubertp · 2023-08-14T13:18:03Z

...ntime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java


 @BuiltinMethod(
-    type = "Vector",
+    type = "Array_Like_Helpers",
    name = "at",
    description = "Returns an element of Vector at the specified index.")


probably needs an update as well

What kind of update would you suggest, @hubertp?

hubertp · 2023-08-14T13:21:25Z

...ain/java/org/enso/interpreter/node/expression/builtin/immutable/InsertBuiltinVectorNode.java

-  @Specialization(guards = "interop.hasArrayElements(values)")
-  Vector fromArrayWithArrayLikeObject(
-      Array vec,
+  private static EnsoObject insertBuiltin(


Could probably just move this method to fromObject?

Done in 80a74d9e49

...ain/java/org/enso/interpreter/node/expression/builtin/immutable/InsertBuiltinVectorNode.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/Array.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrayLikeHelpers.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrayLikeLengthNode.java

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

JaroslavTulach · 2023-08-15T11:24:31Z

benchmark_results.pdf show the state of the benchmarks at the time of merging. Mostly improvements or on par. Slight regression at:

about 30% in WarningBenchmarks_randomElementsVecSum
13% in VectorBenchmarks_averageOverArrayProxy... and VectorBenchmarks_averageAbstractList and VectorBenchmarks_averageOverArray

I merged anyway to avoid further conflicts. We can address the slowdowns in follow up issues.

Moving all array-like data classes into the same package

2201637

JaroslavTulach self-assigned this Aug 10, 2023

JaroslavTulach requested review from 4e6, hubertp and Akirathan as code owners August 10, 2023 10:23

JaroslavTulach marked this pull request as draft August 10, 2023 10:23

JaroslavTulach added 2 commits August 10, 2023 13:15

Unifying configuration of WithDebugCommand with annotation processors

b7efd8d

Hiding ArrayProxy as package private class

22f9714

JaroslavTulach commented Aug 10, 2023

View reviewed changes

project/FrgaalJavaCompiler.scala Outdated Show resolved Hide resolved

JaroslavTulach commented Aug 10, 2023

View reviewed changes

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/builtins/TypeWithKind.java Show resolved Hide resolved

JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Aug 10, 2023

Akirathan mentioned this pull request Aug 10, 2023

Remove list of hard-coded type names from TypeWithKind.java #7552

Closed

Akirathan reviewed Aug 10, 2023

View reviewed changes

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

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/builtins/TypeWithKind.java Show resolved Hide resolved

project/FrgaalJavaCompiler.scala Outdated Show resolved Hide resolved

enso-bot bot mentioned this pull request Aug 11, 2023

Unify & speed access to array-like objects behind ArrayXyzNode #6299

Closed

JaroslavTulach added 14 commits August 11, 2023 08:06

Let Meta*Test pass OK

d4c1140

Enso objects shall extend EnsoObject

91204f4

ArraySlice doesn't have to be public

8974b3c

Less methods in Array

58d661c

Replacing Array public methods with ArrayLikeLengthNode

db95190

ArrayLikeAtNode unifies access to index

e7c297e

Hiding all public methods of Vector

8fc5995

Fixing runtime/bench

cc9df48

Hiding Array.getItems() from public

0d64ede

Renaming to ArrayLikeCoerceToArrayNode

5c7b9cd

Renaming to ArrayLikeCopyToArrayNode

4d80810

Simplifying Flatten with the help of ArrayLikeAtNode and ArrayLikeLen…

f128f8b

…gthNode

Reformatting Java code

660a938

Removing check for UnsupportedMessageException

63941a6

Avoid querying length until really needed

359a608

JaroslavTulach added 3 commits August 13, 2023 18:03

Refactoring into many specialized innerclasses

abe16a7

Specialize length and at from Double and Long vectors

0433105

Create pure EnsoOnly vector from values that need no conversions

6181b10

JaroslavTulach commented Aug 14, 2023

View reviewed changes

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

Akirathan approved these changes Aug 14, 2023

View reviewed changes

Place Warnings into Vector.Generic

3c439ae

GregoryTravis approved these changes Aug 14, 2023

View reviewed changes

hubertp approved these changes Aug 14, 2023

View reviewed changes

Turning ArrayBuilder into a builtin

685580a

JaroslavTulach force-pushed the wip/jtulach/ArrayNodes_6299 branch from ae89884 to 685580a Compare August 14, 2023 15:39

JaroslavTulach added 2 commits August 15, 2023 06:23

Support for Vector_Builder.appendTo

33b1cb6

Merging with #7525

ba8931b

JaroslavTulach force-pushed the wip/jtulach/ArrayNodes_6299 branch from 772f341 to ba8931b Compare August 15, 2023 04:33

Documenting signatures of Array_Like_Helpers functions

fcad022

JaroslavTulach changed the title ~~Moving all array-like data classes into the same package~~ Encapsulating array-like data classes & operations into a single package Aug 15, 2023

JaroslavTulach added 8 commits August 15, 2023 07:09

Reformatted

2af6be8

Removing needless Javadoc

232eb1c

It's a fallback specification

3c99eeb

Avoid shouldNotReachHere. Use panics from ArrayPanics.

3ce8e17

One method less

969d8b2

Removing useless documentation

9bfa61e

Using @fallback to negate previous specializations

24309fc

Throw IAE, convert asGuestValue, convert to Illegal_Argument

dca7e99

JaroslavTulach changed the title ~~Encapsulating array-like data classes & operations into a single package~~ Encapsulating array-like data and operations into a single package Aug 15, 2023

JaroslavTulach merged commit 7a272ec into develop Aug 15, 2023
23 checks passed

JaroslavTulach deleted the wip/jtulach/ArrayNodes_6299 branch August 15, 2023 11:00

enso-bot bot mentioned this pull request Aug 16, 2023

State accepts only Type as key - explain in docs #7572

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encapsulating array-like data and operations into a single package #7544

Encapsulating array-like data and operations into a single package #7544

JaroslavTulach commented Aug 10, 2023 •

edited

Loading

JaroslavTulach commented Aug 11, 2023 •

edited

Loading

JaroslavTulach commented Aug 13, 2023 •

edited

Loading

Akirathan left a comment

Akirathan Aug 14, 2023

JaroslavTulach Aug 15, 2023

Akirathan Aug 14, 2023

JaroslavTulach Aug 15, 2023

JaroslavTulach commented Aug 14, 2023 •

edited

Loading

hubertp left a comment

hubertp Aug 14, 2023

JaroslavTulach Aug 15, 2023

hubertp Aug 14, 2023

JaroslavTulach Aug 15, 2023

JaroslavTulach commented Aug 15, 2023

Encapsulating array-like data and operations into a single package #7544

Encapsulating array-like data and operations into a single package #7544

Conversation

JaroslavTulach commented Aug 10, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

JaroslavTulach commented Aug 11, 2023 • edited Loading

JaroslavTulach commented Aug 13, 2023 • edited Loading

Akirathan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Aug 14, 2023 • edited Loading

hubertp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Aug 15, 2023

JaroslavTulach commented Aug 10, 2023 •

edited

Loading

JaroslavTulach commented Aug 11, 2023 •

edited

Loading

JaroslavTulach commented Aug 13, 2023 •

edited

Loading

JaroslavTulach commented Aug 14, 2023 •

edited

Loading