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

Add experimental annotation framework for functions #496

Merged
merged 2 commits into from
Sep 2, 2024

Conversation

electrum
Copy link
Collaborator

This implements a basic annotation framework, similar to the example shown in #482. It works and could be merged now, with the expectation that we'll try it out with more projects such as SQLite, and eventually extract it as a public interface in its own module.

Ideas for future improvements:

  • More argument type conversions. For example, accepting byte[] or String from (ptr i32, len i32) arguments. This gets complicated quickly, but some basic support might cover most use cases.
  • Automatic tracing of calls and results, for example, similar to strace on Linux. This really needs argument type conversion to be useful, so that the traces can show strings rather than useless pointers.
  • Support multiple return values.

I suspect that @andreaTP will dislike the usage of MethodHandle. I'm thinking we can convert this to be an annotation processor that generates the HostFunction[] adapter, though I'm not sure what the API would look like.

The current implementation doesn't change the API of WasiPreview1: users create it and call toHostFunctions(). An annotation processor would generate a new class, and I don't think it's possible for WasiPreview1 to statically reference that class (as it won't exist when WasiPreview1 is compiled). I see a couple of options:

  • WasiPreview1#toHostFunctions would call some utility method like getHostFunctions(WasiPreview1.class) which would reflectively find the generated class.
  • Change callers to use directly reference the generated class:
    WasiPreview1 instance = new WasiPreview1(logger, opts);
    HostFunctions[] functions = WasiPreview1.HostFunctions.get(instance);

Neither one seems ideal, so I'm hoping someone has better suggestions, if we want to go down the annotation processor route. I'm personally fine with the MethodHandle approach, as this is quite common in these sorts of frameworks, but there are advantages to generating code at compile time.

@evacchi
Copy link
Collaborator

evacchi commented Aug 29, 2024

Provided that I have nothing against MethodHandle (especially in this kind of use case), it should be fine for a class to reference a class generated by an AnnotationProcessor, because the processor should be invoked by javac automatically; e.g. consider AutoValue https://github.com/google/auto/blob/main/value/userguide/index.md#in-your-value-class

@AutoValue
abstract class Animal {
  static Animal create(String name, int numberOfLegs) {
    return new AutoValue_Animal(name, numberOfLegs);
  }

  abstract String name();
  abstract int numberOfLegs();
}

AutoValue_Animal is the generated class.

Another option is to generate the HostFunction "converter"; e.g. check this out for a raw sketch of the idea (here it is defined within WasiPreview1 but it could be a companion class)

https://github.com/dylibso/chicory/pull/484/files#diff-9037d84891e8043e756bfd63e2101686b67d1cad2cc2c4af3b2e6b48675acdf3R1217-R1338

Copy link
Collaborator

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the direction this is going, this is definitely one of the rough edges of our API at the moment.

I'm not afraid of the usage of MethodHandle as soon as we keep it optional, I see a couple of ways to move this forward:

  • move HostModuleFactory to a separate Maven sub-module, so that the user can decide when to use reflection or not
  • achieve the same result with an annotation-processor or similar (I, personally, would prefer this option)

@electrum
Copy link
Collaborator Author

If we want to allow fast calls to host functions, we need to accept that WasmFunctionHandle is not the right interface, as it requires boxing everything. The annotated functions in this PR could be called directly by AOT without any conversion. With WasmFunctionHandle, when calling from AOT, we will box everything just to fit that interface, then unbox before calling the host function.

If we generate code at compile time that uses WasmFunctionHandle, then we're stuck with that (without recompiling the user code) and there's no way to make it fast. Using MethodHandle at runtime allows us to choose the correct adaptation for the interpreter vs AOT. Generating code also adds a dependency on the low level APIs, so they have to be stable.

@andreaTP
Copy link
Collaborator

andreaTP commented Aug 29, 2024

Generating code also adds a dependency on the low level APIs, so they have to be stable.

correct

without recompiling the user code) and there's no way to make it fast

I understand this, but is usually acceptable.

All in all, I now better understand where you are going @electrum , and I do not want to prevent you going down this direction.
Would it be feasible to make this code "optional"?
Or to "somehow" bake it into the aot module?

I'm asking as I still value the fact that the interpreter is completely "reflection free" and I'd like to keep Wasi modules runnable without having to use reflection.
In absence of better options, we can still expose both the MethodHandle and WasmFunctionHandle APIs from 2 maven sub-modules.

As soon as this request is not a blocker I'm happy to support this effort!

@electrum
Copy link
Collaborator Author

My primary goal right now is a better interface for writing host functions (the WASI functions seem much nicer after this refactor), so I'll update this to use an annotation processor, allowing us to move forward and experiment with the annotation interface.

Thanks @evacchi for the pointer to AutoValue. I'm surprised that works, but it's exactly what we need.

We can follow up on the WasmFunctionHandle issue. At a minimum, we would need a union between that and MethodHandle, but using a method handle directly has the issue of knowing the "calling convention". This works fine in the beginning when there is only one calling convention, but becomes limiting if we want support more later.

@andreaTP
Copy link
Collaborator

Happy to hear that we have a path forward!

@electrum
Copy link
Collaborator Author

electrum commented Sep 1, 2024

@andreaTP I updated this to generate code using an annotation processor. The generated suffix of _ModuleFactory seems to match the direction that @evacchi is going with host modules, but I'm happy to rename anything here. The underscore is unconventional for Java classes, but makes it stand out as a generated class and won't conflict with user code (the naming is similar to AutoValue).

@electrum electrum changed the title Add annotation framework for WASI module Add experimental annotation framework for functions Sep 1, 2024
Copy link
Collaborator

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of nitpicks on dependencies but this is brilliant!

I think that this PR is a great start and I'm happy to move forward.
A couple of follow up:

  • docs:

    • instructing people how to use the annotation processor
    • settle expectations toward the supported functionality(e.g. handling of "boxed" primitives like Integer and similar notes)
  • next steps:

    • I'm working on something somehow "similar" with a slightly different scope
    • I'd need a bit more(e.g. emitting the Wasm Module specific "Machine" during compilation/generation) to finalize this PR
    • and I'm not yet sure how all of this will fit together 😊

pom.xml Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
Copy link
Collaborator

@evacchi evacchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

@electrum
Copy link
Collaborator Author

electrum commented Sep 2, 2024

@andreaTP That's cool! Takari Maven Plugin Testing Framework is a great way to test Maven plugins using normal unit tests. We use it for the Trino Maven Plugin (though it's still on JUnit 4).

@andreaTP
Copy link
Collaborator

andreaTP commented Sep 2, 2024

@electrum do you mind if I ask you to separate this development in another PR?
(not against it but I want to have the discussion about the memory layout of things).

We can merge this PR today and move on building on top.

@electrum
Copy link
Collaborator Author

electrum commented Sep 2, 2024

No problem, that's why I kept it as a separate commit. I dropped it from this PR.

I'll also follow up with a README section for this.

Copy link
Collaborator

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@electrum feel free to merge at your convenience, we can work on top of this in separate PRs.

@electrum electrum merged commit fffe966 into dylibso:main Sep 2, 2024
13 checks passed
@electrum electrum deleted the wasi branch September 2, 2024 22:07
@andreaTP
Copy link
Collaborator

andreaTP commented Sep 3, 2024

@electrum I took a look at the Takari Maven Testing project, and looks pretty interesting, although I decided(for now), to rely on a "go-to" solution we are already using in other projects (e.g. the fabric8 kubernetes client ).

I do like to have the "final project like" structure, and, even if not optimal, I'm used to debugging with this setup.
Happy to share tips and tricks when necessary.

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