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

[Alexander Harlap]

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

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

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
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>