Skip to content
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

Raw JNI arrays #144

Merged
merged 9 commits into from
Mar 20, 2022
Merged

Raw JNI arrays #144

merged 9 commits into from
Mar 20, 2022

Conversation

ahnlabb
Copy link
Contributor

@ahnlabb ahnlabb commented Apr 25, 2021

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
Copy link
Member

mkitti commented Apr 25, 2021

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
Copy link
Member

mkitti commented Jun 16, 2021

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

@ahnlabb
Copy link
Contributor Author

ahnlabb commented Jun 17, 2021

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
Copy link
Member

mkitti commented Jun 21, 2021

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
Copy link
Contributor Author

ahnlabb commented Jun 21, 2021

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
Copy link
Member

mkitti commented Mar 20, 2022

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?

@mkitti mkitti marked this pull request as ready for review March 20, 2022 22:32
@mkitti mkitti merged commit 6405ef3 into JuliaInterop:master Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants