JNI ReleasePrimitiveArrayCritical with mode JNI_COMMIT shouldn't end critical region
irogers at google.com
Thu Dec 7 04:55:04 UTC 2017
Thanks, fwiw Jikes RVM agrees that all critical releases end the critical
As does Cacao:
Kaffe (with a non-moving GC) treats the release in the same way as
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
On Wed, Dec 6, 2017 at 5:25 PM, David Holmes <david.holmes at oracle.com>
> 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.
>> 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:
>> You need to see the updated spec:
>> 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.
>> "The semantics of these two functions are very similar to the
>> Get/Release<primitivetype>ArrayElements functions. "
>> "JNI_COMMIT copy back the content but do not free the elems
>> Consider the pattern of:
>> ReleasePrimitiveArrayCritical(..., JNI_COMMIT)
>> ReleasePrimitiveArrayCritical(..., 0)
>> where the first release is to just achieve a copy back. For
>> jniCheck.cpp will copy back but not free a copy in the case of
>> for a critical. The implementation of
>> ReleasePrimitiveArrayCritical ignores
>> all arguments and so it ends the critical region even in the
>> event of a
>> The attached patch makes ReleasePrimitiveArrayCritical consider
>> the mode
>> argument when releasing the GCLocker.
More information about the hotspot-dev