Primitive, non boxing, long array based MStack #531

andreaTP · 2024-09-18T14:08:36Z

Supersede #447 this is a complete implementation, already passing all the tests up to the interpreter, can be tested with something like:

 mvn clean install -pl runtime-tests -am

I started, but got a bit confused about the changes necessary in "aot", @electrum or @danielperano I would love some help in finalizing this PR 🙏 and learning a bit more how asm works ...

This is a prerequisite to avoid arrays construction here, the plan is to open up the underlying array so that we can perform operations just using the offset 🤞

andreaTP · 2024-09-19T17:46:41Z

update: I benchmarked the interpreter and got good improvements on this branch, I think it's in the right direction!
Regarding the AOT, the changes seem to be a bit more invasive than expected, it would be great to have some help.

aot/src/main/java/com/dylibso/chicory/aot/AotMachine.java

electrum · 2024-09-20T19:00:20Z

aot/src/main/java/com/dylibso/chicory/aot/AotMachine.java

-        emitBoxArguments(asm, type.params());
+        // emitBoxArguments(asm, type.params());
+        // asm.visitLdcInsn(LONG);
+        for (int i = 0; i < type.params().size(); i++) {


This needs to use slotCount() to compute the slot parameter correctly, as long/double use two slots in JVM bytecode. This annoyance means you always need to know the type you are dealing with.

The incoming parameters to this method are a native Java value: int, long, float, double

aot/src/main/java/com/dylibso/chicory/aot/AotMethods.java

aot/src/main/java/com/dylibso/chicory/aot/AotUtil.java

andreaTP · 2024-09-25T14:11:02Z

I did a little benchmarking on my super noisy machine(take the results with a lot of salt):

Chicory 0.0.12:

With GraalJIT:
Benchmark                                      Mode  Cnt      Score        Error  Units
ChicoryAotTest.chicoryAotTest                  avgt    3   1282.401 ±    51.518  ns/op
ChicoryInterpreterTest.chicoryInterpreterTest  avgt    3  63086.906 ± 18526.425  ns/op

Without GraalJIT:
Benchmark                                      Mode  Cnt      Score        Error  Units
ChicoryAotTest.chicoryAotTest                  avgt    3   1222.858 ±    326.915  ns/op
ChicoryInterpreterTest.chicoryInterpreterTest  avgt    3  87630.869 ± 138021.856  ns/op

---

Current main:

With GraalJIT:
Benchmark                                      Mode  Cnt      Score       Error  Units
ChicoryAotTest.chicoryAotTest                  avgt    3   1237.301 ±   423.694  ns/op
ChicoryInterpreterTest.chicoryInterpreterTest  avgt    3  71274.055 ± 11410.525  ns/op

Without GraalJIT:
Benchmark                                      Mode  Cnt       Score       Error  Units
ChicoryAotTest.chicoryAotTest                  avgt    3    1349.806 ±   945.091  ns/op
ChicoryInterpreterTest.chicoryInterpreterTest  avgt    3  101900.141 ± 38884.734  ns/op

---

Primitive longs (this branch):

With GraalJIT:
Benchmark                                      Mode  Cnt      Score       Error  Units
ChicoryAotTest.chicoryAotTest                  avgt    3   1153.859 ±   984.977  ns/op
ChicoryInterpreterTest.chicoryInterpreterTest  avgt    3  41791.372 ± 43360.065  ns/op

Without GraalJIT:
Benchmark                                      Mode  Cnt      Score       Error  Units
ChicoryAotTest.chicoryAotTest                  avgt    3    899.077 ±   483.678  ns/op
ChicoryInterpreterTest.chicoryInterpreterTest  avgt    3  49629.772 ± 88844.628  ns/op

andreaTP · 2024-09-25T14:54:59Z

CI is green, ready for final review!

And thanks to @evacchi that paired and kept me sane in getting there with the AOT!

Benchmark from the CI:

evacchi · 2024-09-25T15:33:02Z

this is exciting!

andreaTP · 2024-09-30T08:58:22Z

Begging for review here 🙏 cc. @bhelx @electrum @danielperano

evacchi

I think this should be fine, but I think it would be better to have another pair of eyes on it :)

aot/src/main/java/com/dylibso/chicory/aot/AotUtil.java

electrum · 2024-09-30T18:28:11Z

aot/src/main/java/com/dylibso/chicory/aot/AotUtil.java

            default:
                throw new IllegalArgumentException("Unsupported ValueType: " + type.name());
        }
    }

