refactor(java):separate resposibilities on java engine so static methods can be exposed and do memory management that should work on Java 18 and above #198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sighphyre
commented
Jan 13, 2025
| Class<?> cleanerClass = Class.forName("java.lang.ref.Cleaner"); | ||
| Method registerMethod = cleanerClass.getMethod("register", Object.class, Runnable.class); | ||
|
|
||
| // Avoid capturing the engine itself in the lambda, otherwise this prevents GC! |
There was a problem hiding this comment.
I didn't spend 3 hours searching for this bug. I'm not salty at all
sighphyre
commented
Jan 13, 2025
| takeFeaturesFromResource(customEngine, "custom-strategy-tests.json"); | ||
| Boolean result = customEngine.isEnabled(featureName, context); | ||
| assertNotNull(result); | ||
| assertEquals(expectedIsEnabled, result); | ||
| } | ||
|
|
||
| @Test | ||
| void testResourceCleanup() throws InterruptedException { |
There was a problem hiding this comment.
Borrowed but modified from the existing implementation to use a PhantomReference rather than grabbing big chunks of GC'd references and hoping Yggdrasil is one of them (nothing wrong with the other approach, this just feels a bit more direct)
chriswk
approved these changes
Jan 14, 2025
| import com.sun.jna.FunctionMapper; | ||
| import com.sun.jna.NativeLibrary; | ||
|
|
||
| class CamelToSnakeMapper implements FunctionMapper { |
There was a problem hiding this comment.
That was me yesterday haha
gastonfournier
approved these changes
Jan 14, 2025
Comment on lines
+10
to
+22
| public String getFunctionName(NativeLibrary library, Method method) { | ||
| String methodName = method.getName(); | ||
| StringBuilder snakeCaseName = new StringBuilder(); | ||
|
|
||
| for (char c : methodName.toCharArray()) { | ||
| if (Character.isUpperCase(c)) { | ||
| snakeCaseName.append('_').append(Character.toLowerCase(c)); | ||
| } else { | ||
| snakeCaseName.append(c); | ||
| } | ||
| } | ||
|
|
||
| return snakeCaseName.toString(); |
There was a problem hiding this comment.
I think this is a good candidate for a unit test... but looks sane to me
There was a problem hiding this comment.
Excellent suggestion, will do before merge
EDIT: Changed my mind. Gonna do this in another PR but before release
* feat(java): expose core version in java engine layer * fix(java): force native encoding to utf-8 so windows doesn't set it to something horrible (#200)
sighphyre
added a commit
that referenced
this pull request
Jan 14, 2025
…#197) * feat(java): expose list_known_features * refactor(java):separate resposibilities on java engine so static methods can be exposed and do memory management that should work on Java 18 and above (#198) * refactor(java): rework java engine so that static methods can be exposed cleanly * feat(java): expose core version in java engine layer (#199) * fix(java): force native encoding to utf-8 so windows doesn't set it to something horrible (#200)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sorry, this one does two things because they're really difficult to separate:
Most of this is so we can do cool things like this.
Refactor Java Engine
This splits up the responsibilities more cleanly (according to one ginger's opinions) of the Java engine. The three components do the following:
NativeLoader
Previously
YggdrasilFFI, theNativeLoadernow only holds the responsibility for loading the Yggdrasil binary into memory. This is a purely static class since there's only ever one binary and that needs to be loaded before anything else can be done. Importantly, no pointer management is done at this level.NativeLoaderconsumes the newCamelToSnakeMapperto provide nice camelCase names to the Java code instead of using a decorator to deal with the snake_case names.UnleashFFI
This is the JNA interface to the binary. This exposes a Java interface to consume the Rust code. This does also provide a
getInstancewhich returns the only valid implementation to access the binary. Consumers are free to mock this interface for testing.UnleashEngine
The code that binds the domain logic in Java to the domain logic in Rust. This does do pointer management. Specifically, one UnleashEngine in Java land holds one pointer to one Yggdrasil struct in Rust land. This component therefore also takes over the tests the YggdrasilFFI was previously doing to ensure the engine was properly cleaned up
Dynamically Mount Cleaner Instead of Finalizer
Sooo... small problem. Finalizers are deprecated in Java 17 and marked for removal. Later JVMs provide the option to turn them off entirely, which means our existing implementation just quietly leaks. The more delightful problem is that the alternative to finalizers, the
CleanerAPI, is only Java 9 and above. And we support Java 8.So we inject some code to check the JVM version. If you're Java 8 or below your finalizer executes as normal. If you're Java 9 or above, we use reflection to dynamically mount a cleaner and invoke that. Why reflection? Because we support source level compatibility for Java 8, which can't be compiled with direct references to
Cleanerobjects. Similar to the approach Spring uses