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

Fri Jun 30 12:16:22 UTC 2017

Hi Igor,

Leonid helped me to polish tests.

Could you at final version?

Alex

On 6/29/2017 11:24 PM, Leonid Mesnik wrote:
> Hi
>
> The changes looks good.
>
> Leonid
>> On Jun 29, 2017, at 11:08 AM, Alexander Harlap 
>> <alexander.harlap at oracle.com <mailto:alexander.harlap at oracle.com>> wrote:
>>
>> 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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>