Skip to content

Conversation

@andreaTP
Copy link
Collaborator

Opening this PR in draft as a reference for further discussion on the future of the AOT compiler(and how it should be used).

@electrum
Copy link
Collaborator

Nice work! Allowing to ship AOT compiled code with the application makes sense for cases where the WASM is static. Some comments:

  • The refactoring to split out the aot-tests module is good and seems like something we'll need at some point. If you want to pull out into a separate PR, I'm happy to approve it now. It will make this POC easier to review.
  • This commits the generated machine adapter source code. Shouldn't that be generated at build time?
  • We should reuse the existing wast2json binary resource to avoid bloating the repository. Maybe create a shared module for it? (if we need more binaries, we could consider a separate repo that publishes them as Maven artifacts for reuse)
  • We'll need to make the package and class name configurable, in order to allow users to have multiple of them in the same application. This isn't an issue for AotMachine since it uses a separate class loader per module.

andreaTP added a commit that referenced this pull request Aug 30, 2024
~~On top of: #495 (needs to be merged first)~~
Follow up from: #494
andreaTP added a commit that referenced this pull request Sep 2, 2024
andreaTP added a commit to andreaTP/chicory that referenced this pull request Sep 2, 2024
~~On top of: dylibso#495 (needs to be merged first)~~
Follow up from: dylibso#494
andreaTP added a commit to andreaTP/chicory that referenced this pull request Sep 2, 2024
@andreaTP
Copy link
Collaborator Author

andreaTP commented Sep 3, 2024

Done another iteration, rebasing on top of the improvements already merged and mostly tackling:

  • This commits the generated machine adapter source code. Shouldn't that be generated at build time?

Now the aot-maven-plugin is generating the "StaticMachine" for the compiled code.
The code of the plugin is not amazing but is working for the use case (it needs at least another iteration before finalizing in a real PR).

Notes:

  • in CI the tests are running in ~10 seconds (on the current main is ~15 seconds)
  • the separation of aot-runtime make sense to me, I think I want to separate it in a PR

Issues:
Apart for a code cleanup, the biggest blocker for this PR to land is the fact that the IDE doesn't recognize the generated class file.
Because of this issue the Maven Plugin is executed in a separate Maven submodule, the generated sources are correctly recognized by the IDE and this "masks" the issue, but is not something we can ship to the users 😢
Any help or suggestions on how to fix is very welcome!

Question:
Do we agree on generating the "StaticMachine" as sources in the Maven Plugin or do we have(possibly better) alternatives?

@evacchi
Copy link
Collaborator

evacchi commented Sep 4, 2024

Because of this issue the Maven Plugin is executed in a separate Maven submodule, the generated sources are correctly recognized by the IDE and this "masks" the issue, but is not something we can ship to the users 😢

I quickly investigated again, and it looks like the generated-classes I had in mind was something I made up :D apparently some plugins do have one such directory but IDEs do not really honor it. So, one proposal could be to generate interfaces with method stubs, and let users interact with the interfaces. Constructors might be substituted for factories; e.g.:

// this goes in generated-sources
interface MyInterface {
   static MyInterface create() { return new GeneratedBytecode(); }
   // stubs go here...
}
// we don't care where this goes
class GeneratedBytecode implements MyInterface { ... }

this might workaround that problem (although the IDE might still complain about MyInterface#create...)

@andreaTP
Copy link
Collaborator Author

andreaTP commented Sep 4, 2024

one proposal could be to generate interfaces with method stubs

that's "kinda" what I'm doing already, but I have to admit that the experience is still poor IMHO 😢

@electrum
Copy link
Collaborator

electrum commented Sep 4, 2024

I searched around and it sounds like using a separate module is the only way to make the IDE work. While this not ideal, I don't think it is a blocker for adoption. This has benefits from a build perspective, because the module would only need to be rebuilt when the WASM binary changes or when Chicory is updated.

@andreaTP
Copy link
Collaborator Author

Superseded by #556

@andreaTP andreaTP closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants