JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region

[David Holmes]

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_ReleasePrimitiveArrayCritical
>         <https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#GetPrimitiveArrayCritical_ReleasePrimitiveArrayCritical>
> 
> 
>     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
> 
>