Aot pre-compiled machine #556

andreaTP · 2024-10-03T13:42:07Z

This builds on top of #552 , so review only the second commit here.

Supersede and finalize the effort started in #494

Notes:

I couldn't get rid of the generated Machine implementation to get the interoperability working
this can be ported to an annotation processor if required, but there are a few limitations in reading files from the disk there

electrum

This looks great!

wabt-aot/pom.xml

wabt-aot/src/main/java/com/dylibso/chicory/aot/wabt/Files.java

wabt-aot/pom.xml

runtime/src/test/java/com/dylibso/chicory/runtime/ModuleTest.java

machines-tests/src/test/java/com/dylibso/chicory/testing/MachinesTest.java

machines-tests/pom.xml

aot-maven-plugin/src/main/java/com/dylibso/chicory/maven/aot/AotGenMojo.java

electrum · 2024-10-04T00:28:22Z

Let me see if I can change the generated module so that we don't need to generate the machine.

electrum · 2024-10-04T07:50:35Z

It's mostly done, but I'll need to finish it tomorrow: https://github.com/electrum/chicory/commits/aot-machine

andreaTP · 2024-10-04T10:33:25Z

Let me see if I can change the generated module so that we don't need to generate the machine.

That would be really cool, we still need to generate a wrapper(or something) usable by the user as directly referencing the generated .class horribly breaks the IDE.

andreaTP · 2024-10-04T15:58:15Z

Things are coming together pretty nicely!
Thanks a lot @electrum for supporting the effort.

aot-maven-plugin/src/main/java/com/dylibso/chicory/maven/aot/AotGenMojo.java

electrum · 2024-10-05T06:32:40Z

runtime/src/main/java/com/dylibso/chicory/runtime/Instance.java

            return this;
        }

