JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region

[Ian Rogers]

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

> On 7/12/2017 2:55 PM, Ian Rogers wrote:
>
>> 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.
>>
>
> So ... I should file a bug for the leak in the checked version and apply
> your suggested patch to always pass mode 0?
>
> Thanks,
> David
>

Sounds good to me. I would have filed the bug rather than starting this
thread if I had a user/bug account :-)

Thanks,
Ian


> Thanks,
>> Ian
>>
>>
>> On Wed, Dec 6, 2017 at 5:25 PM, David Holmes <david.holmes at oracle.com
>> <mailto: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>
>>         <mailto: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>
>>                         <https://docs.oracle.com/javas
>> e/8/docs/technotes/guides/jni/spec/functions.html#GetPrimiti
>> veArrayCritical_ReleasePrimitiveArrayCritical
>>         <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>
>>                     <https://docs.oracle.com/javas
>> e/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>
>>                         <https://docs.oracle.com/javas
>> e/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
>>
>>
>>
>>