JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region

Ian Rogers
From:
https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#GetPrimitiveArrayCritical_ReleasePrimitiveArrayCritical

"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

"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

hotspot.jni-release-critical-commit.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region

David Holmes
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

You need to see the updated spec:

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
>
> "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
>
Reply | Threaded
Open this post in threaded view
|

Re: JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region

Ian Rogers
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 perhaps
the attached patch causes the implementation to match the spec. 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.

Thanks,
Ian

On Tue, Dec 5, 2017 at 4:37 PM, David Holmes <[hidden email]>
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
>>
>
> You need to see the updated spec:
>
> 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
>>
>> "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
>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: 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 <[hidden email]
> <mailto:[hidden email]>> 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
>
>
Reply | Threaded
Open this post in threaded view
|

Re: 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 <[hidden email]>
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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region

David Holmes
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


> Thanks,
> Ian
>
> On Wed, Dec 6, 2017 at 5:25 PM, David Holmes <[hidden email]
> <mailto:[hidden email]>> 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
>         <[hidden email] <mailto:[hidden email]>
>         <mailto:[hidden email]
>         <mailto:[hidden email]>>> 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>
>                
>         <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>
>            
>         <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>
>                
>         <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
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region

Ian Rogers
On Wed, Dec 6, 2017 at 9:00 PM, David Holmes <[hidden email]>
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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>         <[hidden email] <mailto:[hidden email]>
>>         <mailto:[hidden email]
>>
>>         <mailto:[hidden email]>>> 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
>>
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region

David Holmes
Filed: https://bugs.openjdk.java.net/browse/JDK-8193234

David

On 8/12/2017 2:58 AM, Ian Rogers wrote:

> On Wed, Dec 6, 2017 at 9:00 PM, David Holmes <[hidden email]
> <mailto:[hidden email]>> 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
>         <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
>         <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
>         <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
>         <[hidden email] <mailto:[hidden email]>
>         <mailto:[hidden email]
>         <mailto:[hidden email]>>> 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
>                  <[hidden email]
>         <mailto:[hidden email]> <mailto:[hidden email]
>         <mailto:[hidden email]>>
>                  <mailto:[hidden email]
>         <mailto:[hidden email]>
>
>                  <mailto:[hidden email]
>         <mailto:[hidden email]>>>> 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>
>                
>         <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>>
>                                
>         <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>
>                
>         <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>
>                
>         <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/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/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>
>                
>         <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/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/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
>
>
>
>