+        public Builder withSkipImportMapping(boolean s) {


Instead of adding this, we should refactor the AOT compiler so that it doesn't need Instance, as it actually only uses Module. The reason it takes Instance is because it implements the Machine interface. But the compilation itself doesn't use it.

I'll put up a PR to refactor it. I like that these changes push the code to have a better structure.

I like this approach and I'll follow up, let me have a little more slack time 😅

electrum · 2024-10-05T06:33:58Z

wabt/pom.xml


  <name>Chicory - wabt</name>
-  <description>wabt tools running in pure Java with Chicory</description>
+  <description>wabt tools compiled to pure Java with Chicory</description>


Technical "compiled to pure JVM bytecode" but that doesn't sound as nice 😆

electrum · 2024-10-05T06:35:35Z

wabt/pom.xml

  <dependencies>
+    <dependency>
+      <groupId>com.dylibso.chicory</groupId>
+      <artifactId>aot-runtime</artifactId>


This is why we need to add it to the BOM

My understanding here is that we want to add to the bom only modules that are actually going to make the "supported" 1.0.
The aot is still out of this category imho(needs a bit more investment), I aim for it to be next, and I'm happy to help whenever possible, but I'm a bit on the fence of adding it to the bom now as it might create misaligned expectations.

With the latest changes in #565, aot-runtime is no longer needed at all!

machine-tests/src/test/java/com/dylibso/chicory/testing/MachinesTest.java

electrum · 2024-10-05T06:59:33Z

machine-tests/src/test/java/com/dylibso/chicory/testing/MachinesTest.java

+    private Instance.Builder quickJsInstanceBuilder() {
+        return Instance.builder(
+                Parser.parse(
+                        MachinesTest.class.getResourceAsStream(


Can use getclass()

electrum · 2024-10-05T07:03:20Z

machine-tests/src/test/java/com/dylibso/chicory/testing/MachinesTest.java

+    }
+
+    @Test
+    public void shouldUseMachineCallOnlyForExport() throws Exception {


Should we move this test to the AOT module? It's not dependent on precompilation

good suggestion 👍 it's just a sanity check from the prev implementation of call_indirect at the end of the day.

Turns out that is not dependent on precompilation, but it uses wabt and moving it to aot introduces a circular dependency.
Let's keep it as a follow up.

electrum · 2024-10-05T07:32:18Z

aot-maven-plugin/pom.xml

+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-plugin-plugin</artifactId>
+        <configuration>
+          <goalPrefix>prefix</goalPrefix>


Should this be chicory?

electrum · 2024-10-05T09:20:16Z

aot-maven-plugin/src/main/java/com/dylibso/chicory/maven/aot/AotGenMojo.java

+        var instance =
+                Instance.builder(Parser.parse(wasmFile))
+                        .withMachineFactory(inst -> new AotMachine(name, inst))
+                        .withStart(false)
+                        .withInitialize(false)
+                        .withSkipImportMapping(true)
+                        .build();
+        var compiled = ((AotMachine) instance.getMachine()).compiledClass();


With #565, this can become

var module = Parser.parse(wasmFile); var result = AotCompiler.compileModule(module, name); var compiled = result.classBytes();

It returns Map<String, byte[]> because we'll need to support multiple classes for method splitting and possibly other features. Any additional classes will be nested classes of the provided class name.

Sounds like an ideal API, just give me a little to review 565 👍 thanks a lot for the support!

I updated the API to allow returning multiple classes, which is needed for inlining AotMethods, method splitting, etc. With that change, this code can become something like

for (Entry<String, byte[]> entry : result.classBytes()) { var binaryName = entry.getKey().replace('.', '/') + ".class" var targetFile = targetClassFolder.toPath().resolve(); try { Files.write(targetFile, entry.getValue()); } catch (IOException e) { throw new MojoExecutionException("Failed to write " + targetFile, e); } }

Note that the configured class name must not be a nested name like x.y.Foo.Bar as it's not possible to differentiate this from an uppercase Foo package, and more importantly, generating code into a nested class is not possible (see this and this for an idea of why).

andreaTP · 2024-10-12T11:50:21Z

aot-maven-plugin/src/main/java/com/dylibso/chicory/maven/aot/AotGenMojo.java

+        var split = name.split("\\.");
+        var finalFolder = targetClassFolder.toPath();
+        var finalSourceFolder = targetSourceFolder.toPath();
+        for (int i = 0; i < (split.length - 1); i++) {


this code is a little messy, but should be easy to improve over it.

andreaTP

I updated everything on top of the great latest improvement done by @electrum .
I go ahead and merge this, we can fix on top any leftover.

andreaTP requested review from bhelx, danielperano, electrum and evacchi October 3, 2024 13:42

andreaTP force-pushed the aot-maven-plugin-rev2 branch from 1df811d to d1306d9 Compare October 3, 2024 14:53

electrum approved these changes Oct 3, 2024

View reviewed changes

andreaTP force-pushed the aot-maven-plugin-rev2 branch 3 times, most recently from 0ece6c8 to 617d65e Compare October 4, 2024 15:40

electrum reviewed Oct 5, 2024

View reviewed changes

AOT pre-compiled machine

7d6ba99

andreaTP force-pushed the aot-maven-plugin-rev2 branch from 617d65e to 7d6ba99 Compare October 12, 2024 11:43

andreaTP commented Oct 12, 2024

View reviewed changes

andreaTP merged commit f9ed8c6 into dylibso:main Oct 12, 2024
13 checks passed

andreaTP mentioned this pull request Oct 22, 2024

[POC] Use the AOT compiled code instead of MH #494

Closed

Aot pre-compiled machine #556

Aot pre-compiled machine #556

Uh oh!

Conversation

andreaTP commented Oct 3, 2024

Uh oh!

electrum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

electrum commented Oct 4, 2024

Uh oh!

electrum commented Oct 4, 2024

Uh oh!

andreaTP commented Oct 4, 2024

Uh oh!

andreaTP commented Oct 4, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

electrum Oct 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

electrum Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreaTP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

electrum Oct 5, 2024 •

edited

Loading

electrum Oct 7, 2024 •

edited

Loading