Possible memory leak in `convert_result`

[ahnlabb]

BioformatsLoader.jl has had some problems with OOM-errors. I just added a workaround using local frames (ahnlabb/BioformatsLoader.jl@4d4e2d5decd87c8bfd2bfca2fdfbc4214b120977). However, it seems like this could be a bigger issue with the result variable in convert_result, it is unclear to me when that local reference is deleted (it is not made into a JavaObject and does not have the deleteref finalizer) and the memory leak issues in BioformatsLoader.jl are fixed if an explicit JNI.DeleteLocalRef(result) is added as in ahnlabb/JavaCall.jl@94289c5c1cf60e459d0ee4fb5b8c21b6b98282d6 (potentially relevant SO question).

If you think this is indeed the correct approach I can open a PR from ahnlabb/JavaCall.jl@94289c5c1cf60e459d0ee4fb5b8c21b6b98282d6.

As an aside, BioformatsLoader could possibly benefit from increased control over the byte array life cycle and passing raw byte[] references to Java (as is discussed in #83).

[mkitti]

The local_frame function you implemented is something I've been thinking about doing in JavaCall. I was actually thinking it might be nice to implement as a macro that could be applied to a Julia function. It was part of my motivation for creating the JNI subpackage. I'm glad that you made good use of it.

My thought is that we should probably use something like local_frame within _jcall to make sure local refs created in the process of calling get cleaned up. I suspect there are many local reference leaks.

On this specific question, it seems strange that this is the only case when result should be deleted. Perhaps we should always delete result and only in occasions when we do actually need to keep one, we should create a new local reference?

Perhaps what we need is a convert_result!:

function convert_result!(rettype, result)
   converted_result = convert_result(rettype, result)
   JNI.DeleteLocalRef(result)
   return converted_result
end

By default we call convert_result! from the higher level methods. We make specific exceptions for when we should not automatically delete result.

Another question is are we sure that result is always a local reference? Could it be a global reference in some circumstances? Maybe we should use JavaRef more internally to help keep track of this.

In summary, I suspect a larger fix is needed. In the meantime, I would be willing to accept your pull request.

[mkitti]

Regarding byte[] do you have a specific proposal on how to do that? I think we should consider support for no copy sharing of array data via java.nio.Buffer. From a Julia byte array, we should make it easy to create a java.nio.Buffer via JNI.NewDirectByteBuffer. We should also make it easy to create a Julia array from a direct java.nio.Buffer via Base.unsafe_wrap.

[mkitti]

I think the above use of java.nio.Buffer could be quite powerful when combined with the support I've proposed in https://github.com/imglib/imglib2/pull/299 .

It would allow some degree of functionality like with NumPy arrays via PyCall.jl that I'm trying to expand: https://github.com/JuliaPy/PyCall.jl/pull/876

[ahnlabb]

The local_frame function you implemented is something I've been thinking about doing in JavaCall. I was actually thinking it might be nice to implement as a macro that could be applied to a Julia function. It was part of my motivation for creating the JNI subpackage. I'm glad that you made good use of it.

Yes, I was glad to see that this was exposed in a useful way, and creating convenience wrappers like local_frame around this functionality is probably a good idea.

On this specific question, it seems strange that this is the only case when result should be deleted. Perhaps we should always delete result and only in occasions when we do actually need to keep one, we should create a new local reference?

I guess it's fine when the returned ref is just wrapped in a JavaObject since these should be covered by the deleteref finalizer in JavaObject. Whenever there's any actual conversion/copying going on it seems likely that something like what you describe would be needed. Not sure how to best encode this into the logic though possibly adding the JavaObject clause from the current implementation to the convert_result! function you describe would be sufficient:

convert_result!(rettype::Type{T}, result) where {T<:JavaObject} = T(result)

From a Julia byte array, we should make it easy to create a java.nio.Buffer via JNI.NewDirectByteBuffer. We should also make it easy to create a Julia array from a direct java.nio.Buffer via Base.unsafe_wrap.

I like this idea, makes a lot of sense to me! I'll probably experiment with it a bit for the BioFormats use-case.