8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

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

8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Mikael Gerdin
Hi all,
Please review this revised change to jweak to support G1.

I've copied Kim's original description of the fix below for reference:

>
> The problem being addressed is that, when using G1, dereferencing a
> jweak must ensure the obtained referent is ensured live, just as for
> other weak reference types.  This has always been a requirement, and
> failure to do so appears to be a since-day1 G1 bug.
>
> There are two categories of places that need to address this issue:
> JNIHandles::resolve and friends, and returning a jobject from native
> code to Java.  In both of these places the jobject whose contained oop
> is being extracted might really be a jweak.  jweak is
> representationally indistinguishable from jobject; jweak is just a
> typedef for jobject.  The documentation says a jweak can be used
> anywhere a jobject is permitted, making that type equivalence
> necessary for a C API.  (A C++ API might be able to have distinct
> types via inheritance, though would still need a method for
> distinguishing them at runtime.  But since JNI is a C API...).
>
> The basic approach being taken is to "tag" jweaks by setting the low
> order bit (recall that jweak == jobject == _jobject*), in order to
> distinguish them from non-jweak jobjects.  This tagging is only being
> done when the selected GC needs the distinction, e.g. G1.  This gives
> us a representational difference between jobject and jweak on which to
> base the different behavior, hiding that distinction behind the
> existing API.
>
> JNIHandles::resolve and friends are changed to unconditionally strip
> off any potential weak tag when translating from jobject to to oop*.
> That's cheaper than testing for the conditional use of tagging and
> then stripping.  Also stripped when deleting JNI handles.
>
> TemplateInterpreterGenerator::generate_native_entry and
> SharedRuntime::generate_native_wrapper are changed to conditionally
> emit code to apply the G1 pre-barrier to the value obtained from a
> jobject result, when the result is tagged as a jweak, which only
> occurs when using G1.
>
> For arm32/arm64, this required moving the g1_write_barrier_pre
> definitions from InterpreterMacroAssembler to MacroAssembler.  Also
> moved g1_write_barrier_post, for consistency.
>

In addition to Kim's final version of the change (which was backed out)
this updated version adds code to mask out the weak tag bit in the fast
JNI field getters on all platforms where fast JNI getters are used.

Since the fast JNI getters only read primitive values from objects me
and Kim decided that there was no need to invoke the G1 pre-barrier if a
jweak was used as the "receiver" for the getters, this simplifies the
change to the fast getters and I was able to come up with a seemingly
efficient instruction encoding on all platforms.

A couple of comments about the implementations:
My goal was to encode clearing of the weak_tag_mask (which at the moment
is 1).
On SPARC and 32 bit arm there is an instruction that can be used
straight off to perform an and with a value that is first negated, ANDN
on SPARC and BTC on ARM.
On 64 bit ARM the encoding for immediates allows encoding of the bitwise
inverse of 1 (0xfffffffffffffffe) but if weak_tag_mask is changed the
encoding may fail.
On x86 I pass the mask as a signed 32 bit immediate but note that the
assembler encodes it as an 8 bit immediate and sets a sign-extension bit
in the opcode.


