RFR(M) 8176100: [REDO][REDO] G1 Needs pre barrier on dereference of weak JNI handles

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR(M) 8176100: [REDO][REDO] G1 Needs pre barrier on dereference of weak JNI handles

Mikael Gerdin
Here we go again...
This time around the fix is exactly the same but since I changed the
lock ranks in 8176363 we shouldn't be hitting any lock ordering assertions.


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.

The aarch64 change has been tested in the previous round of this change.

Full webrev:
http://cr.openjdk.java.net/~mgerdin/8176100/webrev.0/

Bugs:
https://bugs.openjdk.java.net/browse/JDK-8176100
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
|  
Report Content as Inappropriate

Re: RFR(M) 8176100: [REDO][REDO] G1 Needs pre barrier on dereference of weak JNI handles

Kim Barrett
> On Mar 14, 2017, at 9:11 AM, Mikael Gerdin <[hidden email]> wrote:
>
> Here we go again...
> This time around the fix is exactly the same but since I changed the lock ranks in 8176363 we shouldn't be hitting any lock ordering assertions.
>
>
> […]
> Full webrev:
> http://cr.openjdk.java.net/~mgerdin/8176100/webrev.0/
>
> Bugs:
> https://bugs.openjdk.java.net/browse/JDK-8176100
> https://bugs.openjdk.java.net/browse/JDK-8175085
> https://bugs.openjdk.java.net/browse/JDK-8166188
>
> Thanks
> /Mikael

Looks good.  But I've said that before. Hopefully we've run out of
places that cheat about the "equivalence" of jobject and oop*.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M) 8176100: [REDO][REDO] G1 Needs pre barrier on dereference of weak JNI handles

coleen.phillimore
In reply to this post by Mikael Gerdin
This looks good still.   For
https://bugs.openjdk.java.net/browse/JDK-8176363

why are the two locks "special"?  Is that because there is no leaf-2?  
Just curious.

Thanks,
Coleen

On 3/14/17 9:11 AM, Mikael Gerdin wrote:

> Here we go again...
> This time around the fix is exactly the same but since I changed the
> lock ranks in 8176363 we shouldn't be hitting any lock ordering
> assertions.
>
>
> 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.
>
> The aarch64 change has been tested in the previous round of this change.
>
> Full webrev:
> http://cr.openjdk.java.net/~mgerdin/8176100/webrev.0/
>
> Bugs:
> https://bugs.openjdk.java.net/browse/JDK-8176100
> 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
|  
Report Content as Inappropriate

Re: RFR(M) 8176100: [REDO][REDO] G1 Needs pre barrier on dereference of weak JNI handles

Mikael Gerdin
Hi Coleen,

On 2017-03-14 20:35, [hidden email] wrote:
> This looks good still.   For

Thanks.


> https://bugs.openjdk.java.net/browse/JDK-8176363
>
> why are the two locks "special"?  Is that because there is no leaf-2?
> Just curious.

Unclear.
I guess it's because they are guarding a very small region and someone
wanted to make sure that no other locks are taken when they are held.

/Mikael

>
> Thanks,
> Coleen
>
> On 3/14/17 9:11 AM, Mikael Gerdin wrote:
>> Here we go again...
>> This time around the fix is exactly the same but since I changed the
>> lock ranks in 8176363 we shouldn't be hitting any lock ordering
>> assertions.
>>
>>
>> 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.
>>
>> The aarch64 change has been tested in the previous round of this change.
>>
>> Full webrev:
>> http://cr.openjdk.java.net/~mgerdin/8176100/webrev.0/
>>
>> Bugs:
>> https://bugs.openjdk.java.net/browse/JDK-8176100
>> 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
|  
Report Content as Inappropriate

Re: RFR(M) 8176100: [REDO][REDO] G1 Needs pre barrier on dereference of weak JNI handles

Mikael Gerdin
In reply to this post by Kim Barrett
Hi Kim,

On 2017-03-14 16:29, Kim Barrett wrote:

>> On Mar 14, 2017, at 9:11 AM, Mikael Gerdin <[hidden email]> wrote:
>>
>> Here we go again...
>> This time around the fix is exactly the same but since I changed the lock ranks in 8176363 we shouldn't be hitting any lock ordering assertions.
>>
>>
>> […]
>> Full webrev:
>> http://cr.openjdk.java.net/~mgerdin/8176100/webrev.0/
>>
>> Bugs:
>> https://bugs.openjdk.java.net/browse/JDK-8176100
>> https://bugs.openjdk.java.net/browse/JDK-8175085
>> https://bugs.openjdk.java.net/browse/JDK-8166188
>>
>> Thanks
>> /Mikael
>
> Looks good.  But I've said that before. Hopefully we've run out of
> places that cheat about the "equivalence" of jobject and oop*.

Thanks.
I'm re-running a bunch of tests upon request so I'll integrate this once
testing is completed.

/Mikael

>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M) 8176100: [REDO][REDO] G1 Needs pre barrier on dereference of weak JNI handles

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

On Tue, 2017-03-14 at 14:11 +0100, Mikael Gerdin wrote:
> Here we go again...
> This time around the fix is exactly the same but since I changed the 
> lock ranks in 8176363 we shouldn't be hitting any lock ordering
> assertions.

  (still) looks good to me.

As discussed privately, please wait for the current test runs to
complete before pushing to minimize the risk for a [REDO]^3.

Thanks,
  Thomas

Loading...