JNI bindings for FileStorage #61
Conversation
|
The function |
Benchmark for a123167Click to view benchmark
|
|
@twitu thanks, looks like a good start!
Type
I think we shouldn't mix JNI code into pure Rust structures. Desktop developers should not be affected by JNI. Building and importing
Is there a way to reconstruct Result/Option type? Or would it be too inefficient?
Yes, but converting whole structures really sounds inefficient.. Is it too wild to create JNI bindings for |
|
The JNI bindings can be put behind a feature flag and they will only be compiled when the feature flag is set on. Separating the bindings from the logic may not be very helpful it is closely tied to the logic. There's no good way to wrap a generic Result type in a way that's accessible to Java. Option is doable but then it will lose out on a lot of useful information. I guess in this case the best approach is to return Java Exceptions and the let the library user figure out how to handle it and it some rare cases return a default value where it makes sense. I looked in implementation for wrapping container types, especially using A more efficient way will be to keep the BTreeMap on the Rust side and define a specific set of operations on it through the jni bindings. I guess the most basic operation will be to |
Benchmark for 744c333Click to view benchmark
|
Benchmark for 8396b25Click to view benchmark
|
|
@twitu putting JNI bindings behind a feature flag is good-enough solution for this moment. But I think, we'll probably separate it in the future, when it's debugged. Especially, when we'll work on Swift bindings. On other hand, I can imagine having both binding flavours in the same place, together with Rust code, if it'll appear to be ergonomic enough.
Can't we convert Rust
I like this approach. Not sure what exactly operations we'd need right now. We could make it a separate repo later, containing useful structures and more-or-less complete API. For now, the methods necessary to demonstrate storages are enough.
Do you want to do it in this PR or in a follow-up? |
Benchmark for f21bb89Click to view benchmark
|
Benchmark for 437cc53Click to view benchmark
|
Benchmark for a0f6bdfClick to view benchmark
|
Benchmark for 95b2160Click to view benchmark
|
Benchmark for ad27900Click to view benchmark
|
Benchmark for 7d98c27Click to view benchmark
|
Benchmark for 2fccdfaClick to view benchmark
|
Benchmark for 792a49fClick to view benchmark
|
Benchmark for 5e2dc89Click to view benchmark
|
Benchmark for cc2e79bClick to view benchmark
|
Benchmark for 8ceb461Click to view benchmark
|
|
CI is failing due to Java warnings. It is running perfectly locally. |
|
The logs from the failing CI show that there is probably an error not just warnings cd tests && \
if [ ! -f "junit.jar" ]; then \
curl -o "junit.jar" "https://repo1.maven.org/maven2/org/junit/platform/junit-platform-console-standalone/1.9.3/junit-platform-console-standalone-1.9.3.jar"; \
else \
echo "junit.jar already exists."; \
fi
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
100 2552k 100 2552k 0 0 9533k 0 --:--:-- --:--:-- --:--:-- 9525k
(cd ../target/debug && \
export LD_LIBRARY_PATH=$PWD && \
cd ../../fs-storage/tests && \
javac -Xlint:none -d out FileStorage.java && \
javac -Xlint:none -d out -cp out:junit.jar FileStorageTest.java && \
java -jar junit.jar --class-path out --scan-class-path)
Note: FileStorageTest.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
thread '<unnamed>' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/jni-0.21.1/src/wrapper/jnienv.rs:520:9:
NullDeref("*JNIEnv")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5
Aborted (core dumped)
make: *** [Makefile:24: java-test] Error 134 |
|
locally i am getting this : (im on debian) pushkarm029@thinkpad:~/ark-rust(jni-bindings○) » cd fs-storage
pushkarm029@thinkpad:~/ark-rust/fs-storage(jni-bindings○) » make test
cargo build --release --features jni-bindings
Finished `release` profile [optimized] target(s) in 0.24s
cd tests && \
if [ ! -f "junit.jar" ]; then \
curl -o "junit.jar" "https://repo1.maven.org/maven2/org/junit/platform/junit-platform-console-standalone/1.9.3/junit-platform-console-standalone-1.9.3.jar"; \
else \
echo "junit.jar already exists."; \
fi
junit.jar already exists.
(cd ../target/debug && \
export LD_LIBRARY_PATH=$PWD && \
cd ../../fs-storage/tests && \
javac -Xlint:none -d out FileStorage.java && \
javac -Xlint:none -d out -cp out:junit.jar FileStorageTest.java && \
java -jar junit.jar --class-path out --scan-class-path)
Note: FileStorageTest.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
Thanks for using JUnit! Support its development at https://junit.org/sponsoring
╷
├─ JUnit Jupiter ✔
│ └─ FileStorageTest ✔
│ ├─ testFileStorageNeedsSyncing() ✔
│ ├─ testFileStorageMonoidCombine() ✔
│ ├─ testFileStorageAutoDelete() ✔
│ ├─ testFileStorageMainScenario() ✔
│ └─ testFileStorageWriteRead() ✔
├─ JUnit Vintage ✔
└─ JUnit Platform Suite ✔
Test run finished after 186 ms
[ 4 containers found ]
[ 0 containers skipped ]
[ 4 containers started ]
[ 0 containers aborted ]
[ 4 containers successful ]
[ 0 containers failed ]
[ 5 tests found ]
[ 0 tests skipped ]
[ 5 tests started ]
[ 0 tests aborted ]
[ 5 tests successful ]
[ 0 tests failed ]
|
There was a problem hiding this comment.
@Pushkarm029 in your last commit returning jstring from all functions is not a proper solution to handling errors.
A Rust Result<T> cannot be returned directly across jni bindings because bindings don't allow passing generics. There is no good way to communicate if the Result contains a bool, or integer or string or something else.
There are two possible options to consider -
-
Return exceptions - throw exception using
throwfunction in JNIEnv. Perhaps mplement a conversion from ArkStorageError to jni::error::Exception if it's not ergonomic by default. -
Implement a conversion to Kotlin Result type1 by creating a native java object similar to how a BTreeMap is transformed into a LinkedHashMap.
Note: that the error message in both cases will be a string version of the ArkStorageError
Footnotes
The main purpose of this point was to avoid the writing the Java wrapper code like the one implemented in However, that does not seem to be possible based on my exploration. There is no way to create Java class instances with hidden state1 on the Rust side that can then be transferred over through JNI bindings. Even looking through the documentation of jni and jnix there is no way to define a Java "class" on the Rust side. It only provides functionality to convert between Rust and Java data types and export static methods through JNI bindings. As such, I believe this PR is ready to merge. The syncing logic PR can adopt these changes as it might take more time to identify and fix the timestamp bug in it. Footnotes |
Benchmark for 70a378aClick to view benchmark
|
Benchmark for 24437c5Click to view benchmark
|
Signed-off-by: pushkarm029 <[email protected]>
Benchmark for d4baf9cClick to view benchmark
|
Signed-off-by: pushkarm029 <[email protected]>
Benchmark for 3b12ccaClick to view benchmark
|
Signed-off-by: pushkarm029 <[email protected]>
Benchmark for 21d16f0Click to view benchmark
|
|
@twitu Could you also create tracking issue for functional error handling ( And we need the sample wired to this branch, that would be the best demo. |
There was a problem hiding this comment.
@Pushkarm029 the new syncing logic has introduced a new struct for SyncStatus that must be exposed through JNI along with a couple of functions added to the BaseStorage trait. Also some of the existing tests and functions need to be updated.
/// Get [`SyncStatus`] of the storage
fn sync_status(&self) -> Result<SyncStatus>;
/// Sync the in-memory storage with the storage on disk
fn sync(&mut self) -> Result<()>;One way to expose SyncStatus is using the jnix crate which will automatically derive the conversion logic to and from java objects. Note that jnix should be feature flagged just like jni.
Kotlin style Results is scoped in a separate issue #74
Signed-off-by: pushkarm029 <[email protected]>
|
done; pls review @twitu @tareknaser |
Benchmark for a949330Click to view benchmark
|
| @@ -0,0 +1,11 @@ | |||
| /// Represents the synchronization status of the storage. | |||
| public enum SyncStatus { | |||
There was a problem hiding this comment.
It's unfortunate that we cannot avoid duplicating the SyncStatus definition. I'd hoped the jnix_macro would create a class definition as part of the exported package.
Signed-off-by: pushkarm029 <[email protected]>
Benchmark for 62c6a19Click to view benchmark
|
There was a problem hiding this comment.
I have left some comments, but I think we should merge. These comments can be addressed in #75.
| @@ -44,3 +44,15 @@ key2: value2 | |||
| ```bash | |||
| cargo run --example cli read /tmp/z | |||
| ``` | |||
|
|
|||
| ## Java Wrapper | |||
There was a problem hiding this comment.
These commands should be run inside tests/
|
|
||
| ## Steps to test Java Wrapper | ||
| ```bash | ||
| cd tests && make test |
There was a problem hiding this comment.
There is no make test
it's os dependant
| test-linux: build-rust download-test-lib java-test-linux | ||
|
|
||
| .PHONY: test-mac | ||
| test-mac: build-rust download-test-lib java-test-mac | ||
|
|
||
| .PHONY: test-windows | ||
| test-windows: build-rust download-test-lib java-test-windows |
There was a problem hiding this comment.
We can detect the OS in the Makefile to have only one make test
For reference: https://gist.github.com/sighingnow/deee806603ec9274fd47
but there may be even an easier way
| @@ -15,11 +15,16 @@ name = "cli" | |||
| log = { version = "0.4.17", features = ["release_max_level_off"] } | |||
| serde_json = "1.0.82" | |||
| serde = { version = "1.0.138", features = ["derive"] } | |||
|
|
|||
| jni = { version = "0.21.1", optional = true } | |||
| jnix = { version = "0.5.1", features = ["derive"] } | |||
There was a problem hiding this comment.
I guess jnix should be optional as well
There was a problem hiding this comment.
We should also git ignore *.h files
| ```java | ||
| javac -h . FileStorage.java | ||
| javac FileStorage.java | ||
| LD_LIBRARY_PATH=<project-root-path>/target/debug && java FileStorage.java |
There was a problem hiding this comment.
The project root path is always ../ if we run from fs-storage/
| @@ -0,0 +1,11 @@ | |||
| /// Represents the synchronization status of the storage. | |||
| public enum SyncStatus { | |||
There was a problem hiding this comment.
We should add a note in the rust code of SyncStatus that any change should be applied here as well
|
|
||
| .PHONY: download-test-lib | ||
| download-test-lib: | ||
| cd tests && \ |
There was a problem hiding this comment.
| cd tests && \ | |
| @cd tests && \ |
So it doesn't print the command to the terminal each time
|
Thanks @tareknaser for the review, i completed all these suggestions in #76 |
Add JNI bindings for FileStorage to create an instance and for all BaseStorage functions.
Once a FileStorage instance is created, it is boxed and leaked as jlong. This jlong can then be upcasted back to a FileStorage instance. All BaseStorage functions take the jlong as an argument upcast it and then perform the necessary operations.
Note: FileStorage needs to be specialized to use with JNI. After discussion, I've specialized it to
FileStorage<String, String>, however it can be easily changed.TODO:
jlongin the class instance itself rather than returning it. It will reduce the amount of Java/Kotlin wrapper code that needs to be writtenread_fs? It returns the key value pairs as a BTreeMap, however jni by default does not know how to convert BTreeMap to Java's LinkedHashMap. Do we want to support theread_fsoperation for JNI bindings? This means we'll have to implement a custom conversion logic from BTreeMap to LinkedHashMap