RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

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

RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Kim Barrett
Please review this new test of the handling of weak JNI handles during
G1 concurrent mark.

CR:
https://bugs.openjdk.java.net/browse/JDK-8178813

Webrev:
http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.00/

Testing:
JPRT  

Locally ran new test with G1-specific code in JNIHandles::resolve and
MacroAssembler::resolve_jobject temporarily removed, and verified
expected failures.

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

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Kim Barrett
Still looking for reviewers.

> On Apr 14, 2017, at 4:41 PM, Kim Barrett <[hidden email]> wrote:
>
> Please review this new test of the handling of weak JNI handles during
> G1 concurrent mark.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8178813
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.00/
>
> Testing:
> JPRT  
>
> Locally ran new test with G1-specific code in JNIHandles::resolve and
> MacroAssembler::resolve_jobject temporarily removed, and verified
> expected failures.


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

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Mikael Gerdin
Hi Kim,

On 2017-05-05 07:45, Kim Barrett wrote:

> Still looking for reviewers.
>
>> On Apr 14, 2017, at 4:41 PM, Kim Barrett <[hidden email]> wrote:
>>
>> Please review this new test of the handling of weak JNI handles during
>> G1 concurrent mark.
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8178813
>>
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.00/

Test seems ok to me, I might have made the "static boolean resolve" an
instance member and created two different instances of the test class
because I dislike that kind of state in static members but I'm not going
to block the change because of it.

Reviewed.
/Mikael

>>
>> Testing:
>> JPRT
>>
>> Locally ran new test with G1-specific code in JNIHandles::resolve and
>> MacroAssembler::resolve_jobject temporarily removed, and verified
>> expected failures.
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Kim Barrett
> On May 5, 2017, at 7:58 AM, Mikael Gerdin <[hidden email]> wrote:
>
> Hi Kim,
>
> On 2017-05-05 07:45, Kim Barrett wrote:
>> Still looking for reviewers.
>>
>>> On Apr 14, 2017, at 4:41 PM, Kim Barrett <[hidden email]> wrote:
>>>
>>> Please review this new test of the handling of weak JNI handles during
>>> G1 concurrent mark.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8178813
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.00/
>
> Test seems ok to me, I might have made the "static boolean resolve" an instance member and created two different instances of the test class because I dislike that kind of state in static members but I'm not going to block the change because of it.

I think you are right, and I’m going to change it along those lines.  New webrev will be forthcoming.

> Reviewed.
> /Mikael
>
>>>
>>> Testing:
>>> JPRT
>>>
>>> Locally ran new test with G1-specific code in JNIHandles::resolve and
>>> MacroAssembler::resolve_jobject temporarily removed, and verified
>>> expected failures.


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

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Kim Barrett
> On May 5, 2017, at 6:19 PM, Kim Barrett <[hidden email]> wrote:
>
>> On May 5, 2017, at 7:58 AM, Mikael Gerdin <[hidden email]> wrote:
>>
>> Hi Kim,
>>
>> On 2017-05-05 07:45, Kim Barrett wrote:
>>> Still looking for reviewers.
>>>
>>>> On Apr 14, 2017, at 4:41 PM, Kim Barrett <[hidden email]> wrote:
>>>>
>>>> Please review this new test of the handling of weak JNI handles during
>>>> G1 concurrent mark.
>>>>
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8178813
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.00/
>>
>> Test seems ok to me, I might have made the "static boolean resolve" an instance member and created two different instances of the test class because I dislike that kind of state in static members but I'm not going to block the change because of it.
>
> I think you are right, and I’m going to change it along those lines.  New webrev will be forthcoming.

full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01/
incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01.inc/

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

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Mikael Gerdin
Hi Kim,

On 2017-05-09 07:46, Kim Barrett wrote:

