-
Notifications
You must be signed in to change notification settings - Fork 40
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
Conversation
Provided that I have nothing against
Another option is to generate the |
There was a problem hiding this 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)
If we want to allow fast calls to host functions, we need to accept that If we generate code at compile time that uses |
correct
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. 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. As soon as this request is not a blocker I'm happy to support this effort! |
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 |
Happy to hear that we have a path forward! |
@andreaTP I updated this to generate code using an annotation processor. The generated suffix of |
446df97
to
ba111dc
Compare
There was a problem hiding this 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:
7ab2943
to
bae5731
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great!
@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). |
@electrum do you mind if I ask you to separate this development in another PR? We can merge this PR today and move on building on top. |
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. |
There was a problem hiding this 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 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. |
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:
byte[]
orString
from(ptr i32, len i32)
arguments. This gets complicated quickly, but some basic support might cover most use cases.strace
on Linux. This really needs argument type conversion to be useful, so that the traces can show strings rather than useless pointers.I suspect that @andreaTP will dislike the usage of
MethodHandle
. I'm thinking we can convert this to be an annotation processor that generates theHostFunction[]
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 calltoHostFunctions()
. An annotation processor would generate a new class, and I don't think it's possible forWasiPreview1
to statically reference that class (as it won't exist whenWasiPreview1
is compiled). I see a couple of options:WasiPreview1#toHostFunctions
would call some utility method likegetHostFunctions(WasiPreview1.class)
which would reflectively find the generated class.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.