Testing:
Standard JPRT and tier2-3 HotSpot tests.
I've modified the CallWithJNIWeak test to call all the primitive getters
and some other interesting cases I came up with.
I've also run the CallWithJNIWeak test on all platforms on both product
and fastdebug builds (since fast JNI getters are implicitly disabled in
debug builds due to VerifyJNIFields being enabled by default in debug
builds.

I've not built or tested the aarch64 port but I think it's correct and I
hope someone can test it for me.


Webrevs:
Kim's initial change:
http://cr.openjdk.java.net/~mgerdin/8175085/webrev.kim/webrev/
My additions:
http://cr.openjdk.java.net/~mgerdin/8175085/webrev.mg/webrev/
Full webrev:
http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/

Bugs:
https://bugs.openjdk.java.net/browse/JDK-8175085
https://bugs.openjdk.java.net/browse/JDK-8166188

Thanks
/Mikael
Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Kim Barrett
> On Feb 22, 2017, at 11:07 AM, Mikael Gerdin <[hidden email]> wrote:
>
> Hi all,
> Please review this revised change to jweak to support G1.
>
> I've copied Kim's original description of the fix below for reference:
>
>>
>> The problem being addressed is that, when using G1, dereferencing a
>> jweak must ensure the obtained referent is ensured live, just as for
>> other weak reference types.  This has always been a requirement, and
>> failure to do so appears to be a since-day1 G1 bug.
>>
>> There are two categories of places that need to address this issue:
>> JNIHandles::resolve and friends, and returning a jobject from native
>> code to Java.  In both of these places the jobject whose contained oop
>> is being extracted might really be a jweak.  jweak is
>> representationally indistinguishable from jobject; jweak is just a
>> typedef for jobject.  The documentation says a jweak can be used
>> anywhere a jobject is permitted, making that type equivalence
>> necessary for a C API.  (A C++ API might be able to have distinct
>> types via inheritance, though would still need a method for
>> distinguishing them at runtime.  But since JNI is a C API...).
>>
>> The basic approach being taken is to "tag" jweaks by setting the low
>> order bit (recall that jweak == jobject == _jobject*), in order to
>> distinguish them from non-jweak jobjects.  This tagging is only being
>> done when the selected GC needs the distinction, e.g. G1.  This gives
>> us a representational difference between jobject and jweak on which to
>> base the different behavior, hiding that distinction behind the
>> existing API.
>>
>> JNIHandles::resolve and friends are changed to unconditionally strip
>> off any potential weak tag when translating from jobject to to oop*.
>> That's cheaper than testing for the conditional use of tagging and
>> then stripping.  Also stripped when deleting JNI handles.
>>
>> TemplateInterpreterGenerator::generate_native_entry and
>> SharedRuntime::generate_native_wrapper are changed to conditionally
>> emit code to apply the G1 pre-barrier to the value obtained from a
>> jobject result, when the result is tagged as a jweak, which only
>> occurs when using G1.
>>
>> For arm32/arm64, this required moving the g1_write_barrier_pre
>> definitions from InterpreterMacroAssembler to MacroAssembler.  Also
>> moved g1_write_barrier_post, for consistency.
>>
>
> In addition to Kim's final version of the change (which was backed out) this updated version adds code to mask out the weak tag bit in the fast JNI field getters on all platforms where fast JNI getters are used.
>
> […]
>
> Webrevs:
> Kim's initial change:
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.kim/webrev/
> My additions:
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.mg/webrev/
> Full webrev:
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/
>
> Bugs:
> https://bugs.openjdk.java.net/browse/JDK-8175085
> https://bugs.openjdk.java.net/browse/JDK-8166188
>
> Thanks
> /Mikael

Mikael - Thanks for taking this up for me.  Just a few minor comments.

------------------------------------------------------------------------------  
The usual copyright reminder.

------------------------------------------------------------------------------  
src/cpu/arm/vm/jniFastGetField_arm.cpp
 122 #ifndef AARCH64
 123   __ bic(R1, R1, JNIHandles::weak_tag_mask);
 124 #else
 125   __ andr(R1, R1, ~JNIHandles::weak_tag_mask);
 126 #endif

Usual style when selecting between AARCH64 and not seems to be to put
the AARCH64 code first, e.g.

#ifdef AARCH64
  ...
#else
  ...
#endif

------------------------------------------------------------------------------
src/cpu/x86/vm/jniFastGetField_x86_32.cpp
  90   assert(inverted_jweak_mask == -2, "Otherwise check this code");

Use STATIC_ASSERT rather than assert.

------------------------------------------------------------------------------
src/cpu/x86/vm/jniFastGetField_x86_32.cpp
  89   const intptr_t inverted_jweak_mask = ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
  90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
  91   __ andl(rdx, inverted_jweak_mask); // mask is subject to sign-extension


With three occurrences of this snippet in this file and two more
similar ones in the 64-bit file, a macroAssembler helper seems called
for here.

------------------------------------------------------------------------------
src/cpu/x86/vm/jniFastGetField_x86_64.cpp
  84   const intptr_t inverted_jweak_mask = ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
  85   const int32_t truncated_mask = static_cast<int32_t>(inverted_jweak_mask);

Why the two-step conversion here?  Why not just

  const int32_t truncated_mask = static_cast<int32_t>(JNIHandles::weak_tag_mask);

That would also make the 32 and 64-bit code more similar in the
suggested macroAssembler helper.

------------------------------------------------------------------------------
test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
  51 #define CHECK(variable, expected)                                 \
  52   do { if ((variable) != (expected)) {                            \
  53     (*env)->ThrowNew(env, exception,  #variable" != " #expected); \
  54     return;                                                       \
  55   }                                                               \
  56 } while(0);

The formatting of the code in the macro body is strange and confusing.

Also the do-while-zero idiom that I'm used to leaves off the final
semicolon in the macro.  Instead of the uses are expected to be
terminated with semicolons, as you've done.

------------------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Mikael Gerdin
Hi Kim,

Thanks for the prompt review!

On 2017-02-22 20:40, Kim Barrett wrote:

>> On Feb 22, 2017, at 11:07 AM, Mikael Gerdin <[hidden email]> wrote:
>>
>> Hi all,
>> Please review this revised change to jweak to support G1.
>>
>> I've copied Kim's original description of the fix below for reference:
>>
>>>
>>> The problem being addressed is that, when using G1, dereferencing a
>>> jweak must ensure the obtained referent is ensured live, just as for
>>> other weak reference types.  This has always been a requirement, and
>>> failure to do so appears to be a since-day1 G1 bug.
>>>
>>> There are two categories of places that need to address this issue:
>>> JNIHandles::resolve and friends, and returning a jobject from native
>>> code to Java.  In both of these places the jobject whose contained oop
>>> is being extracted might really be a jweak.  jweak is
>>> representationally indistinguishable from jobject; jweak is just a
>>> typedef for jobject.  The documentation says a jweak can be used
>>> anywhere a jobject is permitted, making that type equivalence
>>> necessary for a C API.  (A C++ API might be able to have distinct
>>> types via inheritance, though would still need a method for
>>> distinguishing them at runtime.  But since JNI is a C API...).
>>>
>>> The basic approach being taken is to "tag" jweaks by setting the low
>>> order bit (recall that jweak == jobject == _jobject*), in order to
>>> distinguish them from non-jweak jobjects.  This tagging is only being
>>> done when the selected GC needs the distinction, e.g. G1.  This gives
>>> us a representational difference between jobject and jweak on which to
>>> base the different behavior, hiding that distinction behind the
>>> existing API.
>>>
>>> JNIHandles::resolve and friends are changed to unconditionally strip
>>> off any potential weak tag when translating from jobject to to oop*.
>>> That's cheaper than testing for the conditional use of tagging and
>>> then stripping.  Also stripped when deleting JNI handles.
>>>
>>> TemplateInterpreterGenerator::generate_native_entry and
>>> SharedRuntime::generate_native_wrapper are changed to conditionally
>>> emit code to apply the G1 pre-barrier to the value obtained from a
>>> jobject result, when the result is tagged as a jweak, which only
>>> occurs when using G1.
>>>
>>> For arm32/arm64, this required moving the g1_write_barrier_pre
>>> definitions from InterpreterMacroAssembler to MacroAssembler.  Also
>>> moved g1_write_barrier_post, for consistency.
>>>
>>
>> In addition to Kim's final version of the change (which was backed out) this updated version adds code to mask out the weak tag bit in the fast JNI field getters on all platforms where fast JNI getters are used.
>>
>> […]
>>
>> Webrevs:
>> Kim's initial change:
>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.kim/webrev/
>> My additions:
>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.mg/webrev/
>> Full webrev:
>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/
>>
>> Bugs:
>> https://bugs.openjdk.java.net/browse/JDK-8175085
>> https://bugs.openjdk.java.net/browse/JDK-8166188
>>
>> Thanks
>> /Mikael
>
> Mikael - Thanks for taking this up for me.  Just a few minor comments.
>
> ------------------------------------------------------------------------------
> The usual copyright reminder.

Updated copyrights.

>
> ------------------------------------------------------------------------------
> src/cpu/arm/vm/jniFastGetField_arm.cpp
>  122 #ifndef AARCH64
>  123   __ bic(R1, R1, JNIHandles::weak_tag_mask);
>  124 #else
>  125   __ andr(R1, R1, ~JNIHandles::weak_tag_mask);
>  126 #endif
>
> Usual style when selecting between AARCH64 and not seems to be to put
> the AARCH64 code first, e.g.
>
> #ifdef AARCH64
>   ...
> #else
>   ...
> #endif

Fixed.

>
> ------------------------------------------------------------------------------
> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>
> Use STATIC_ASSERT rather than assert.
>
> ------------------------------------------------------------------------------
> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>   89   const intptr_t inverted_jweak_mask = ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>   91   __ andl(rdx, inverted_jweak_mask); // mask is subject to sign-extension
>
>
> With three occurrences of this snippet in this file and two more
> similar ones in the 64-bit file, a macroAssembler helper seems called
> for here.
>
> ------------------------------------------------------------------------------
> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>   84   const intptr_t inverted_jweak_mask = ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>   85   const int32_t truncated_mask = static_cast<int32_t>(inverted_jweak_mask);
>
> Why the two-step conversion here?  Why not just
>
>   const int32_t truncated_mask = static_cast<int32_t>(JNIHandles::weak_tag_mask);

I had changed this a bunch of times since I was a bit unsure about how
to best do this but when I decided to do the bitwise invert after the
cast to signed I forgot to clean up the casts.

>
> That would also make the 32 and 64-bit code more similar in the
> suggested macroAssembler helper.

Indeed, I even found MacroAssembler::andptr which makes this a nice and
small helper.

>
> ------------------------------------------------------------------------------
> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>   51 #define CHECK(variable, expected)                                 \
>   52   do { if ((variable) != (expected)) {                            \
>   53     (*env)->ThrowNew(env, exception,  #variable" != " #expected); \
>   54     return;                                                       \
>   55   }                                                               \
>   56 } while(0);
>
> The formatting of the code in the macro body is strange and confusing.

Cleaned up formatting.

>
> Also the do-while-zero idiom that I'm used to leaves off the final
> semicolon in the macro.  Instead of the uses are expected to be
> terminated with semicolons, as you've done.

Oh, oops.

I also took the liberty to add STATIC_ASSERTS on 64 bit arm since the
encoding of the immediate in the "andr" instruction is a bit "funny".
The assembler code only verifies that immediate encoding works in debug
builds but in debug builds the fast JNI getters aren't generated.

New webrevs:
Incremental:
http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental/webrev/
Full:
http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full2/webrev/

Thanks
/Mikael

>
> ------------------------------------------------------------------------------
>
Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Kim Barrett
> On Feb 23, 2017, at 8:29 AM, Mikael Gerdin <[hidden email]> wrote:[…]
>
> New webrevs:
> Incremental:
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental/webrev/
> Full:
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full2/webrev/

looks good

Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Mikael Gerdin


On 2017-02-23 15:50, Kim Barrett wrote:

>> On Feb 23, 2017, at 8:29 AM, Mikael Gerdin <[hidden email]> wrote:[…]
>>
>> New webrevs:
>> Incremental:
>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental/webrev/
>> Full:
>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full2/webrev/
>
> looks good
>

Thanks for the review, Kim!

/Mikael
Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Daniel D. Daugherty
In reply to this post by Mikael Gerdin
On 2/23/17 6:29 AM, Mikael Gerdin wrote:

> Hi Kim,
>
> Thanks for the prompt review!
>
> On 2017-02-22 20:40, Kim Barrett wrote:
>>> On Feb 22, 2017, at 11:07 AM, Mikael Gerdin
>>> <[hidden email]> wrote:
>>>
>>> Hi all,
>>> Please review this revised change to jweak to support G1.
>>>
>>> I've copied Kim's original description of the fix below for reference:
>>>
>>>>
>>>> The problem being addressed is that, when using G1, dereferencing a
>>>> jweak must ensure the obtained referent is ensured live, just as for
>>>> other weak reference types.  This has always been a requirement, and
>>>> failure to do so appears to be a since-day1 G1 bug.
>>>>
>>>> There are two categories of places that need to address this issue:
>>>> JNIHandles::resolve and friends, and returning a jobject from native
>>>> code to Java.  In both of these places the jobject whose contained oop
>>>> is being extracted might really be a jweak.  jweak is
>>>> representationally indistinguishable from jobject; jweak is just a
>>>> typedef for jobject.  The documentation says a jweak can be used
>>>> anywhere a jobject is permitted, making that type equivalence
>>>> necessary for a C API.  (A C++ API might be able to have distinct
>>>> types via inheritance, though would still need a method for
>>>> distinguishing them at runtime.  But since JNI is a C API...).
>>>>
>>>> The basic approach being taken is to "tag" jweaks by setting the low
>>>> order bit (recall that jweak == jobject == _jobject*), in order to
>>>> distinguish them from non-jweak jobjects.  This tagging is only being
>>>> done when the selected GC needs the distinction, e.g. G1. This gives
>>>> us a representational difference between jobject and jweak on which to
>>>> base the different behavior, hiding that distinction behind the
>>>> existing API.
>>>>
>>>> JNIHandles::resolve and friends are changed to unconditionally strip
>>>> off any potential weak tag when translating from jobject to to oop*.
>>>> That's cheaper than testing for the conditional use of tagging and
>>>> then stripping.  Also stripped when deleting JNI handles.
>>>>
>>>> TemplateInterpreterGenerator::generate_native_entry and
>>>> SharedRuntime::generate_native_wrapper are changed to conditionally
>>>> emit code to apply the G1 pre-barrier to the value obtained from a
>>>> jobject result, when the result is tagged as a jweak, which only
>>>> occurs when using G1.
>>>>
>>>> For arm32/arm64, this required moving the g1_write_barrier_pre
>>>> definitions from InterpreterMacroAssembler to MacroAssembler.  Also
>>>> moved g1_write_barrier_post, for consistency.
>>>>
>>>
>>> In addition to Kim's final version of the change (which was backed
>>> out) this updated version adds code to mask out the weak tag bit in
>>> the fast JNI field getters on all platforms where fast JNI getters
>>> are used.
>>>
>>> […]
>>>
>>> Webrevs:
>>> Kim's initial change:
>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.kim/webrev/
>>> My additions:
>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.mg/webrev/
>>> Full webrev:
>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/
>>>
>>> Bugs:
>>> https://bugs.openjdk.java.net/browse/JDK-8175085
>>> https://bugs.openjdk.java.net/browse/JDK-8166188
>>>
>>> Thanks
>>> /Mikael
>>
>> Mikael - Thanks for taking this up for me.  Just a few minor comments.
>>
>> ------------------------------------------------------------------------------
>>
>> The usual copyright reminder.
>
> Updated copyrights.
>
>>
>> ------------------------------------------------------------------------------
>>
>> src/cpu/arm/vm/jniFastGetField_arm.cpp
>>  122 #ifndef AARCH64
>>  123   __ bic(R1, R1, JNIHandles::weak_tag_mask);
>>  124 #else
>>  125   __ andr(R1, R1, ~JNIHandles::weak_tag_mask);
>>  126 #endif
>>
>> Usual style when selecting between AARCH64 and not seems to be to put
>> the AARCH64 code first, e.g.
>>
>> #ifdef AARCH64
>>   ...
>> #else
>>   ...
>> #endif
>
> Fixed.
>
>>
>> ------------------------------------------------------------------------------
>>
>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>
>> Use STATIC_ASSERT rather than assert.
>>
>> ------------------------------------------------------------------------------
>>
>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>   89   const intptr_t inverted_jweak_mask =
>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>   91   __ andl(rdx, inverted_jweak_mask); // mask is subject to
>> sign-extension
>>
>>
>> With three occurrences of this snippet in this file and two more
>> similar ones in the 64-bit file, a macroAssembler helper seems called
>> for here.
>>
>> ------------------------------------------------------------------------------
>>
>> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>>   84   const intptr_t inverted_jweak_mask =
>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>   85   const int32_t truncated_mask =
>> static_cast<int32_t>(inverted_jweak_mask);
>>
>> Why the two-step conversion here?  Why not just
>>
>>   const int32_t truncated_mask =
>> static_cast<int32_t>(JNIHandles::weak_tag_mask);
>
> I had changed this a bunch of times since I was a bit unsure about how
> to best do this but when I decided to do the bitwise invert after the
> cast to signed I forgot to clean up the casts.
>
>>
>> That would also make the 32 and 64-bit code more similar in the
>> suggested macroAssembler helper.
>
> Indeed, I even found MacroAssembler::andptr which makes this a nice
> and small helper.
>
>>
>> ------------------------------------------------------------------------------
>>
>> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>>   51 #define CHECK(variable, expected)                                 \
>>   52   do { if ((variable) != (expected)) {                            \
>>   53     (*env)->ThrowNew(env, exception,  #variable" != " #expected); \
>>   54 return;                                                       \
>>   55 } \
>>   56 } while(0);
>>
>> The formatting of the code in the macro body is strange and confusing.
>
> Cleaned up formatting.
>
>>
>> Also the do-while-zero idiom that I'm used to leaves off the final
>> semicolon in the macro.  Instead of the uses are expected to be
>> terminated with semicolons, as you've done.
>
> Oh, oops.
>
> I also took the liberty to add STATIC_ASSERTS on 64 bit arm since the
> encoding of the immediate in the "andr" instruction is a bit "funny".
> The assembler code only verifies that immediate encoding works in
> debug builds but in debug builds the fast JNI getters aren't generated.
>
> New webrevs:
> Incremental:
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental/webrev/
> Full:
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full2/webrev/

I only took a look at these files from the full webrev. These are the
files touch in the two incremental webrevs:

src/cpu/aarch64/vm/jniFastGetField_aarch64.cpp
     No comments.

src/cpu/arm/vm/jniFastGetField_arm.cpp
     No comments.

src/cpu/sparc/vm/jniFastGetField_sparc.cpp
     No comments.

src/cpu/x86/vm/jniFastGetField_x86_32.cpp
     No comments.

src/cpu/x86/vm/jniFastGetField_x86_64.cpp
     No comments.

src/cpu/x86/vm/macroAssembler_x86.cpp
     I like the new helper!

src/cpu/x86/vm/macroAssembler_x86.hpp
     No comments.

test/runtime/jni/CallWithJNIWeak/CallWithJNIWeak.java
     L50 - any reason for the blank line?

test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
     L32 : Java_CallWithJNIWeak_testJNIFieldAccessors (JNIEnv *env,
jclass clazz, jobject this) {
     L102: Java_CallWithJNIWeak_runTests (JNIEnv *env, jclass clazz,
jobject this) {
     L137: Java_CallWithJNIWeak_weakReceiverTest0 (JNIEnv *env, jobject
obj) {
         Please delete the space before the first '('.

You may want to add a comment between the two halves of
the test for the literal values that you're setting in
the .java file and testing for in the .c file.

In your (Mikael G) original invite:

 > Testing:
 > Standard JPRT and tier2-3 HotSpot tests.

Does tier2 and/or tier3 cover the tests that failed in Mach5?


 > I've modified the CallWithJNIWeak test to call all the primitive
 > getters and some other interesting cases I came up with.
 > I've also run the CallWithJNIWeak test on all platforms on both
 > product and fastdebug builds (since fast JNI getters are implicitly
 > disabled in debug builds due to VerifyJNIFields being enabled by
 > default in debug builds.

Great coverage with the new tests!


 > I've not built or tested the aarch64 port but I think it's correct
 > and I hope someone can test it for me.

Have you heard back from anyone on aarch64? I haven't
seen any e-mail on this OpenJDK thread...

Thumbs up!

Dan


>
> Thanks
> /Mikael
>
>>
>> ------------------------------------------------------------------------------
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Mikael Gerdin
Hi Dan,

Thanks for looking at this review!

On 2017-02-23 17:18, Daniel D. Daugherty wrote:

> On 2/23/17 6:29 AM, Mikael Gerdin wrote:
>> Hi Kim,
>>
>> Thanks for the prompt review!
>>
>> On 2017-02-22 20:40, Kim Barrett wrote:
>>>> On Feb 22, 2017, at 11:07 AM, Mikael Gerdin
>>>> <[hidden email]> wrote:
>>>>
>>>> Hi all,
>>>> Please review this revised change to jweak to support G1.
>>>>
>>>> I've copied Kim's original description of the fix below for reference:
>>>>
>>>>>
>>>>> The problem being addressed is that, when using G1, dereferencing a
>>>>> jweak must ensure the obtained referent is ensured live, just as for
>>>>> other weak reference types.  This has always been a requirement, and
>>>>> failure to do so appears to be a since-day1 G1 bug.
>>>>>
>>>>> There are two categories of places that need to address this issue:
>>>>> JNIHandles::resolve and friends, and returning a jobject from native
>>>>> code to Java.  In both of these places the jobject whose contained oop
>>>>> is being extracted might really be a jweak.  jweak is
>>>>> representationally indistinguishable from jobject; jweak is just a
>>>>> typedef for jobject.  The documentation says a jweak can be used
>>>>> anywhere a jobject is permitted, making that type equivalence
>>>>> necessary for a C API.  (A C++ API might be able to have distinct
>>>>> types via inheritance, though would still need a method for
>>>>> distinguishing them at runtime.  But since JNI is a C API...).
>>>>>
>>>>> The basic approach being taken is to "tag" jweaks by setting the low
>>>>> order bit (recall that jweak == jobject == _jobject*), in order to
>>>>> distinguish them from non-jweak jobjects.  This tagging is only being
>>>>> done when the selected GC needs the distinction, e.g. G1. This gives
>>>>> us a representational difference between jobject and jweak on which to
>>>>> base the different behavior, hiding that distinction behind the
>>>>> existing API.
>>>>>
>>>>> JNIHandles::resolve and friends are changed to unconditionally strip
>>>>> off any potential weak tag when translating from jobject to to oop*.
>>>>> That's cheaper than testing for the conditional use of tagging and
>>>>> then stripping.  Also stripped when deleting JNI handles.
>>>>>
>>>>> TemplateInterpreterGenerator::generate_native_entry and
>>>>> SharedRuntime::generate_native_wrapper are changed to conditionally
>>>>> emit code to apply the G1 pre-barrier to the value obtained from a
>>>>> jobject result, when the result is tagged as a jweak, which only
>>>>> occurs when using G1.
>>>>>
>>>>> For arm32/arm64, this required moving the g1_write_barrier_pre
>>>>> definitions from InterpreterMacroAssembler to MacroAssembler.  Also
>>>>> moved g1_write_barrier_post, for consistency.
>>>>>
>>>>
>>>> In addition to Kim's final version of the change (which was backed
>>>> out) this updated version adds code to mask out the weak tag bit in
>>>> the fast JNI field getters on all platforms where fast JNI getters
>>>> are used.
>>>>
>>>> […]
>>>>
>>>> Webrevs:
>>>> Kim's initial change:
>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.kim/webrev/
>>>> My additions:
>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.mg/webrev/
>>>> Full webrev:
>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/
>>>>
>>>> Bugs:
>>>> https://bugs.openjdk.java.net/browse/JDK-8175085
>>>> https://bugs.openjdk.java.net/browse/JDK-8166188
>>>>
>>>> Thanks
>>>> /Mikael
>>>
>>> Mikael - Thanks for taking this up for me.  Just a few minor comments.
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> The usual copyright reminder.
>>
>> Updated copyrights.
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/cpu/arm/vm/jniFastGetField_arm.cpp
>>>  122 #ifndef AARCH64
>>>  123   __ bic(R1, R1, JNIHandles::weak_tag_mask);
>>>  124 #else
>>>  125   __ andr(R1, R1, ~JNIHandles::weak_tag_mask);
>>>  126 #endif
>>>
>>> Usual style when selecting between AARCH64 and not seems to be to put
>>> the AARCH64 code first, e.g.
>>>
>>> #ifdef AARCH64
>>>   ...
>>> #else
>>>   ...
>>> #endif
>>
>> Fixed.
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>>
>>> Use STATIC_ASSERT rather than assert.
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>>   89   const intptr_t inverted_jweak_mask =
>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>>   91   __ andl(rdx, inverted_jweak_mask); // mask is subject to
>>> sign-extension
>>>
>>>
>>> With three occurrences of this snippet in this file and two more
>>> similar ones in the 64-bit file, a macroAssembler helper seems called
>>> for here.
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>>>   84   const intptr_t inverted_jweak_mask =
>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>>   85   const int32_t truncated_mask =
>>> static_cast<int32_t>(inverted_jweak_mask);
>>>
>>> Why the two-step conversion here?  Why not just
>>>
>>>   const int32_t truncated_mask =
>>> static_cast<int32_t>(JNIHandles::weak_tag_mask);
>>
>> I had changed this a bunch of times since I was a bit unsure about how
>> to best do this but when I decided to do the bitwise invert after the
>> cast to signed I forgot to clean up the casts.
>>
>>>
>>> That would also make the 32 and 64-bit code more similar in the
>>> suggested macroAssembler helper.
>>
>> Indeed, I even found MacroAssembler::andptr which makes this a nice
>> and small helper.
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>>>   51 #define CHECK(variable, expected)                                 \
>>>   52   do { if ((variable) != (expected)) {                            \
>>>   53     (*env)->ThrowNew(env, exception,  #variable" != " #expected); \
>>>   54 return;                                                       \
>>>   55 } \
>>>   56 } while(0);
>>>
>>> The formatting of the code in the macro body is strange and confusing.
>>
>> Cleaned up formatting.
>>
>>>
>>> Also the do-while-zero idiom that I'm used to leaves off the final
>>> semicolon in the macro.  Instead of the uses are expected to be
>>> terminated with semicolons, as you've done.
>>
>> Oh, oops.
>>
>> I also took the liberty to add STATIC_ASSERTS on 64 bit arm since the
>> encoding of the immediate in the "andr" instruction is a bit "funny".
>> The assembler code only verifies that immediate encoding works in
>> debug builds but in debug builds the fast JNI getters aren't generated.
>>
>> New webrevs:
>> Incremental:
>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental/webrev/
>> Full:
>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full2/webrev/
>
> I only took a look at these files from the full webrev. These are the
> files touch in the two incremental webrevs:
>
> src/cpu/aarch64/vm/jniFastGetField_aarch64.cpp
>     No comments.
>
> src/cpu/arm/vm/jniFastGetField_arm.cpp
>     No comments.
>
> src/cpu/sparc/vm/jniFastGetField_sparc.cpp
>     No comments.
>
> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>     No comments.
>
> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>     No comments.
>
> src/cpu/x86/vm/macroAssembler_x86.cpp
>     I like the new helper!
>
> src/cpu/x86/vm/macroAssembler_x86.hpp
>     No comments.
>
> test/runtime/jni/CallWithJNIWeak/CallWithJNIWeak.java
>     L50 - any reason for the blank line?

No reason whatsoever, I'll remove it.

>
> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>     L32 : Java_CallWithJNIWeak_testJNIFieldAccessors (JNIEnv *env,
> jclass clazz, jobject this) {
>     L102: Java_CallWithJNIWeak_runTests (JNIEnv *env, jclass clazz,
> jobject this) {
>     L137: Java_CallWithJNIWeak_weakReceiverTest0 (JNIEnv *env, jobject
> obj) {
>         Please delete the space before the first '('.

Oh, oops. I'm going to blame javah for that :)

>
> You may want to add a comment between the two halves of
> the test for the literal values that you're setting in
> the .java file and testing for in the .c file.

Added short comments in the .java and .c files.

>
> In your (Mikael G) original invite:
>
>> Testing:
>> Standard JPRT and tier2-3 HotSpot tests.
>
> Does tier2 and/or tier3 cover the tests that failed in Mach5?

Somehow I forgot to mention that I also ran jdk_tier3 and the tests that
failed in Mach5 did pass on all platforms.

>
>
>> I've modified the CallWithJNIWeak test to call all the primitive
>> getters and some other interesting cases I came up with.
>> I've also run the CallWithJNIWeak test on all platforms on both
>> product and fastdebug builds (since fast JNI getters are implicitly
>> disabled in debug builds due to VerifyJNIFields being enabled by
>> default in debug builds.
>
> Great coverage with the new tests!
>
>
>> I've not built or tested the aarch64 port but I think it's correct
>> and I hope someone can test it for me.
>
> Have you heard back from anyone on aarch64? I haven't
> seen any e-mail on this OpenJDK thread...

I've cc:ed Andrew in this thread to see if I can catch his attention.

New webrevs at:
http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full3/webrev/
http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental2/webrev/

Thanks
/Mikael

>
> Thumbs up!
>
> Dan
>
>
>>
>> Thanks
>> /Mikael
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Volker Simonis
Hi Mikael,

I had a little problems to apply the patch from your webrev because
the file actually contains two patches [1] but after fixing that
manually, I could still build and run the test on ppc64 and s390x.

Thanks,
Volker

[1] http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/webrev/hotspot.changeset

On Fri, Feb 24, 2017 at 10:55 AM, Mikael Gerdin
<[hidden email]> wrote:

> Hi Dan,
>
> Thanks for looking at this review!
>
>
> On 2017-02-23 17:18, Daniel D. Daugherty wrote:
>>
>> On 2/23/17 6:29 AM, Mikael Gerdin wrote:
>>>
>>> Hi Kim,
>>>
>>> Thanks for the prompt review!
>>>
>>> On 2017-02-22 20:40, Kim Barrett wrote:
>>>>>
>>>>> On Feb 22, 2017, at 11:07 AM, Mikael Gerdin
>>>>> <[hidden email]> wrote:
>>>>>
>>>>> Hi all,
>>>>> Please review this revised change to jweak to support G1.
>>>>>
>>>>> I've copied Kim's original description of the fix below for reference:
>>>>>
>>>>>>
>>>>>> The problem being addressed is that, when using G1, dereferencing a
>>>>>> jweak must ensure the obtained referent is ensured live, just as for
>>>>>> other weak reference types.  This has always been a requirement, and
>>>>>> failure to do so appears to be a since-day1 G1 bug.
>>>>>>
>>>>>> There are two categories of places that need to address this issue:
>>>>>> JNIHandles::resolve and friends, and returning a jobject from native
>>>>>> code to Java.  In both of these places the jobject whose contained oop
>>>>>> is being extracted might really be a jweak.  jweak is
>>>>>> representationally indistinguishable from jobject; jweak is just a
>>>>>> typedef for jobject.  The documentation says a jweak can be used
>>>>>> anywhere a jobject is permitted, making that type equivalence
>>>>>> necessary for a C API.  (A C++ API might be able to have distinct
>>>>>> types via inheritance, though would still need a method for
>>>>>> distinguishing them at runtime.  But since JNI is a C API...).
>>>>>>
>>>>>> The basic approach being taken is to "tag" jweaks by setting the low
>>>>>> order bit (recall that jweak == jobject == _jobject*), in order to
>>>>>> distinguish them from non-jweak jobjects.  This tagging is only being
>>>>>> done when the selected GC needs the distinction, e.g. G1. This gives
>>>>>> us a representational difference between jobject and jweak on which to
>>>>>> base the different behavior, hiding that distinction behind the
>>>>>> existing API.
>>>>>>
>>>>>> JNIHandles::resolve and friends are changed to unconditionally strip
>>>>>> off any potential weak tag when translating from jobject to to oop*.
>>>>>> That's cheaper than testing for the conditional use of tagging and
>>>>>> then stripping.  Also stripped when deleting JNI handles.
>>>>>>
>>>>>> TemplateInterpreterGenerator::generate_native_entry and
>>>>>> SharedRuntime::generate_native_wrapper are changed to conditionally
>>>>>> emit code to apply the G1 pre-barrier to the value obtained from a
>>>>>> jobject result, when the result is tagged as a jweak, which only
>>>>>> occurs when using G1.
>>>>>>
>>>>>> For arm32/arm64, this required moving the g1_write_barrier_pre
>>>>>> definitions from InterpreterMacroAssembler to MacroAssembler.  Also
>>>>>> moved g1_write_barrier_post, for consistency.
>>>>>>
>>>>>
>>>>> In addition to Kim's final version of the change (which was backed
>>>>> out) this updated version adds code to mask out the weak tag bit in
>>>>> the fast JNI field getters on all platforms where fast JNI getters
>>>>> are used.
>>>>>
>>>>> […]
>>>>>
>>>>> Webrevs:
>>>>> Kim's initial change:
>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.kim/webrev/
>>>>> My additions:
>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.mg/webrev/
>>>>> Full webrev:
>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/
>>>>>
>>>>> Bugs:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8175085
>>>>> https://bugs.openjdk.java.net/browse/JDK-8166188
>>>>>
>>>>> Thanks
>>>>> /Mikael
>>>>
>>>>
>>>> Mikael - Thanks for taking this up for me.  Just a few minor comments.
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> The usual copyright reminder.
>>>
>>>
>>> Updated copyrights.
>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> src/cpu/arm/vm/jniFastGetField_arm.cpp
>>>>  122 #ifndef AARCH64
>>>>  123   __ bic(R1, R1, JNIHandles::weak_tag_mask);
>>>>  124 #else
>>>>  125   __ andr(R1, R1, ~JNIHandles::weak_tag_mask);
>>>>  126 #endif
>>>>
>>>> Usual style when selecting between AARCH64 and not seems to be to put
>>>> the AARCH64 code first, e.g.
>>>>
>>>> #ifdef AARCH64
>>>>   ...
>>>> #else
>>>>   ...
>>>> #endif
>>>
>>>
>>> Fixed.
>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>>>
>>>> Use STATIC_ASSERT rather than assert.
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>>>   89   const intptr_t inverted_jweak_mask =
>>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>>>   91   __ andl(rdx, inverted_jweak_mask); // mask is subject to
>>>> sign-extension
>>>>
>>>>
>>>> With three occurrences of this snippet in this file and two more
>>>> similar ones in the 64-bit file, a macroAssembler helper seems called
>>>> for here.
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>>>>   84   const intptr_t inverted_jweak_mask =
>>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>>>   85   const int32_t truncated_mask =
>>>> static_cast<int32_t>(inverted_jweak_mask);
>>>>
>>>> Why the two-step conversion here?  Why not just
>>>>
>>>>   const int32_t truncated_mask =
>>>> static_cast<int32_t>(JNIHandles::weak_tag_mask);
>>>
>>>
>>> I had changed this a bunch of times since I was a bit unsure about how
>>> to best do this but when I decided to do the bitwise invert after the
>>> cast to signed I forgot to clean up the casts.
>>>
>>>>
>>>> That would also make the 32 and 64-bit code more similar in the
>>>> suggested macroAssembler helper.
>>>
>>>
>>> Indeed, I even found MacroAssembler::andptr which makes this a nice
>>> and small helper.
>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>>>>   51 #define CHECK(variable, expected)                                 \
>>>>   52   do { if ((variable) != (expected)) {                            \
>>>>   53     (*env)->ThrowNew(env, exception,  #variable" != " #expected); \
>>>>   54 return;                                                       \
>>>>   55 } \
>>>>   56 } while(0);
>>>>
>>>> The formatting of the code in the macro body is strange and confusing.
>>>
>>>
>>> Cleaned up formatting.
>>>
>>>>
>>>> Also the do-while-zero idiom that I'm used to leaves off the final
>>>> semicolon in the macro.  Instead of the uses are expected to be
>>>> terminated with semicolons, as you've done.
>>>
>>>
>>> Oh, oops.
>>>
>>> I also took the liberty to add STATIC_ASSERTS on 64 bit arm since the
>>> encoding of the immediate in the "andr" instruction is a bit "funny".
>>> The assembler code only verifies that immediate encoding works in
>>> debug builds but in debug builds the fast JNI getters aren't generated.
>>>
>>> New webrevs:
>>> Incremental:
>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental/webrev/
>>> Full:
>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full2/webrev/
>>
>>
>> I only took a look at these files from the full webrev. These are the
>> files touch in the two incremental webrevs:
>>
>> src/cpu/aarch64/vm/jniFastGetField_aarch64.cpp
>>     No comments.
>>
>> src/cpu/arm/vm/jniFastGetField_arm.cpp
>>     No comments.
>>
>> src/cpu/sparc/vm/jniFastGetField_sparc.cpp
>>     No comments.
>>
>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>     No comments.
>>
>> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>>     No comments.
>>
>> src/cpu/x86/vm/macroAssembler_x86.cpp
>>     I like the new helper!
>>
>> src/cpu/x86/vm/macroAssembler_x86.hpp
>>     No comments.
>>
>> test/runtime/jni/CallWithJNIWeak/CallWithJNIWeak.java
>>     L50 - any reason for the blank line?
>
>
> No reason whatsoever, I'll remove it.
>
>>
>> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>>     L32 : Java_CallWithJNIWeak_testJNIFieldAccessors (JNIEnv *env,
>> jclass clazz, jobject this) {
>>     L102: Java_CallWithJNIWeak_runTests (JNIEnv *env, jclass clazz,
>> jobject this) {
>>     L137: Java_CallWithJNIWeak_weakReceiverTest0 (JNIEnv *env, jobject
>> obj) {
>>         Please delete the space before the first '('.
>
>
> Oh, oops. I'm going to blame javah for that :)
>
>>
>> You may want to add a comment between the two halves of
>> the test for the literal values that you're setting in
>> the .java file and testing for in the .c file.
>
>
> Added short comments in the .java and .c files.
>
>>
>> In your (Mikael G) original invite:
>>
>>> Testing:
>>> Standard JPRT and tier2-3 HotSpot tests.
>>
>>
>> Does tier2 and/or tier3 cover the tests that failed in Mach5?
>
>
> Somehow I forgot to mention that I also ran jdk_tier3 and the tests that
> failed in Mach5 did pass on all platforms.
>
>>
>>
>>> I've modified the CallWithJNIWeak test to call all the primitive
>>> getters and some other interesting cases I came up with.
>>> I've also run the CallWithJNIWeak test on all platforms on both
>>> product and fastdebug builds (since fast JNI getters are implicitly
>>> disabled in debug builds due to VerifyJNIFields being enabled by
>>> default in debug builds.
>>
>>
>> Great coverage with the new tests!
>>
>>
>>> I've not built or tested the aarch64 port but I think it's correct
>>> and I hope someone can test it for me.
>>
>>
>> Have you heard back from anyone on aarch64? I haven't
>> seen any e-mail on this OpenJDK thread...
>
>
> I've cc:ed Andrew in this thread to see if I can catch his attention.
>
> New webrevs at:
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full3/webrev/
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental2/webrev/
>
> Thanks
> /Mikael
>
>
>>
>> Thumbs up!
>>
>> Dan
>>
>>
>>>
>>> Thanks
>>> /Mikael
>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Mikael Gerdin
Hi Volker,

On 2017-02-24 12:37, Volker Simonis wrote:
> Hi Mikael,
>
> I had a little problems to apply the patch from your webrev because
> the file actually contains two patches [1] but after fixing that
> manually, I could still build and run the test on ppc64 and s390x.

I seem to recall that that's a known limitation in webrev.
Thanks for verifying the fix on your side.

/Mikael

>
> Thanks,
> Volker
>
> [1] http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/webrev/hotspot.changeset
>
> On Fri, Feb 24, 2017 at 10:55 AM, Mikael Gerdin
> <[hidden email]> wrote:
>> Hi Dan,
>>
>> Thanks for looking at this review!
>>
>>
>> On 2017-02-23 17:18, Daniel D. Daugherty wrote:
>>>
>>> On 2/23/17 6:29 AM, Mikael Gerdin wrote:
>>>>
>>>> Hi Kim,
>>>>
>>>> Thanks for the prompt review!
>>>>
>>>> On 2017-02-22 20:40, Kim Barrett wrote:
>>>>>>
>>>>>> On Feb 22, 2017, at 11:07 AM, Mikael Gerdin
>>>>>> <[hidden email]> wrote:
>>>>>>
>>>>>> Hi all,
>>>>>> Please review this revised change to jweak to support G1.
>>>>>>
>>>>>> I've copied Kim's original description of the fix below for reference:
>>>>>>
>>>>>>>
>>>>>>> The problem being addressed is that, when using G1, dereferencing a
>>>>>>> jweak must ensure the obtained referent is ensured live, just as for
>>>>>>> other weak reference types.  This has always been a requirement, and
>>>>>>> failure to do so appears to be a since-day1 G1 bug.
>>>>>>>
>>>>>>> There are two categories of places that need to address this issue:
>>>>>>> JNIHandles::resolve and friends, and returning a jobject from native
>>>>>>> code to Java.  In both of these places the jobject whose contained oop
>>>>>>> is being extracted might really be a jweak.  jweak is
>>>>>>> representationally indistinguishable from jobject; jweak is just a
>>>>>>> typedef for jobject.  The documentation says a jweak can be used
>>>>>>> anywhere a jobject is permitted, making that type equivalence
>>>>>>> necessary for a C API.  (A C++ API might be able to have distinct
>>>>>>> types via inheritance, though would still need a method for
>>>>>>> distinguishing them at runtime.  But since JNI is a C API...).
>>>>>>>
>>>>>>> The basic approach being taken is to "tag" jweaks by setting the low
>>>>>>> order bit (recall that jweak == jobject == _jobject*), in order to
>>>>>>> distinguish them from non-jweak jobjects.  This tagging is only being
>>>>>>> done when the selected GC needs the distinction, e.g. G1. This gives
>>>>>>> us a representational difference between jobject and jweak on which to
>>>>>>> base the different behavior, hiding that distinction behind the
>>>>>>> existing API.
>>>>>>>
>>>>>>> JNIHandles::resolve and friends are changed to unconditionally strip
>>>>>>> off any potential weak tag when translating from jobject to to oop*.
>>>>>>> That's cheaper than testing for the conditional use of tagging and
>>>>>>> then stripping.  Also stripped when deleting JNI handles.
>>>>>>>
>>>>>>> TemplateInterpreterGenerator::generate_native_entry and
>>>>>>> SharedRuntime::generate_native_wrapper are changed to conditionally
>>>>>>> emit code to apply the G1 pre-barrier to the value obtained from a
>>>>>>> jobject result, when the result is tagged as a jweak, which only
>>>>>>> occurs when using G1.
>>>>>>>
>>>>>>> For arm32/arm64, this required moving the g1_write_barrier_pre
>>>>>>> definitions from InterpreterMacroAssembler to MacroAssembler.  Also
>>>>>>> moved g1_write_barrier_post, for consistency.
>>>>>>>
>>>>>>
>>>>>> In addition to Kim's final version of the change (which was backed
>>>>>> out) this updated version adds code to mask out the weak tag bit in
>>>>>> the fast JNI field getters on all platforms where fast JNI getters
>>>>>> are used.
>>>>>>
>>>>>> […]
>>>>>>
>>>>>> Webrevs:
>>>>>> Kim's initial change:
>>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.kim/webrev/
>>>>>> My additions:
>>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.mg/webrev/
>>>>>> Full webrev:
>>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/
>>>>>>
>>>>>> Bugs:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8175085
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8166188
>>>>>>
>>>>>> Thanks
>>>>>> /Mikael
>>>>>
>>>>>
>>>>> Mikael - Thanks for taking this up for me.  Just a few minor comments.
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> The usual copyright reminder.
>>>>
>>>>
>>>> Updated copyrights.
>>>>
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/cpu/arm/vm/jniFastGetField_arm.cpp
>>>>>  122 #ifndef AARCH64
>>>>>  123   __ bic(R1, R1, JNIHandles::weak_tag_mask);
>>>>>  124 #else
>>>>>  125   __ andr(R1, R1, ~JNIHandles::weak_tag_mask);
>>>>>  126 #endif
>>>>>
>>>>> Usual style when selecting between AARCH64 and not seems to be to put
>>>>> the AARCH64 code first, e.g.
>>>>>
>>>>> #ifdef AARCH64
>>>>>   ...
>>>>> #else
>>>>>   ...
>>>>> #endif
>>>>
>>>>
>>>> Fixed.
>>>>
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>>>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>>>>
>>>>> Use STATIC_ASSERT rather than assert.
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>>>>   89   const intptr_t inverted_jweak_mask =
>>>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>>>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>>>>   91   __ andl(rdx, inverted_jweak_mask); // mask is subject to
>>>>> sign-extension
>>>>>
>>>>>
>>>>> With three occurrences of this snippet in this file and two more
>>>>> similar ones in the 64-bit file, a macroAssembler helper seems called
>>>>> for here.
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>>>>>   84   const intptr_t inverted_jweak_mask =
>>>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>>>>   85   const int32_t truncated_mask =
>>>>> static_cast<int32_t>(inverted_jweak_mask);
>>>>>
>>>>> Why the two-step conversion here?  Why not just
>>>>>
>>>>>   const int32_t truncated_mask =
>>>>> static_cast<int32_t>(JNIHandles::weak_tag_mask);
>>>>
>>>>
>>>> I had changed this a bunch of times since I was a bit unsure about how
>>>> to best do this but when I decided to do the bitwise invert after the
>>>> cast to signed I forgot to clean up the casts.
>>>>
>>>>>
>>>>> That would also make the 32 and 64-bit code more similar in the
>>>>> suggested macroAssembler helper.
>>>>
>>>>
>>>> Indeed, I even found MacroAssembler::andptr which makes this a nice
>>>> and small helper.
>>>>
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>>>>>   51 #define CHECK(variable, expected)                                 \
>>>>>   52   do { if ((variable) != (expected)) {                            \
>>>>>   53     (*env)->ThrowNew(env, exception,  #variable" != " #expected); \
>>>>>   54 return;                                                       \
>>>>>   55 } \
>>>>>   56 } while(0);
>>>>>
>>>>> The formatting of the code in the macro body is strange and confusing.
>>>>
>>>>
>>>> Cleaned up formatting.
>>>>
>>>>>
>>>>> Also the do-while-zero idiom that I'm used to leaves off the final
>>>>> semicolon in the macro.  Instead of the uses are expected to be
>>>>> terminated with semicolons, as you've done.
>>>>
>>>>
>>>> Oh, oops.
>>>>
>>>> I also took the liberty to add STATIC_ASSERTS on 64 bit arm since the
>>>> encoding of the immediate in the "andr" instruction is a bit "funny".
>>>> The assembler code only verifies that immediate encoding works in
>>>> debug builds but in debug builds the fast JNI getters aren't generated.
>>>>
>>>> New webrevs:
>>>> Incremental:
>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental/webrev/
>>>> Full:
>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full2/webrev/
>>>
>>>
>>> I only took a look at these files from the full webrev. These are the
>>> files touch in the two incremental webrevs:
>>>
>>> src/cpu/aarch64/vm/jniFastGetField_aarch64.cpp
>>>     No comments.
>>>
>>> src/cpu/arm/vm/jniFastGetField_arm.cpp
>>>     No comments.
>>>
>>> src/cpu/sparc/vm/jniFastGetField_sparc.cpp
>>>     No comments.
>>>
>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>>     No comments.
>>>
>>> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>>>     No comments.
>>>
>>> src/cpu/x86/vm/macroAssembler_x86.cpp
>>>     I like the new helper!
>>>
>>> src/cpu/x86/vm/macroAssembler_x86.hpp
>>>     No comments.
>>>
>>> test/runtime/jni/CallWithJNIWeak/CallWithJNIWeak.java
>>>     L50 - any reason for the blank line?
>>
>>
>> No reason whatsoever, I'll remove it.
>>
>>>
>>> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>>>     L32 : Java_CallWithJNIWeak_testJNIFieldAccessors (JNIEnv *env,
>>> jclass clazz, jobject this) {
>>>     L102: Java_CallWithJNIWeak_runTests (JNIEnv *env, jclass clazz,
>>> jobject this) {
>>>     L137: Java_CallWithJNIWeak_weakReceiverTest0 (JNIEnv *env, jobject
>>> obj) {
>>>         Please delete the space before the first '('.
>>
>>
>> Oh, oops. I'm going to blame javah for that :)
>>
>>>
>>> You may want to add a comment between the two halves of
>>> the test for the literal values that you're setting in
>>> the .java file and testing for in the .c file.
>>
>>
>> Added short comments in the .java and .c files.
>>
>>>
>>> In your (Mikael G) original invite:
>>>
>>>> Testing:
>>>> Standard JPRT and tier2-3 HotSpot tests.
>>>
>>>
>>> Does tier2 and/or tier3 cover the tests that failed in Mach5?
>>
>>
>> Somehow I forgot to mention that I also ran jdk_tier3 and the tests that
>> failed in Mach5 did pass on all platforms.
>>
>>>
>>>
>>>> I've modified the CallWithJNIWeak test to call all the primitive
>>>> getters and some other interesting cases I came up with.
>>>> I've also run the CallWithJNIWeak test on all platforms on both
>>>> product and fastdebug builds (since fast JNI getters are implicitly
>>>> disabled in debug builds due to VerifyJNIFields being enabled by
>>>> default in debug builds.
>>>
>>>
>>> Great coverage with the new tests!
>>>
>>>
>>>> I've not built or tested the aarch64 port but I think it's correct
>>>> and I hope someone can test it for me.
>>>
>>>
>>> Have you heard back from anyone on aarch64? I haven't
>>> seen any e-mail on this OpenJDK thread...
>>
>>
>> I've cc:ed Andrew in this thread to see if I can catch his attention.
>>
>> New webrevs at:
>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full3/webrev/
>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental2/webrev/
>>
>> Thanks
>> /Mikael
>>
>>
>>>
>>> Thumbs up!
>>>
>>> Dan
>>>
>>>
>>>>
>>>> Thanks
>>>> /Mikael
>>>>
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Thomas Schatzl
In reply to this post by Mikael Gerdin
Hi,

On Fri, 2017-02-24 at 10:55 +0100, Mikael Gerdin wrote:
> Hi Dan,
>
>
[...]

> > >
> > > I've not built or tested the aarch64 port but I think it's
> > > correct
> > > and I hope someone can test it for me.
> > Have you heard back from anyone on aarch64? I haven't
> > seen any e-mail on this OpenJDK thread...
> I've cc:ed Andrew in this thread to see if I can catch his attention.
>
> New webrevs at:
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full3/webrev/
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental2/webre
> v/

  looks good.

Thomas

Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Mikael Gerdin
In reply to this post by Mikael Gerdin
Actually CC:ing Andrew.

On 2017-02-24 10:55, Mikael Gerdin wrote:

> Hi Dan,
>
> Thanks for looking at this review!
>
> On 2017-02-23 17:18, Daniel D. Daugherty wrote:
>> On 2/23/17 6:29 AM, Mikael Gerdin wrote:
>>> Hi Kim,
>>>
>>> Thanks for the prompt review!
>>>
>>> On 2017-02-22 20:40, Kim Barrett wrote:
>>>>> On Feb 22, 2017, at 11:07 AM, Mikael Gerdin
>>>>> <[hidden email]> wrote:
>>>>>
>>>>> Hi all,
>>>>> Please review this revised change to jweak to support G1.
>>>>>
>>>>> I've copied Kim's original description of the fix below for reference:
>>>>>
>>>>>>
>>>>>> The problem being addressed is that, when using G1, dereferencing a
>>>>>> jweak must ensure the obtained referent is ensured live, just as for
>>>>>> other weak reference types.  This has always been a requirement, and
>>>>>> failure to do so appears to be a since-day1 G1 bug.
>>>>>>
>>>>>> There are two categories of places that need to address this issue:
>>>>>> JNIHandles::resolve and friends, and returning a jobject from native
>>>>>> code to Java.  In both of these places the jobject whose contained
>>>>>> oop
>>>>>> is being extracted might really be a jweak.  jweak is
>>>>>> representationally indistinguishable from jobject; jweak is just a
>>>>>> typedef for jobject.  The documentation says a jweak can be used
>>>>>> anywhere a jobject is permitted, making that type equivalence
>>>>>> necessary for a C API.  (A C++ API might be able to have distinct
>>>>>> types via inheritance, though would still need a method for
>>>>>> distinguishing them at runtime.  But since JNI is a C API...).
>>>>>>
>>>>>> The basic approach being taken is to "tag" jweaks by setting the low
>>>>>> order bit (recall that jweak == jobject == _jobject*), in order to
>>>>>> distinguish them from non-jweak jobjects.  This tagging is only being
>>>>>> done when the selected GC needs the distinction, e.g. G1. This gives
>>>>>> us a representational difference between jobject and jweak on
>>>>>> which to
>>>>>> base the different behavior, hiding that distinction behind the
>>>>>> existing API.
>>>>>>
>>>>>> JNIHandles::resolve and friends are changed to unconditionally strip
>>>>>> off any potential weak tag when translating from jobject to to oop*.
>>>>>> That's cheaper than testing for the conditional use of tagging and
>>>>>> then stripping.  Also stripped when deleting JNI handles.
>>>>>>
>>>>>> TemplateInterpreterGenerator::generate_native_entry and
>>>>>> SharedRuntime::generate_native_wrapper are changed to conditionally
>>>>>> emit code to apply the G1 pre-barrier to the value obtained from a
>>>>>> jobject result, when the result is tagged as a jweak, which only
>>>>>> occurs when using G1.
>>>>>>
>>>>>> For arm32/arm64, this required moving the g1_write_barrier_pre
>>>>>> definitions from InterpreterMacroAssembler to MacroAssembler.  Also
>>>>>> moved g1_write_barrier_post, for consistency.
>>>>>>
>>>>>
>>>>> In addition to Kim's final version of the change (which was backed
>>>>> out) this updated version adds code to mask out the weak tag bit in
>>>>> the fast JNI field getters on all platforms where fast JNI getters
>>>>> are used.
>>>>>
>>>>> […]
>>>>>
>>>>> Webrevs:
>>>>> Kim's initial change:
>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.kim/webrev/
>>>>> My additions:
>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.mg/webrev/
>>>>> Full webrev:
>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/
>>>>>
>>>>> Bugs:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8175085
>>>>> https://bugs.openjdk.java.net/browse/JDK-8166188
>>>>>
>>>>> Thanks
>>>>> /Mikael
>>>>
>>>> Mikael - Thanks for taking this up for me.  Just a few minor comments.
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> The usual copyright reminder.
>>>
>>> Updated copyrights.
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> src/cpu/arm/vm/jniFastGetField_arm.cpp
>>>>  122 #ifndef AARCH64
>>>>  123   __ bic(R1, R1, JNIHandles::weak_tag_mask);
>>>>  124 #else
>>>>  125   __ andr(R1, R1, ~JNIHandles::weak_tag_mask);
>>>>  126 #endif
>>>>
>>>> Usual style when selecting between AARCH64 and not seems to be to put
>>>> the AARCH64 code first, e.g.
>>>>
>>>> #ifdef AARCH64
>>>>   ...
>>>> #else
>>>>   ...
>>>> #endif
>>>
>>> Fixed.
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>>>
>>>> Use STATIC_ASSERT rather than assert.
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>>>   89   const intptr_t inverted_jweak_mask =
>>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>>>   91   __ andl(rdx, inverted_jweak_mask); // mask is subject to
>>>> sign-extension
>>>>
>>>>
>>>> With three occurrences of this snippet in this file and two more
>>>> similar ones in the 64-bit file, a macroAssembler helper seems called
>>>> for here.
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>>>>   84   const intptr_t inverted_jweak_mask =
>>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>>>   85   const int32_t truncated_mask =
>>>> static_cast<int32_t>(inverted_jweak_mask);
>>>>
>>>> Why the two-step conversion here?  Why not just
>>>>
>>>>   const int32_t truncated_mask =
>>>> static_cast<int32_t>(JNIHandles::weak_tag_mask);
>>>
>>> I had changed this a bunch of times since I was a bit unsure about how
>>> to best do this but when I decided to do the bitwise invert after the
>>> cast to signed I forgot to clean up the casts.
>>>
>>>>
>>>> That would also make the 32 and 64-bit code more similar in the
>>>> suggested macroAssembler helper.
>>>
>>> Indeed, I even found MacroAssembler::andptr which makes this a nice
>>> and small helper.
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>>>>   51 #define CHECK(variable,
>>>> expected)                                 \
>>>>   52   do { if ((variable) != (expected))
>>>> {                            \
>>>>   53     (*env)->ThrowNew(env, exception,  #variable" != "
>>>> #expected); \
>>>>   54 return;                                                       \
>>>>   55 } \
>>>>   56 } while(0);
>>>>
>>>> The formatting of the code in the macro body is strange and confusing.
>>>
>>> Cleaned up formatting.
>>>
>>>>
>>>> Also the do-while-zero idiom that I'm used to leaves off the final
>>>> semicolon in the macro.  Instead of the uses are expected to be
>>>> terminated with semicolons, as you've done.
>>>
>>> Oh, oops.
>>>
>>> I also took the liberty to add STATIC_ASSERTS on 64 bit arm since the
>>> encoding of the immediate in the "andr" instruction is a bit "funny".
>>> The assembler code only verifies that immediate encoding works in
>>> debug builds but in debug builds the fast JNI getters aren't generated.
>>>
>>> New webrevs:
>>> Incremental:
>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental/webrev/
>>> Full:
>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full2/webrev/
>>
>> I only took a look at these files from the full webrev. These are the
>> files touch in the two incremental webrevs:
>>
>> src/cpu/aarch64/vm/jniFastGetField_aarch64.cpp
>>     No comments.
>>
>> src/cpu/arm/vm/jniFastGetField_arm.cpp
>>     No comments.
>>
>> src/cpu/sparc/vm/jniFastGetField_sparc.cpp
>>     No comments.
>>
>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>     No comments.
>>
>> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>>     No comments.
>>
>> src/cpu/x86/vm/macroAssembler_x86.cpp
>>     I like the new helper!
>>
>> src/cpu/x86/vm/macroAssembler_x86.hpp
>>     No comments.
>>
>> test/runtime/jni/CallWithJNIWeak/CallWithJNIWeak.java
>>     L50 - any reason for the blank line?
>
> No reason whatsoever, I'll remove it.
>
>>
>> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>>     L32 : Java_CallWithJNIWeak_testJNIFieldAccessors (JNIEnv *env,
>> jclass clazz, jobject this) {
>>     L102: Java_CallWithJNIWeak_runTests (JNIEnv *env, jclass clazz,
>> jobject this) {
>>     L137: Java_CallWithJNIWeak_weakReceiverTest0 (JNIEnv *env, jobject
>> obj) {
>>         Please delete the space before the first '('.
>
> Oh, oops. I'm going to blame javah for that :)
>
>>
>> You may want to add a comment between the two halves of
>> the test for the literal values that you're setting in
>> the .java file and testing for in the .c file.
>
> Added short comments in the .java and .c files.
>
>>
>> In your (Mikael G) original invite:
>>
>>> Testing:
>>> Standard JPRT and tier2-3 HotSpot tests.
>>
>> Does tier2 and/or tier3 cover the tests that failed in Mach5?
>
> Somehow I forgot to mention that I also ran jdk_tier3 and the tests that
> failed in Mach5 did pass on all platforms.
>
>>
>>
>>> I've modified the CallWithJNIWeak test to call all the primitive
>>> getters and some other interesting cases I came up with.
>>> I've also run the CallWithJNIWeak test on all platforms on both
>>> product and fastdebug builds (since fast JNI getters are implicitly
>>> disabled in debug builds due to VerifyJNIFields being enabled by
>>> default in debug builds.
>>
>> Great coverage with the new tests!
>>
>>
>>> I've not built or tested the aarch64 port but I think it's correct
>>> and I hope someone can test it for me.
>>
>> Have you heard back from anyone on aarch64? I haven't
>> seen any e-mail on this OpenJDK thread...
>
> I've cc:ed Andrew in this thread to see if I can catch his attention.
>
> New webrevs at:
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full3/webrev/
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental2/webrev/
>
> Thanks
> /Mikael
>
>>
>> Thumbs up!
>>
>> Dan
>>
>>
>>>
>>> Thanks
>>> /Mikael
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Stuart Monteith
In reply to this post by Mikael Gerdin
Hi,
   I can build and test the patch on AArch64. I can post the results here.

BR,
   Stuart


On 24 February 2017 at 09:55, Mikael Gerdin <[hidden email]> wrote:

> Hi Dan,
>
> Thanks for looking at this review!
>
>
> On 2017-02-23 17:18, Daniel D. Daugherty wrote:
>>
>> On 2/23/17 6:29 AM, Mikael Gerdin wrote:
>>>
>>> Hi Kim,
>>>
>>> Thanks for the prompt review!
>>>
>>> On 2017-02-22 20:40, Kim Barrett wrote:
>>>>>
>>>>> On Feb 22, 2017, at 11:07 AM, Mikael Gerdin
>>>>> <[hidden email]> wrote:
>>>>>
>>>>> Hi all,
>>>>> Please review this revised change to jweak to support G1.
>>>>>
>>>>> I've copied Kim's original description of the fix below for reference:
>>>>>
>>>>>>
>>>>>> The problem being addressed is that, when using G1, dereferencing a
>>>>>> jweak must ensure the obtained referent is ensured live, just as for
>>>>>> other weak reference types.  This has always been a requirement, and
>>>>>> failure to do so appears to be a since-day1 G1 bug.
>>>>>>
>>>>>> There are two categories of places that need to address this issue:
>>>>>> JNIHandles::resolve and friends, and returning a jobject from native
>>>>>> code to Java.  In both of these places the jobject whose contained oop
>>>>>> is being extracted might really be a jweak.  jweak is
>>>>>> representationally indistinguishable from jobject; jweak is just a
>>>>>> typedef for jobject.  The documentation says a jweak can be used
>>>>>> anywhere a jobject is permitted, making that type equivalence
>>>>>> necessary for a C API.  (A C++ API might be able to have distinct
>>>>>> types via inheritance, though would still need a method for
>>>>>> distinguishing them at runtime.  But since JNI is a C API...).
>>>>>>
>>>>>> The basic approach being taken is to "tag" jweaks by setting the low
>>>>>> order bit (recall that jweak == jobject == _jobject*), in order to
>>>>>> distinguish them from non-jweak jobjects.  This tagging is only being
>>>>>> done when the selected GC needs the distinction, e.g. G1. This gives
>>>>>> us a representational difference between jobject and jweak on which to
>>>>>> base the different behavior, hiding that distinction behind the
>>>>>> existing API.
>>>>>>
>>>>>> JNIHandles::resolve and friends are changed to unconditionally strip
>>>>>> off any potential weak tag when translating from jobject to to oop*.
>>>>>> That's cheaper than testing for the conditional use of tagging and
>>>>>> then stripping.  Also stripped when deleting JNI handles.
>>>>>>
>>>>>> TemplateInterpreterGenerator::generate_native_entry and
>>>>>> SharedRuntime::generate_native_wrapper are changed to conditionally
>>>>>> emit code to apply the G1 pre-barrier to the value obtained from a
>>>>>> jobject result, when the result is tagged as a jweak, which only
>>>>>> occurs when using G1.
>>>>>>
>>>>>> For arm32/arm64, this required moving the g1_write_barrier_pre
>>>>>> definitions from InterpreterMacroAssembler to MacroAssembler.  Also
>>>>>> moved g1_write_barrier_post, for consistency.
>>>>>>
>>>>>
>>>>> In addition to Kim's final version of the change (which was backed
>>>>> out) this updated version adds code to mask out the weak tag bit in
>>>>> the fast JNI field getters on all platforms where fast JNI getters
>>>>> are used.
>>>>>
>>>>> […]
>>>>>
>>>>> Webrevs:
>>>>> Kim's initial change:
>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.kim/webrev/
>>>>> My additions:
>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.mg/webrev/
>>>>> Full webrev:
>>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/
>>>>>
>>>>> Bugs:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8175085
>>>>> https://bugs.openjdk.java.net/browse/JDK-8166188
>>>>>
>>>>> Thanks
>>>>> /Mikael
>>>>
>>>>
>>>> Mikael - Thanks for taking this up for me.  Just a few minor comments.
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> The usual copyright reminder.
>>>
>>>
>>> Updated copyrights.
>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> src/cpu/arm/vm/jniFastGetField_arm.cpp
>>>>  122 #ifndef AARCH64
>>>>  123   __ bic(R1, R1, JNIHandles::weak_tag_mask);
>>>>  124 #else
>>>>  125   __ andr(R1, R1, ~JNIHandles::weak_tag_mask);
>>>>  126 #endif
>>>>
>>>> Usual style when selecting between AARCH64 and not seems to be to put
>>>> the AARCH64 code first, e.g.
>>>>
>>>> #ifdef AARCH64
>>>>   ...
>>>> #else
>>>>   ...
>>>> #endif
>>>
>>>
>>> Fixed.
>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>>>
>>>> Use STATIC_ASSERT rather than assert.
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>>>   89   const intptr_t inverted_jweak_mask =
>>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>>>   90   assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>>>   91   __ andl(rdx, inverted_jweak_mask); // mask is subject to
>>>> sign-extension
>>>>
>>>>
>>>> With three occurrences of this snippet in this file and two more
>>>> similar ones in the 64-bit file, a macroAssembler helper seems called
>>>> for here.
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>>>>   84   const intptr_t inverted_jweak_mask =
>>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>>>   85   const int32_t truncated_mask =
>>>> static_cast<int32_t>(inverted_jweak_mask);
>>>>
>>>> Why the two-step conversion here?  Why not just
>>>>
>>>>   const int32_t truncated_mask =
>>>> static_cast<int32_t>(JNIHandles::weak_tag_mask);
>>>
>>>
>>> I had changed this a bunch of times since I was a bit unsure about how
>>> to best do this but when I decided to do the bitwise invert after the
>>> cast to signed I forgot to clean up the casts.
>>>
>>>>
>>>> That would also make the 32 and 64-bit code more similar in the
>>>> suggested macroAssembler helper.
>>>
>>>
>>> Indeed, I even found MacroAssembler::andptr which makes this a nice
>>> and small helper.
>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>>>>   51 #define CHECK(variable, expected)                                 \
>>>>   52   do { if ((variable) != (expected)) {                            \
>>>>   53     (*env)->ThrowNew(env, exception,  #variable" != " #expected); \
>>>>   54 return;                                                       \
>>>>   55 } \
>>>>   56 } while(0);
>>>>
>>>> The formatting of the code in the macro body is strange and confusing.
>>>
>>>
>>> Cleaned up formatting.
>>>
>>>>
>>>> Also the do-while-zero idiom that I'm used to leaves off the final
>>>> semicolon in the macro.  Instead of the uses are expected to be
>>>> terminated with semicolons, as you've done.
>>>
>>>
>>> Oh, oops.
>>>
>>> I also took the liberty to add STATIC_ASSERTS on 64 bit arm since the
>>> encoding of the immediate in the "andr" instruction is a bit "funny".
>>> The assembler code only verifies that immediate encoding works in
>>> debug builds but in debug builds the fast JNI getters aren't generated.
>>>
>>> New webrevs:
>>> Incremental:
>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental/webrev/
>>> Full:
>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full2/webrev/
>>
>>
>> I only took a look at these files from the full webrev. These are the
>> files touch in the two incremental webrevs:
>>
>> src/cpu/aarch64/vm/jniFastGetField_aarch64.cpp
>>     No comments.
>>
>> src/cpu/arm/vm/jniFastGetField_arm.cpp
>>     No comments.
>>
>> src/cpu/sparc/vm/jniFastGetField_sparc.cpp
>>     No comments.
>>
>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>     No comments.
>>
>> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>>     No comments.
>>
>> src/cpu/x86/vm/macroAssembler_x86.cpp
>>     I like the new helper!
>>
>> src/cpu/x86/vm/macroAssembler_x86.hpp
>>     No comments.
>>
>> test/runtime/jni/CallWithJNIWeak/CallWithJNIWeak.java
>>     L50 - any reason for the blank line?
>
>
> No reason whatsoever, I'll remove it.
>
>>
>> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>>     L32 : Java_CallWithJNIWeak_testJNIFieldAccessors (JNIEnv *env,
>> jclass clazz, jobject this) {
>>     L102: Java_CallWithJNIWeak_runTests (JNIEnv *env, jclass clazz,
>> jobject this) {
>>     L137: Java_CallWithJNIWeak_weakReceiverTest0 (JNIEnv *env, jobject
>> obj) {
>>         Please delete the space before the first '('.
>
>
> Oh, oops. I'm going to blame javah for that :)
>
>>
>> You may want to add a comment between the two halves of
>> the test for the literal values that you're setting in
>> the .java file and testing for in the .c file.
>
>
> Added short comments in the .java and .c files.
>
>>
>> In your (Mikael G) original invite:
>>
>>> Testing:
>>> Standard JPRT and tier2-3 HotSpot tests.
>>
>>
>> Does tier2 and/or tier3 cover the tests that failed in Mach5?
>
>
> Somehow I forgot to mention that I also ran jdk_tier3 and the tests that
> failed in Mach5 did pass on all platforms.
>
>>
>>
>>> I've modified the CallWithJNIWeak test to call all the primitive
>>> getters and some other interesting cases I came up with.
>>> I've also run the CallWithJNIWeak test on all platforms on both
>>> product and fastdebug builds (since fast JNI getters are implicitly
>>> disabled in debug builds due to VerifyJNIFields being enabled by
>>> default in debug builds.
>>
>>
>> Great coverage with the new tests!
>>
>>
>>> I've not built or tested the aarch64 port but I think it's correct
>>> and I hope someone can test it for me.
>>
>>
>> Have you heard back from anyone on aarch64? I haven't
>> seen any e-mail on this OpenJDK thread...
>
>
> I've cc:ed Andrew in this thread to see if I can catch his attention.
>
> New webrevs at:
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full3/webrev/
> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental2/webrev/
>
> Thanks
> /Mikael
>
>
>>
>> Thumbs up!
>>
>> Dan
>>
>>
>>>
>>> Thanks
>>> /Mikael
>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Andrew Dinn
In reply to this post by Mikael Gerdin
Hi Mikael,

On 24/02/17 12:02, Mikael Gerdin wrote:
> Actually CC:ing Andrew.

I'm still building this and will test it as soon as I can.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Stuart Monteith
Hello,
  After applying the patch to the latest tree jdk9/dev tree, it builds
cleanly and the hotspot JTReg tests pass. I'm currently running with
jdk and langtools.

BR,
   Stuart


On 24 February 2017 at 16:36, Andrew Dinn <[hidden email]> wrote:

> Hi Mikael,
>
> On 24/02/17 12:02, Mikael Gerdin wrote:
>> Actually CC:ing Andrew.
>
> I'm still building this and will test it as soon as I can.
>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Andrew Dinn
Hi Stuart,

On 24/02/17 18:06, Stuart Monteith wrote:
> Hello,
>   After applying the patch to the latest tree jdk9/dev tree, it builds
> cleanly and the hotspot JTReg tests pass. I'm currently running with
> jdk and langtools.

Apologies for the delay in reporting my results. My build also completed
cleanly and pass the hotspot jtreg test.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Reply | Threaded
Open this post in threaded view
|

Re: 8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles

Mikael Gerdin
Hi,

On 2017-02-27 13:52, Andrew Dinn wrote:

> Hi Stuart,
>
> On 24/02/17 18:06, Stuart Monteith wrote:
>> Hello,
>>   After applying the patch to the latest tree jdk9/dev tree, it builds
>> cleanly and the hotspot JTReg tests pass. I'm currently running with
>> jdk and langtools.
>
> Apologies for the delay in reporting my results. My build also completed
> cleanly and pass the hotspot jtreg test.

Thanks guys.
I'll go ahead and push this tomorow.

/Mikael

>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>