>> On May 5, 2017, at 6:19 PM, Kim Barrett <[hidden email]> wrote:
>>
>>> On May 5, 2017, at 7:58 AM, Mikael Gerdin <[hidden email]> wrote:
>>>
>>> Hi Kim,
>>>
>>> On 2017-05-05 07:45, Kim Barrett wrote:
>>>> Still looking for reviewers.
>>>>
>>>>> On Apr 14, 2017, at 4:41 PM, Kim Barrett <[hidden email]> wrote:
>>>>>
>>>>> Please review this new test of the handling of weak JNI handles during
>>>>> G1 concurrent mark.
>>>>>
>>>>> CR:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8178813
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.00/
>>>
>>> Test seems ok to me, I might have made the "static boolean resolve" an instance member and created two different instances of the test class because I dislike that kind of state in static members but I'm not going to block the change because of it.
>>
>> I think you are right, and I’m going to change it along those lines.  New webrev will be forthcoming.
>
> full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01.inc/
>

Thanks for changing this, still looks good.
/Mikael
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Per Liden
In reply to this post by Kim Barrett
Hi Kim,

On 2017-05-09 07:46, Kim Barrett wrote:

>> On May 5, 2017, at 6:19 PM, Kim Barrett <[hidden email]> wrote:
>>
>>> On May 5, 2017, at 7:58 AM, Mikael Gerdin <[hidden email]> wrote:
>>>
>>> Hi Kim,
>>>
>>> On 2017-05-05 07:45, Kim Barrett wrote:
>>>> Still looking for reviewers.
>>>>
>>>>> On Apr 14, 2017, at 4:41 PM, Kim Barrett <[hidden email]> wrote:
>>>>>
>>>>> Please review this new test of the handling of weak JNI handles during
>>>>> G1 concurrent mark.
>>>>>
>>>>> CR:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8178813
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.00/
>>>
>>> Test seems ok to me, I might have made the "static boolean resolve" an instance member and created two different instances of the test class because I dislike that kind of state in static members but I'm not going to block the change because of it.
>>
>> I think you are right, and I’m going to change it along those lines.  New webrev will be forthcoming.
>
> full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01.inc/

Looks good to me.

cheers,
Per

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

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

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

On Tue, 2017-05-09 at 10:08 +0200, Mikael Gerdin wrote:
> Hi Kim,
> [...]
> On 2017-05-09 07:46, Kim Barrett wrote:
> > [...]
> > full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01/
> > incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01.inc/

  some minor comments:

- I would recommend using whitebox api to issue the correct kind of GC
instead of calling system.gc() in the first place. This would avoid the
need to disallow ExplicitGCInvokesConcurrent. It also seems to be a
minor effort as the test already uses whitebox. If you think the loop
using system.gc is preferable, feel free to keep it.

- TestJNIWeakG1.java:206: the printed text does not correspond to the
method name like the other methods do ("checkShouldNotClear" vs.
"checkShouldNotCrash")

- not sure about the need for the error handling using booleans in the
main()/check() method. I would think that letting the Exception just
propagate and the test terminate asap would be sufficient.

The extra information about the second test failing in case the first
failed does not seem to be very helpful given that the test is very
small anyway.

Thanks,
  Thomas

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

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Thomas Schatzl
Hi again,

On Tue, 2017-05-09 at 10:34 +0200, Thomas Schatzl wrote:

> Hi Kim,
>
> On Tue, 2017-05-09 at 10:08 +0200, Mikael Gerdin wrote:
> >
> > Hi Kim,
> > [...]
> > On 2017-05-09 07:46, Kim Barrett wrote:
> > >
> > > [...]
> > > full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01/
> > > incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01.inc
> > > /
>   some minor comments:
>
> - I would recommend using whitebox api to issue the correct kind of
> GC instead of calling system.gc() in the first place. This would
> avoid the need to disallow ExplicitGCInvokesConcurrent. It also seems
> to be a minor effort as the test already uses whitebox. If you think
> the loop using system.gc is preferable, feel free to keep it.
>
> - TestJNIWeakG1.java:206: the printed text does not correspond to the
> method name like the other methods do ("checkShouldNotClear" vs.
> "checkShouldNotCrash")

I do not need a re-review for this change.

> - not sure about the need for the error handling using booleans in
> the main()/check() method. I would think that letting the Exception
> just propagate and the test terminate asap would be sufficient.
>
> The extra information about the second test failing in case the first
> failed does not seem to be very helpful given that the test is very
> small anyway.

