JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region
David Holmes
david.holmes at oracle.com
Fri Dec 8 05:40:20 UTC 2017
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 <david.holmes at oracle.com
> <mailto: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
> <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
> <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 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>>
> <mailto: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_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
>
>
>
>
More information about the hotspot-dev
mailing list