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

Fri Jun 30 03:24:32 UTC 2017

Hi

The changes looks good. 

Leonid
> On Jun 29, 2017, at 11:08 AM, Alexander Harlap <alexander.harlap at oracle.com> wrote:
> 
> Here is new version: http://cr.openjdk.java.net/~aharlap/8178507/webrev.04/ <http://cr.openjdk.java.net/~aharlap/8178507/webrev.04/>
> I made changes , recommended by Leonid: vm.debug for debug-only options (very good!)
> and spitted  TestMemoryInitialization into two separate tests.
> Alex
> 
> 
> On 6/28/2017 5:35 PM, Leonid Mesnik wrote:
>> 
>>> On Jun 28, 2017, at 11:27 AM, Alexander Harlap <alexander.harlap at oracle.com <mailto: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/%7Eaharlap/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
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 