-    public static Method boxer(ValueType type) {
+    public static Method convertToLong(ValueType type) {


public static void emitJvmToLong(MethodVisitor asm, ValueType type) { switch (type) { case I32: case ExternRef: case FuncRef: asm.visitInsn(Opcodes.I2L); return; case I64: return; case F32: emitInvokeStatic(asm, F32_TO_LONG); return; case F64: emitInvokeStatic(asm, F64_TO_LONG); return; } }

electrum · 2024-09-30T18:28:28Z

aot/src/main/java/com/dylibso/chicory/aot/AotUtil.java

            default:
                throw new IllegalArgumentException("Unsupported ValueType: " + type.name());
        }
    }

-    public static MethodHandle unboxerHandle(ValueType type) {
+    public static MethodHandle convertFromLongHandle(ValueType type) {


longToJvmHandle

electrum · 2024-09-30T18:28:41Z

aot/src/main/java/com/dylibso/chicory/aot/AotUtil.java

        } catch (IllegalAccessException e) {
            throw new AssertionError(e);
        }
    }

-    public static MethodHandle boxerHandle(ValueType type) {
+    public static MethodHandle convertToLongHandle(ValueType type) {


jvmToLongHandle

electrum · 2024-09-30T18:29:37Z

aot-tests/pom.xml

@@ -50,6 +50,12 @@
      <artifactId>junit-jupiter-engine</artifactId>
      <scope>test</scope>
    </dependency>
+    <!-- Enable better debugging -->


Should we simply remove the <optional>true</optional> from AOT, so that this is always available? The only downside is the additional size of the asm-util JAR.

electrum · 2024-09-30T20:53:11Z

aot/src/main/java/com/dylibso/chicory/aot/ValueConversions.java

+    // From long
+    public static int toInt(long val) {
+        return (int) val;
+    }
+
+    public static long toLong(long val) {
+        return val;
+    }
+
+    public static float toFloat(long val) {
+        return Value.longToFloat(val);
+    }
+
+    public static double toDouble(long val) {
+        return Value.longToDouble(val);
+    }
+
+    // To Long
+    public static long asLong(int val) {
+        return val;
+    }
+
+    public static long asLong(long val) {
+        return val;
+    }
+
+    public static long asLong(float val) {
+        return Value.floatToLong(val);
+    }
+
+    public static long asLong(double val) {
+        return Value.doubleToLong(val);
+    }


Let's name these the same as the method constants. I find the overloads hard to read.

Suggested change

// From long

public static int toInt(long val) {

return (int) val;

}

public static long toLong(long val) {

return val;

}

public static float toFloat(long val) {

return Value.longToFloat(val);

}

public static double toDouble(long val) {

return Value.longToDouble(val);

}

// To Long

public static long asLong(int val) {

return val;

}

public static long asLong(long val) {

return val;

}

public static long asLong(float val) {

return Value.floatToLong(val);

}

public static long asLong(double val) {

return Value.doubleToLong(val);

}

public static int longToI32(long val) {

return (int) val;

}

public static long longToI64(long val) {

return val;

}

public static float longToF32(long val) {

return Value.longToFloat(val);

}

public static double longToF64(long val) {

return Value.longToDouble(val);

}

public static long i32ToLong(int val) {

return val;

}

public static long i64ToLong(long val) {

return val;

}

public static long f32ToLong(float val) {

return Value.floatToLong(val);

}

public static long f64ToLong(double val) {

return Value.doubleToLong(val);

}

electrum · 2024-09-30T20:55:54Z

aot/src/main/java/com/dylibso/chicory/aot/AotUtil.java

+            I64_2_LONG = ValueConversions.class.getMethod("asLong", long.class);
+            F32_2_LONG = ValueConversions.class.getMethod("asLong", float.class);
+            F64_2_LONG = ValueConversions.class.getMethod("asLong", double.class);
+            EXTREF_2_LONG = ValueConversions.class.getMethod("asLong", int.class);


No need for these anymore, as they are the same as the i32 version.

electrum · 2024-09-30T21:00:32Z

aot/src/main/java/com/dylibso/chicory/aot/AotUtil.java

        try {
-            return publicLookup().unreflect(unboxer(type));
+            return publicLookup().unreflect(convertFromLong(type));


Since we no longer have the Method version of the conversion functions, we'll probably want to add constants for the method handles and make this a switch statement.

electrum · 2024-09-30T21:32:29Z

function-processor/src/main/java/com/dylibso/chicory/function/processor/FunctionProcessor.java

-                    arguments.add(new MethodCallExpr(argExpr, "asFloat"));
+                    arguments.add(
+                            new MethodCallExpr(
+                                    new NameExpr("Float"),


Should these use the new methods in Value?

electrum · 2024-09-30T21:33:03Z

pom.xml

@@ -567,6 +567,7 @@
          <ignoredUnusedDeclaredDependencies>
            <dependency>com.dylibso.chicory:wasm-corpus</dependency>
            <dependency>org.junit.jupiter:junit-jupiter-engine</dependency>
+            <dependency>org.ow2.asm:asm-util</dependency>


Let's make this a non-optional dependency in AOT

electrum · 2024-10-01T01:09:36Z

aot/src/main/java/com/dylibso/chicory/aot/AotMachine.java

        }
        if (type.returns().size() > 1) {
            return result;
        }
-        result = filterReturnValue(result, boxerHandle(type.returns().get(0)));
-        return filterReturnValue(result, ValueWrapper.HANDLE);


I think ValueWrapper is unused now

now it's a LongArrayWrapper to: long -> long[]

andreaTP · 2024-10-02T09:33:37Z

ok, let's merge this, any follow up comment will be fixed on top.

andreaTP requested review from electrum, evacchi and danielperano September 18, 2024 14:08

andreaTP changed the title ~~Primitive long stack rev 2~~ Primitive, non boxing, long array based MStack Sep 18, 2024

andreaTP mentioned this pull request Sep 18, 2024

Exploration - long array primitive MStack #447

Closed

electrum reviewed Sep 20, 2024

View reviewed changes

andreaTP force-pushed the primitive-long-stack-rev-2 branch from cf3c868 to 22226f5 Compare September 24, 2024 08:37

andreaTP mentioned this pull request Sep 25, 2024

[aot] call_indirect optimization #540

Closed

andreaTP marked this pull request as ready for review September 25, 2024 14:54

andreaTP requested review from bhelx and electrum September 25, 2024 14:54

evacchi approved these changes Sep 30, 2024

View reviewed changes

electrum approved these changes Sep 30, 2024

View reviewed changes

electrum reviewed Oct 1, 2024

View reviewed changes

andreaTP added 11 commits October 1, 2024 12:16

a little better start

0cf529c

this is gonig the right direction now

2ca3c56

drawing the line

312e2b5

minor

32d2928

all tests passing

2a64a63

unboxing array based MStack

bb43d29

partial AOT implementation... need help

ce96be5

checkstyle

91a15f9

runtime-tests passing

33b8e45

going on

49f3d48

WIP

7546cce

andreaTP added 16 commits October 1, 2024 12:17

shrinking the diff

f5e9f12

getting there

e2793d8

a few more bits

98c07fd

cleanup

f4bdb2f

ext refs and func refs back to int

213e4be

minor

e818421

fix some table issues

efb8ef1

more

5dd2997

cleanup - working

107aca5

cleanup

ee133ef

Remove full class file printing

f63b8e7

Fix Readme

0cfbceb

remove outdated TODOs

333b771

review

9070a05

Use Value utility methos instead of raw Float and Double

19186ae

minor

c6f1cd6

andreaTP force-pushed the primitive-long-stack-rev-2 branch from 4dffefe to c6f1cd6 Compare October 1, 2024 11:17

finishing up

79ddbb7

andreaTP merged commit 3a46c2b into dylibso:main Oct 2, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Primitive, non boxing, long array based MStack #531

Primitive, non boxing, long array based MStack #531

andreaTP commented Sep 18, 2024

andreaTP commented Sep 19, 2024

electrum Sep 20, 2024

andreaTP commented Sep 25, 2024

andreaTP commented Sep 25, 2024 •

edited

Loading

evacchi commented Sep 25, 2024

andreaTP commented Sep 30, 2024

evacchi left a comment

electrum Sep 30, 2024

electrum Sep 30, 2024

electrum Sep 30, 2024

electrum Sep 30, 2024

electrum Sep 30, 2024

electrum Sep 30, 2024

electrum Sep 30, 2024

electrum Sep 30, 2024

electrum Sep 30, 2024

electrum Oct 1, 2024

andreaTP Oct 1, 2024

andreaTP commented Oct 2, 2024

Primitive, non boxing, long array based MStack #531

Primitive, non boxing, long array based MStack #531

Conversation

andreaTP commented Sep 18, 2024

andreaTP commented Sep 19, 2024

Choose a reason for hiding this comment

andreaTP commented Sep 25, 2024

andreaTP commented Sep 25, 2024 • edited Loading

evacchi commented Sep 25, 2024

andreaTP commented Sep 30, 2024

evacchi 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaTP commented Oct 2, 2024

andreaTP commented Sep 25, 2024 •

edited

Loading