I am okay to keep it as is too, but it only seems some extra
unnecessary code.

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

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Kim Barrett
In reply to this post by Thomas Schatzl
> On May 9, 2017, at 4:34 AM, Thomas Schatzl <[hidden email]> wrote:
>
> Hi Kim,
>
> On Tue, 2017-05-09 at 10:08 +0200, Mikael Gerdin wrote:
>> Hi Kim,
>> [...]
>> On 2017-05-09 07:46, Kim Barrett wrote:
>>> [...]
>>> full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01/
>>> incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01.inc/
>
>   some minor comments:

Thanks for looking at this.

> - I would recommend using whitebox api to issue the correct kind of GC
> instead of calling system.gc() in the first place. This would avoid the
> need to disallow ExplicitGCInvokesConcurrent. It also seems to be a
> minor effort as the test already uses whitebox. If you think the loop
> using system.gc is preferable, feel free to keep it.

Good point.  Fixed.

> - TestJNIWeakG1.java:206: the printed text does not correspond to the
> method name like the other methods do ("checkShouldNotClear" vs.
> "checkShouldNotCrash”)

Oops.  Fixed.

> - not sure about the need for the error handling using booleans in the
> main()/check() method. I would think that letting the Exception just
> propagate and the test terminate asap would be sufficient.
>
> The extra information about the second test failing in case the first
> failed does not seem to be very helpful given that the test is very
> small anyway.

It made it easier to test the test, by backing out the fixes being tested and
verifying both failure modes.  But I agree it does uglify the test itself, and
now that we expected it to always pass, that’s more important.  So fixed.

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.02/
incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.02.inc/

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

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Per Liden
On 2017-05-11 01:39, Kim Barrett wrote:
[...]
> It made it easier to test the test, by backing out the fixes being tested and
> verifying both failure modes.  But I agree it does uglify the test itself, and
> now that we expected it to always pass, that’s more important.  So fixed.
>
> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.02/
> incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.02.inc/
>

Still looks good.

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

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Thomas Schatzl
In reply to this post by Kim Barrett
Hi Kim,

On Wed, 2017-05-10 at 19:39 -0400, Kim Barrett wrote:

> >
> > On May 9, 2017, at 4:34 AM, Thomas Schatzl <[hidden email]
> > om> wrote:
> >
> > Hi Kim,
> >
> > On Tue, 2017-05-09 at 10:08 +0200, Mikael Gerdin wrote:
> > >
> > > Hi Kim,
> > > [...]
> > > On 2017-05-09 07:46, Kim Barrett wrote:
> > > >
> > > > [...]
> > > > full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01/
> > > > incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.01.i
> > > > nc/
> >   some minor comments:
> Thanks for looking at this.
>
> >
> > - I would recommend using whitebox api to issue the correct kind of
> > [...]
> Good point.  Fixed.
>
> >
> > - TestJNIWeakG1.java:206: the printed text does not correspond to
> > [...]
> Oops.  Fixed.
>
> >
> > - not sure about the need for the error handling using booleans in
> > [...]
> It made it easier to test the test, by backing out the fixes being
> tested and verifying both failure modes.  But I agree it does uglify
> the test itself, and now that we expected it to always pass, that’s
> more important.  So fixed.
>
> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.02/
> incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.02.inc/
>

  looks good.

Thomas

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

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Kim Barrett
> On May 11, 2017, at 5:57 AM, Thomas Schatzl <[hidden email]> wrote:
>
>> […]
>> New webrevs:
>> full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.02/
>> incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.02.inc/
>>
>
>   looks good.
>
> Thomas

Thanks.

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

Re: RFR: 8178813: Add test for G1 pre-barrier on dereference of weak JNI handles

Kim Barrett
In reply to this post by Per Liden
> On May 11, 2017, at 5:34 AM, Per Liden <[hidden email]> wrote:
>
> On 2017-05-11 01:39, Kim Barrett wrote:
> [...]
>> It made it easier to test the test, by backing out the fixes being tested and
>> verifying both failure modes.  But I agree it does uglify the test itself, and
>> now that we expected it to always pass, that’s more important.  So fixed.
>>
>> New webrevs:
>> full: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.02/
>> incr: http://cr.openjdk.java.net/~kbarrett/8178813/hotspot.02.inc/
>>
>
> Still looks good.
>
> cheers,
> Per

Thanks.

Loading...