Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
JaroslavTulach opened this issue Apr 16, 2023 · 6 comments · Fixed by #7544
Closed

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

JaroslavTulach opened this issue Apr 16, 2023 · 6 comments · Fixed by #7544
Assignees
Labels
--data corruption or loss Important: data corruption or loss -compiler

Comments

@JaroslavTulach
Copy link
Member

Currently the Enso engine is using InteropLibrary to read elements from array-like objects. However that's not enough - we have to guarantee that such elements are converted to Enso supported objects using HostValueToEnsoNode. As various incidents indicate:

we often forget to do such conversion.

The standard Truffle way of avoiding such problem and also making the array-like object access fast is to define nodes that specialize according to the data (Vector, Array, foreign object). Let's define:

  • ArrayLengthNode
  • ArrayAtNode

and replace all array-like access with these nodes rather than using InteropLibrary directly.

@JaroslavTulach
Copy link
Member Author

While investigating #7420 it turned out that eliminating need for HostValueToEnsoNode conversion can speed things up:

diff --git engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Vector.java engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Vector.java
index a966d47a5d..e9e283deaa 100644
--- engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Vector.java
+++ engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Vector.java
@@ -119,7 +119,7 @@ public final class Vector implements TruffleObject {
       }
       return WithWarnings.wrap(EnsoContext.get(interop), toEnso.execute(v), extracted);
     }
-    return toEnso.execute(v);
+    return v;
   }
 
   public static Vector fromArray(Object arr) {

the runtime/benchOnly caseBench3 benchmark is 10% faster. Eliminate need for this check for Enso only Vectors.

@JaroslavTulach JaroslavTulach changed the title Unify access to array-like objects behind ArrayXyzNode Unify & speed access to array-like objects behind ArrayXyzNode Aug 3, 2023
@JaroslavTulach
Copy link
Member Author

The handling of warnings is also something that would better be unified as

demonstrates.

@enso-bot
Copy link

enso-bot bot commented Aug 11, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-10):

Progress: - cleanup array-like classes: #7544

Next Day: Array access unification

@enso-bot
Copy link

enso-bot bot commented Aug 12, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-11):

Progress: - Array access unification - #7544

Next Day: Finish array access unification

@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to 👁️ Code review in Issues Board Aug 13, 2023
@enso-bot
Copy link

enso-bot bot commented Aug 14, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-13):

Progress: - Array & Vector work: #7544 It should be finished by 2023-08-14.

Next Day: Finish array access unification

@enso-bot
Copy link

enso-bot bot commented Aug 15, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-14):

Progress: - ArrayBuilder builtin: ae89884

  • benchmarking
  • standups & meetings It should be finished by 2023-08-14.

Next Day: Finish array access unification

@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--data corruption or loss Important: data corruption or loss -compiler
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants