JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region

[Ian Rogers]

Thanks, fwiw Jikes RVM agrees that all critical releases end the critical
region:
https://github.com/JikesRVM/JikesRVM/blob/master/rvm/src/org/jikesrvm/jni/JNIFunctions.java#L5936

As does Cacao:
http://mips.complang.tuwien.ac.at/hg/cacao/file/2b4c9b6c245d/src/native/jni.cpp#l3208

Kaffe (with a non-moving GC) treats the release in the same way as
Release<Primitive>ArrayElements:
https://github.com/kaffe/kaffe/blob/master/kaffe/kaffevm/jni/jni-arrays.c#L404
which is the same as how it is handled on Android.

HotSpot has managed to put a foot in both camps, ending the critical always
(a la Jikes RVM and Cacao) but honoring commit behavior (and leaking) when
in -Xcheck:jni.

Thanks,
Ian

On Wed, Dec 6, 2017 at 5:25 PM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Ian,
>
> On 7/12/2017 4:48 AM, Ian Rogers wrote:
>
>> Thanks David, I think we may have differing opinions on clarity :-) In
>> the case of -Xcheck:jni and with HotSpot a copy is always made, and so
>>
>
> Ah I see. I was considering only the "real" functionality.
>
> The fact the checked version of getPrimitiveArrayCritical creates a copy
> seems to be treated as an internal detail as it does not AFAICS report that
> a copy was in fact made - the passed in isCopy pointer will be set false by
> the real getPrimitiveArrayCritical.
>
> perhaps the attached patch causes the implementation to match the spec.
>>
>
> The whole logic surrounding the management of the copy seems somewhat
> suspect to me. As you note with your patch check_wrapped_array_release
> honours the mode, which in the checked case allows, AFAICS, the internally
> made copy to "leak". So ignoring the passed in mode and always passing 0 to
> check_wrapped_array_release seems more correct.
>
> Given that commit doesn't free a copy, is it intended behavior that in
>> HotSpot it ends a critical region? ie In a VM that allowed copies you could
>> imagine, Get, Commit, Release and as spec-ed the critical region ends at
>> the commit if not a copy, and at the release if it is. Perhaps the simplest
>> thing is to remove the notion of commit here.
>>
>
> I honestly do not know what this API is really trying to achieve in that
> regard. I don't know the intended usage of "commit" mode. Why would you not
> free the copied buffer? The spec unhelpfully only states:
>
> "The other options give the programmer more control over memory management
> and should be used with extreme care."
>
> And if you don't free it what can you do with it? For Get*ArrayElements it
> clearly states
>
> "The result is valid until the corresponding Release<PrimitiveType>ArrayElements()
> function is called."
>
> so once any Release* is done, regardless of mode, it would seem the array
> (copy or not) is no longer valid and should not be accessed again. So I
> don't see any expectation that you can call Release* multiple times and so
> in terms of the critical variants I expect the critical section to end when
> Release is called - regardless of mode.
>
> David
>
> Thanks,
>> Ian
>>
>>
>> On Tue, Dec 5, 2017 at 4:37 PM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>>     Hi Ian,
>>
>>     On 6/12/2017 9:15 AM, Ian Rogers wrote:
>>
>>         From:
>>         https://docs.oracle.com/javase/8/docs/technotes/guides/jni/
>> spec/functions.html#GetPrimitiveArrayCritical_ReleasePrimiti
>> veArrayCritical
>>         <https://docs.oracle.com/javase/8/docs/technotes/guides/jni/
>> spec/functions.html#GetPrimitiveArrayCritical_ReleasePrimiti
>> veArrayCritical>
>>
>>
>>     You need to see the updated spec:
>>
>>     https://docs.oracle.com/javase/9/docs/specs/jni/functions.
>> html#getprimitivearraycritical-releaseprimitivearraycritical
>>     <https://docs.oracle.com/javase/9/docs/specs/jni/functions.
>> html#getprimitivearraycritical-releaseprimitivearraycritical>
>>
>>     That spec makes it clear:
>>
>>     "mode: the release mode (see Primitive Array Release Modes): 0,
>>     JNI_COMMIT or JNI_ABORT. Ignored if carray was a not copy."
>>
>>     In hotspot getCriticalArrayPrimitive never returns a copy so the
>>     mode is always ignored, as per the existing comment.
>>
>>     David
>>     -----
>>
>>
>>         "The semantics of these two functions are very similar to the
>>         existing
>>         Get/Release<primitivetype>ArrayElements functions. "
>>
>>         https://docs.oracle.com/javase/8/docs/technotes/guides/jni/
>> spec/functions.html#Release_PrimitiveType_ArrayElements_routines
>>         <https://docs.oracle.com/javase/8/docs/technotes/guides/jni/
>> spec/functions.html#Release_PrimitiveType_ArrayElements_routines>
>>
>>         "JNI_COMMIT copy back the content but do not free the elems
>> buffer"
>>
>>         Consider the pattern of:
>>         GetPrimitiveArrayCritical(...)
>>         ReleasePrimitiveArrayCritical(..., JNI_COMMIT)
>>         ReleasePrimitiveArrayCritical(..., 0)
>>
>>         where the first release is to just achieve a copy back. For
>> example,
>>         jniCheck.cpp will copy back but not free a copy in the case of
>>         JNI_COMMIT
>>         for a critical. The implementation of
>>         ReleasePrimitiveArrayCritical ignores
>>         all arguments and so it ends the critical region even in the
>>         event of a
>>         commit.
>>
>>         The attached patch makes ReleasePrimitiveArrayCritical consider
>>         the mode
>>         argument when releasing the GCLocker.
>>
>>         Thanks,
>>         Ian
>>
>>
>>