Request for review for JDK-8178507 - co-locate nsk.regression.gc tests

Wed Jun 28 21:35:35 UTC 2017

> On Jun 28, 2017, at 11:27 AM, Alexander Harlap <alexander.harlap at oracle.com> wrote:
> 
> Hi Leonid and Igor,
> 
> It looks like we need extra round of review:
> 
> New version is here: http://cr.openjdk.java.net/~aharlap/8178507/webrev.03/ <http://cr.openjdk.java.net/~aharlap/8178507/webrev.03/>
> Two issues:
> 
> 1. TestFullGCALot.java - it may take too long. So I added option -XX:+FullGCALotInterval=120 to make sure we do not hit timeout and do not slow down testing, also -XX:+IgnoreUnrecognizedVMOptions - do not fail in product mode
> 
It would be better to use requires to skip test for product bits. (vm.debug)
> 2. TestMemoryInitiazation.java - feature to initialize debug memory to some special words currently is supported only for CMS and Serial gc. So I modified Test to run now only for these gc's:
> 
>  * @requires vm.gc.Serial | vm.gc.ConcMarkSweep
>  * @summary Simple test for -XX:+CheckMemoryInitialization doesn't crash VM
>  * @run main/othervm -XX:+UseSerialGC -XX:+IgnoreUnrecognizedVMOptions -XX:+CheckMemoryInitialization TestMemoryInitialization
>  * @run main/othervm -XX:+UseConcMarkSweepGC -XX:+IgnoreUnrecognizedVMOptions -XX:+CheckMemoryInitialization TestMemoryInitialization
> 
Test tries to run VM 2 times with Serial/CMS GC in the case if any of them is supported or/and set. So test fails if CMS is not supported. In the case if any of GC is set explicitly it should fail with unsupported GC combinations.

The better would be to split test into 2 single tests TestMemoryInitializationSerialGC & TestMemoryInitializationCMSGC which shares java code. Also CMS has been deprecated in JDK9 so I don’t know it make a sense to test it JDK10.

Leonid
> I will add enhancement request to support CheckMemoryInitialization flag in G1.
> 
> Alex
> On 6/26/2017 7:04 PM, Igor Ignatyev wrote:
>> 
>>> On Jun 26, 2017, at 2:54 PM, Alexander Harlap <alexander.harlap at oracle.com <mailto:alexander.harlap at oracle.com>> wrote:
>>> 
>>> Thank you Igor and Leonid,
>>> 
>>> I fixed mentioned typos and unnecessary return (see http://cr.openjdk.java.net/~aharlap/8178507/webrev.02/ <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.02/>)
>>> 
>> perfect.
>>> Do I need more reviews?
>>> 
>> no, you can go ahead and integrate it.
>> 
>> -- Igor
>>> Alex
>>> 
>>> On 6/26/2017 4:32 PM, Igor Ignatyev wrote:
>>>> Hi Alexander,
>>>> 
>>>> besides the small nits which Leonid mentioned, there is one in http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html: <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html:> 
>>>>>   28  * @summary Test verifies only that VM doesnâ€™t crash but throw expected Error.  
>>>> I guess "doesnâ€™t" is 'doesn't' w/ a fancy apostrophe. otherwise looks good to me, Reviewed.
>>>> 
>>>> -- Igor
>>>> 
>>>>> On Jun 26, 2017, at 1:11 PM, Leonid Mesnik <leonid.mesnik at oracle.com <mailto:leonid.mesnik at oracle.com>> wrote:
>>>>> 
>>>>> Hi
>>>>> 
>>>>> New changes looks good for me. Please get review from Reviewer. 
>>>>> 
>>>>> The only 2 small nits which don’t require separate review from me:
>>>>> 
>>>>> http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/test/gc/TestFullGCALot.java.html <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/test/gc/TestFullGCALot.java.html> <http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/test/gc/TestFullGCALot.java.html <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/test/gc/TestFullGCALot.java.html>>
>>>>> typo in 
>>>>> 37 System.out.println("Hellow world!");
>>>>> 
>>>>> http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html> <http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/test/gc/TestStackOverflow.java.html>>
>>>>> return is not needed in
>>>>> 58 return;
>>>>> 
>>>>> Thanks 
>>>>> Leonid
>>>>>> On Jun 26, 2017, at 1:04 PM, Alexander Harlap <alexander.harlap at oracle.com <mailto:alexander.harlap at oracle.com>> wrote:
>>>>>> 
>>>>>> Hi Leonid,
>>>>>> 
>>>>>> I accommodated your suggestions.
>>>>>> 
>>>>>> New version of changeset located at http://cr.openjdk.java.net/~aharlap/8178507/webrev.01/ <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.01/>
>>>>>> 
>>>>>> 
>>>>>> Alex
>>>>>> 
>>>>>> 
>>>>>> On 6/23/2017 6:18 PM, Leonid Mesnik wrote:
>>>>>>> Hi
>>>>>>> 
>>>>>>> Basically changes looks good. Below are some comments:
>>>>>>> 
>>>>>>>> On Jun 22, 2017, at 9:16 AM, Alexander Harlap <alexander.harlap at oracle.com <mailto:alexander.harlap at oracle.com>> wrote:
>>>>>>>> 
>>>>>>>> Please review change for JDK-8178507 <https://bugs.openjdk.java.net/browse/JDK-8178507 <https://bugs.openjdk.java.net/browse/JDK-8178507>> - co-locate nsk.regression.gc tests
>>>>>>>> 
>>>>>>>> JDK-8178507 <https://bugs.openjdk.java.net/browse/JDK-8178507 <https://bugs.openjdk.java.net/browse/JDK-8178507>> is last remaining sub-task ofJDK-8178482 <https://bugs.openjdk.java.net/browse/JDK-8178482 <https://bugs.openjdk.java.net/browse/JDK-8178482>> - Co-locate remaining GC tests
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Proposed change located at http://cr.openjdk.java.net/~aharlap/8178507/webrev.00/ <http://cr.openjdk.java.net/%7Eaharlap/8178507/webrev.00/>
>>>>>>>> 
>>>>>>>> Co-located and converted to JTREG tests are:
>>>>>>>> 
>>>>>>>>   nsk/regression/b4187687 => hotspot/test/gc/TestFullGCALot.java
>>>>>>> The out variable is no used and return code is not checked in method “run”. Wouldn't it simpler just to move println into main and remove method ‘run’ completely?
>>>>>>>>   nsk/regression/b4396719 => hotspot/test/gc/TestStackOverflow.java
>>>>>>> The method ‘run’ always returns 0. It would be better to make it void or just remove it. Test never throws any exception. So it make a sense to write in comments that test verifies only that VM doesn’t crash but throw expected Error.
>>>>>>> 
>>>>>>>>   nsk/regression/b4668531 => hotspot/test/gc/TestMemoryInitialization.java
>>>>>>> The variable buffer is “read-only”. It make a sense to make variable ‘buffer' public static member of class TestMemoryInitialization. So compiler could not optimize it usage during any optimization like escape analysis.
>>>>>>>>   nsk/regression/b6186200 => hotspot/test/gc/cslocker/TestCSLocker.java
>>>>>>>> 
>>>>>>> Port looks good. It seems that test doesn’t verify that lock really happened. Could be this improved as a part of this fix or by filing separate RFE?
>>>>>>> 
>>>>>>> Leonid
>>>>>>>> Thank you,
>>>>>>>> 
>>>>>>>> Alex
>>>>>>>> 
>>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 