Every builtin type has a common super class

[Akirathan]

Pull Request Description

This PR is separated from #11589. It only introduces BuiltinObject, a common supertype for all the builtin types. It does not change any behavior.

BuiltinObject defines hasMetaObject, getMetaObject, hasType and getType messages, so they no longer have to be implemented in subclasses.

Important Notes

• Introduce also test BuiltinsJavaInteropTest
• Builtin annotation processor enforces that every builtin class extend BuiltinObject class.

Checklist

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

• Ensure no benchmark regressions
• The documentation has been updated, if necessary.
• All code follows the
  Scala,
  Java,
  TypeScript,
  and
  Rust
  style guides. In case you are using a language not listed above, follow the Rust style guide.
• Unit tests have been written where possible.
• If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
  or the Snowflake database integration, a run of the Extra Tests has been scheduled.
  • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

[Akirathan]

The Invalid polyglot layer sharing AssertionErrors (for example at https://github.com/enso-org/enso/actions/runs/12316940368/job/34378324502#step:7:6027) were caused by caching Type inside BuiltinObject. Fixed by 7510906 - JVM Tests succeeds after this change.

GitHub

Every builtin type has a common super class · enso-org/enso@8feab15

Enso Analytics is a self-service data prep and analysis platform designed for data teams. - Every builtin type has a common super class · 8feab15

[JaroslavTulach]

It is suprising the BuiltinObject instances are shared between different EnsoContext. Can you investigate why that happens? The WeakReference is poor solution (both in this PR and in the EnsoProjectNode).

[Akirathan]

It is suprising the BuiltinObject instances are shared between different EnsoContext. Can you investigate why that happens?

@JaroslavTulach I am not sure how to reproduce / investigate this problem. My latest attempt is the following test on 8feab15:

package org.enso.interpreter.test.builtins;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;

import java.util.ArrayList;
import java.util.List;
import org.enso.interpreter.node.expression.builtin.meta.EqualsNode;
import org.enso.interpreter.runtime.data.Type;
import org.enso.interpreter.runtime.library.dispatch.TypeOfNode;
import org.enso.interpreter.test.ValuesGenerator;
import org.enso.interpreter.test.ValuesGenerator.Language;
import org.enso.test.utils.ContextUtils;
import org.graalvm.polyglot.Context;
import org.graalvm.polyglot.Value;
import org.junit.Test;

public class BuiltinsSharedEngineTest {
  public static List<Value> generateBuiltinObjects(Context ctx) {
    var valuesGenerator = ValuesGenerator.create(ctx, Language.ENSO);
    var builtinTypes = new ArrayList<Value>();
    ContextUtils.executeInContext(
        ctx,
        () -> {
          valuesGenerator.allTypes().stream()
              .filter(
                  val -> {
                    var asType = getType(val, ctx);
                    return !shouldSkipType(asType, ctx);
                  })
              .forEach(builtinTypes::add);
          return null;
        });
    return builtinTypes;
  }

  private static Type getType(Value object, Context ctx) {
    var unwrapped = ContextUtils.unwrapValue(ctx, object);
    return TypeOfNode.getUncached().findTypeOrNull(unwrapped);
  }

  private static boolean shouldSkipType(Type type, Context ctx) {
    if (type == null) {
      return true;
    }
    if (!type.isBuiltin()) {
      return true;
    }
    var builtins = ContextUtils.leakContext(ctx).getBuiltins();
    var typesToSkip =
        List.of(
            builtins.function(), builtins.dataflowError(), builtins.warning(), builtins.nothing());
    var shouldBeSkipped = typesToSkip.stream().anyMatch(toSkip -> toSkip == type);
    return shouldBeSkipped;
  }

  @Test
  public void foo() {
    var ctx1 = ContextUtils.createDefaultContext();
    var builtinsFromCtx1 = generateBuiltinObjects(ctx1);
    // Ensure BuiltinObject cached Type from ctx1
    ContextUtils.executeInContext(ctx1, () -> {
      var equalsNode = EqualsNode.getUncached();
      for (var builtin : builtinsFromCtx1) {
        var meta = builtin.getMetaObject();
        assertThat("Has not-null meta", meta, is(notNullValue()));
        var res = equalsNode.execute(null, 42L, meta);
        assertThat(res.isTrue(), is(false));
      }
      return null;
    });

    // Use the cached types from ctx1 in ctx2
    var ctx2 = ContextUtils.createDefaultContext();
    ContextUtils.executeInContext(ctx2, () -> {
      var equalsNode = EqualsNode.getUncached();
      for (var builtin : builtinsFromCtx1) {
        var meta = builtin.getMetaObject();
        assertThat("Has not-null meta", meta, is(notNullValue()));
        var res = equalsNode.execute(null, 42L, meta);
        assertThat(res.isTrue(), is(false));
      }
      return null;
    });
  }
}

i.e., trying to ensure that BuiltinObject has cached type from ctx1, and reuse this cached type in a different context by passing it to EqualsNode. But this test still passes.

Do you have some different ideas / suggestions?

[Akirathan]

Another attempt to investigate the InvalidSharing exception is on b58a29d - just throw an AssertionError if there is a mismatch of contexts or builtin types between the current value and the cached value. Now, the tests should throw the assertion error in the commit instead of InvalidSharing exceptions.

[Akirathan]

After some pair programming with @JaroslavTulach, we managed to find the culprit of the invalid sharing assertion errors. The following patch fixes it:

diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/Vector.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/Vector.java
index 697898208a..d0e10cfd0f 100644
--- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/Vector.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/Vector.java
@@ -19,9 +19,6 @@ import org.enso.interpreter.runtime.warning.WarningsLibrary;
 @ExportLibrary(InteropLibrary.class)
 @Builtin(pkg = "immutable", stdlibName = "Standard.Base.Data.Vector.Vector", name = "Vector")
 abstract class Vector extends BuiltinObject {
-  private static final Vector EMPTY_LONG = new Long(new long[0]);
-  private static final Vector EMPTY_DOUBLE = new Double(new double[0]);
-  private static final Vector EMPTY_VECTOR = new EnsoOnly(new Object[0]);
 
   protected Vector() {}
 
@@ -91,7 +88,7 @@ abstract class Vector extends BuiltinObject {
 
   static Vector fromLongArray(long[] arr) {
     if (arr == null || arr.length == 0) {
-      return EMPTY_LONG;
+      return new Long(new long[0]);
     } else {
       return new Long(arr);
     }
@@ -99,7 +96,7 @@ abstract class Vector extends BuiltinObject {
 
   static Vector fromDoubleArray(double[] arr) {
     if (arr == null || arr.length == 0) {
-      return EMPTY_DOUBLE;
+      return new Double(new double[0]);
     } else {
       return new Double(arr);
     }
@@ -107,7 +104,7 @@ abstract class Vector extends BuiltinObject {
 
   static Vector fromEnsoOnlyArray(Object[] arr) {
     if (arr == null || arr.length == 0) {
-      return EMPTY_VECTOR;
+      return new EnsoOnly(new Object[0]);
     } else {
       return new EnsoOnly(arr);
     }

[JaroslavTulach]

   static Vector fromLongArray(long[] arr) {
     if (arr == null || arr.length == 0) {
-      return EMPTY_LONG;
+      return new Long(new long[0]);
     } else {
       return new Long(arr);
     }

The use of EMPTY_LONG can be replaced, if useful...

Many Truffle implementations of languages use a concept of class loader shared instances. For example Graal.js null is also a (class loader wide) singleton.

However that's not the case of Enso. Not even our Nothing is fully shared - Nothing is EnsoContext shared only. As such these EMPTY_LONG & co. class loader wide singletons are exceptional in Enso concepts and no surprise they cause the invalid sharing exceptions in this PR.

Solutions

• always allocate new value instead of using EMPTY_LONG & co.
• share EMPTY_LONG in EnsoContext - e.g. EnsoContext.get(...).getBuiltins().vector().emptyLong()
• modify our code to be ready for classloader shared instances

[JaroslavTulach]

No sharing layer errors anymore. One minor problem with @TruffleBoundary. Check the benchmarks, integrate and let's move on.

[JaroslavTulach]

There is a native image built failure caused by missing @TruffleBoundary.

[Akirathan]

Benchmarks scheduled:

• engine: https://github.com/enso-org/enso/actions/runs/12471172474
• stdlib: https://github.com/enso-org/enso/actions/runs/12471182303

GitHub

Benchmark Engine · enso-org/enso@36e39c2

Enso Analytics is a self-service data prep and analysis platform designed for data teams. - Benchmark Engine · 36e39c2

GitHub

Benchmark Standard Libraries · enso-org/enso@36e39c2

Enso Analytics is a self-service data prep and analysis platform designed for data teams. - Benchmark Standard Libraries · 36e39c2

[Akirathan]

Benchmarks are OK, I will address the last review comment and merge.