-
Notifications
You must be signed in to change notification settings - Fork 60
feat: compile go-feature-flag to Java bytecode #1628
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @andreaTP, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant performance improvement for the Go-Feature-Flag provider by integrating the Chicory compiler. Instead of interpreting the WebAssembly module at runtime, the WASM is now compiled into Java bytecode during the build process. This change not only promises a substantial speedup but also simplifies deployment by embedding the compiled module directly, removing the need for external WASM file management. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
9b0071a to
3e45b1e
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.
Code Review
This pull request is a great improvement, migrating the GO Feature Flag provider to use the Chicory AOT compiler instead of the interpreter. This should result in a significant performance boost as mentioned in the description. The changes in pom.xml to use the chicory-compiler-maven-plugin and the corresponding adjustments in EvaluationWasm.java to load the pre-compiled module are well-implemented.
I have one suggestion to improve build reproducibility by pinning the version of the new Maven plugin.
Additionally, it seems the download-wasm.sh script is now obsolete with these changes and could be removed to keep the codebase clean.
Overall, this is a solid contribution that improves the performance and packaging of the provider.
| <groupId>org.codehaus.mojo</groupId> | ||
| <artifactId>exec-maven-plugin</artifactId> | ||
| <groupId>com.dylibso.chicory</groupId> | ||
| <artifactId>chicory-compiler-maven-plugin</artifactId> |
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.
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.
Fixed
This is not correct, |
Signed-off-by: andreatp <[email protected]>
3e45b1e to
4803d60
Compare
thomaspoignant
left a comment
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.
@andreaTP thanks a lot for this PR, this is such an honour to have you here.
Chicory is a really awesome lib and I was not able to find this optimisation myself, thanks a lot for this 🙇.
This is great to have the same thing but fast 🙌 .
|
Glad to see you’re making good use of Chicory, this is a great project, and I’m always happy to help 🙂 I also noticed that the Wasm module is built with the |
This PR
This PR uses the Chicory compiler at build time instead of the(default) slow interpreter.
You can check the differences in the docs.
With this configuration I'd expect a huge increase of performance, also you don't need anymore to carry around the original
wasmfile as everything is taken care by the Maven plugin.Related Issues
Fixes #1234523
Notes
@aepfli successful nerd snipe 😅 congratz!
Follow-up Tasks
Using the
@WasmModuleInterfaceannotation to have an automatically typed version of the exported functions.Given, at the moment, there are only 3 exported functions this looks superfluous ...
How to test
Same functionality and tests as before just going faster 🚀