JuliaInterop / JavaCall.jl

Call Java from Julia
http://juliainterop.github.io/JavaCall.jl
Other
118 stars 53 forks source link

Raw JNI arrays #144

Closed ahnlabb closed 2 years ago

ahnlabb commented 3 years ago

After the discussion in #134 I've experimented with approaches to exposing more of the JNI array functionality. Here is the draft for one such approach so that we can discuss whether this direction is of interest for JavaCall.

This would be a very lightweight approach to exposing the JNI array functionality where the user can choose to keep a returned array as a pointer and get and set elements directly through an unsafe_wrap array.

There are also some parts of JProxies that point in this direction but they are less lightweight with a different interface from the rest of JavaCall. I couldn't get them to do exactly what I want. However, if you think the JProxies approach is a better direction I'd be glad to help to surface this functionality through documentation and/or changes to JProxies.

My own immediate use for this is in BioformatsLoader where it would allow me to use the pre-allocated versions of the reader and perform the reinterpretation and reshaping of the array simultaneously as the copy from java. In general, I believe it could expand what is possible without java adapters. @mkitti has already done a lot of work in this general direction and may have ideas for alternative approaches.

One alternative to this approach would be to only wrap the array ref without actually getting the elements. This would be simpler, safer, and even more lightweight. "Direct" access to the array from Julia is pretty convenient though.

Here is an example of how the proposed functionality can be used:

julia> const JArr = @jimport java.util.Arrays
JavaObject{Symbol("java.util.Arrays")}

julia> arr = JavaCall.JNIArray{jint}(10)
10-element JavaCall.JNIArray{Int32}:
 0
 0
 0
 0
 0
 0
 0
 0
 0
 0

julia> show(arr .= 1:10)
Int32[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
julia> jcall(JArr, "toString", JString, (JavaCall.JNIArray{jint},), arr)
"[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]"

julia> jcall(JArr, "fill", jvoid, (JavaCall.JNIArray{jint},jint), arr, 5)

julia> show(arr)
Int32[5, 5, 5, 5, 5, 5, 5, 5, 5, 5]

I'm sorry if the diff is a bit noisy, I tried to consolidate some things to make it easier for the "raw" arrays to plug in. In the process, I found that the "Attempt to call method on Java NULL" error is sometimes thrown as an ErrorException that should probably be fixed independent of whether this gets merged in some form.

mkitti commented 3 years ago

This is a very neat. As we build this out, let's make sure to build out tests to make sure that we are maintaining the old functionality while also demonstrating the new capabilities

Also make sure to implement as much of the AbstractArray interface as possible: https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-array

mkitti commented 3 years ago

Hi @ahnlabb , what is the status of this PR?

ahnlabb commented 3 years ago

Hi @mkitti, this has been sitting on the shelf for a while partly because I've had to prioritize other things and partly to allow me (and anyone else) to think about the API. In its current state, it fulfills the basic AbstractArray interface. The arrays are static (I don't implement resize!) and I think keeping the basic arrays simple is good but maybe we want a higher-level API on top of them?

I've written some basic tests for the new functionality today but still not expanded on the old tests.

Do you have any thoughts on the API or how to improve testing? I'm going to try using it in BioformatsLoader tomorrow to get a feel for the ergonomics.

mkitti commented 3 years ago

I looked through this and if possible, I would like to work through this in three phases:

  1. Can you make a reproducible minimum working example that produces the "Attempt to call method on Java NULL" error? I want to make sure that we have not actually created this problem in this PR.
  2. Perhaps the changes to core.jl should be a separate pull request so we can discuss what those are. From the package maintainer perspective, changes to common code like that require considerable scrutiny. Since it sounds like you are mainly focusing on the JNIArray interface, perhaps the changes to core.jl are settled already?
  3. Focus on the core JNIArray functionality with the basic AbstractArray interface to make it work.
ahnlabb commented 3 years ago

I looked through this and if possible, I would like to work through this in three phases:

Thank you @mkitti, this makes sense to me. I have created #149 for phases 1 and 2. I think proceeding with the JNIArrays without doing something like #149 risks convoluting the codebase and might hamper future development. However, I fully agree that it warrants its own extensive discussion and (like you say) a close inspection. I hope my arguments in #149 for the need for some kind of consolidation are convincing.

mkitti commented 2 years ago

Now that we have #149 merged, I think the JNIArray seems easier to do. Let me see if I can try to resolve the conflicts.

Also, is there a reason it is called JNIArray and not JNIVector?