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

Thu Jun 29 18:08:40 UTC 2017

Here is new version: 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/
>>
>> 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/)
>>>>
>>> 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> - co-locate 
>>>>>>>>> nsk.regression.gc tests
>>>>>>>>>
>>>>>>>>> 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> - 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>