RFR: 8144578: TestOptionsWithRanges test only ever uses the default collector

[sangheon]

Hi Dmitry,

webrev.03 looks good to me.

Thanks,
Sangheon


On 02/15/2016 02:06 AM, Dmitry Dmitriev wrote:
> David,
> I removed duplicated 
> getErrorMessageCommandLine(value):<http://cr.openjdk.java.net/%7Eddmitriev/8144578/webrev.03/>http://cr.openjdk.java.net/~ddmitriev/8144578/webrev.03/
>
> Thanks,
> Dmitry
>
> On 15.02.2016 12:59, David Holmes wrote:
>> On 15/02/2016 7:41 PM, Dmitry Dmitriev wrote:
>>> Hello David,
>>>
>>> Thank you for looking into this.
>>>
>>> On 15.02.2016 8:32, David Holmes wrote:
>>>> Hi Dmitry,
>>>>
>>>> On 11/02/2016 11:54 PM, Dmitry Dmitriev wrote:
>>>>> Hello,
>>>>>
>>>>> Please, need a Reviewer for that change.
>>>>> I uploaded updated webrev.02:
>>>>> http://cr.openjdk.java.net/~ddmitriev/8144578/webrev.02/
>>>>> <http://cr.openjdk.java.net/%7Eddmitriev/8144578/webrev.02/>
>>>>> Difference from webrev.01: I removed excluding of MinTLABSize and
>>>>> MarkSweepAlwaysCompactCount options from testing because underlying
>>>>> problems were fixed.
>>>>
>>>> JVMOption.java:
>>>>
>>>> The refactoring tended to obscure the primary change. But as you have
>>>> refactored could you also remove the duplicated call to
>>>> getErrorMessageCommandLine(value) please :)
>>> Yes, thanks. Will do!
>>>>
>>>> ---
>>>>
>>>> JVMOptionsUtils.java
>>>>
>>>> Is there not a more direct way to get the current GC argument from
>>>> jtreg ?
>>> The one of alternatives was to parse jtreg properties with command line
>>> options, but was decided not to depend on command line options in this
>>> case.
>>
>> Okay. Seems complicated but it is what it is.
>>
>>>>
>>>> ---
>>>>
>>>> JVMStartup.java
>>>>
>>>> Not sure what relevance the WeakRef usage and System.gc really has
>>>> here. Not sure why weakRef is volatile, nor createWeakRef is
>>>> synchronized ??
>>> The idea was to make this class slightly more complex(instead of using
>>> simple HelloWorld before) and therefore I use volatile and 
>>> synchronized.
>>> Also I used WeakRef and System.gc() to ensure that gc was happened.
>>
>> Okay. The details of the "test" don't really mastter.
>>
>> Thanks,
>> David
>>
>>> Thank you,
>>> Dmitry
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> Thanks,
>>>>> Dmitry
>>>>>
>>>>> On 14.01.2016 16:59, Dmitry Dmitriev wrote:
>>>>>> Hi Sangheon,
>>>>>>
>>>>>> Thank you for the review! Updated webrev:
>>>>>> http://cr.openjdk.java.net/~ddmitriev/8144578/webrev.01/
>>>>>> <http://cr.openjdk.java.net/%7Eddmitriev/8144578/webrev.01/>
>>>>>> Comments inline.
>>>>>>
>>>>>> On 13.01.2016 21:50, sangheon wrote:
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> Thank you for fixing this.
>>>>>>> Overall seems good.
>>>>>>>
>>>>>>> -------------------------------------------------------------------- 
>>>>>>>
>>>>>>> test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java 
>>>>>>>
>>>>>>>
>>>>>>> 87 /*
>>>>>>> 88 * JDK-8144578
>>>>>>> 89 * Temporarily remove testing of max range for 
>>>>>>> ParGCArrayScanChunk
>>>>>>> because
>>>>>>> 90 * JVM can hang when ParGCArrayScanChunk=4294967296 and 
>>>>>>> ParallelGC
>>>>>>> is used
>>>>>>> 91 */
>>>>>>> 92 excludeTestMaxRange("ParGCArrayScanChunk");
>>>>>>>
>>>>>>> issue number should be 8145204.
>>>>>> Fixed.
>>>>>>>
>>>>>>> -------------------------------------------------------------------- 
>>>>>>>
>>>>>>> test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/JVMOptionsUtils.java 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> line 181
>>>>>>>
>>>>>>> - if (name.startsWith("G1")) {
>>>>>>> - option.addPrepend("-XX:+UseG1GC");
>>>>>>> - }
>>>>>>> -
>>>>>>> - if (name.startsWith("CMS")) {
>>>>>>> - option.addPrepend("-XX:+UseConcMarkSweepGC");
>>>>>>> - }
>>>>>>> -
>>>>>>>
>>>>>>> Is this change really needed for dedicated gc flags(starting with
>>>>>>> "G1" or "CMS")?
>>>>>>> I thought this CR is targeted for non-dedicated gc flags such as
>>>>>>> TLABWasteIncrement.
>>>>>> I return deleted lines.
>>>>>>
>>>>>> Thanks,
>>>>>> Dmitry
>>>>>>>
>>>>>>> And if you still think that above lines should be removed, please
>>>>>>> remove line 224 as well.
>>>>>>>
>>>>>>>  224             case "NewSizeThreadIncrease":
>>>>>>>  225 option.addPrepend("-XX:+UseSerialGC");
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sangheon
>>>>>>>
>>>>>>>
>>>>>>> On 01/13/2016 09:11 AM, Dmitry Dmitriev wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Please review small enhancement to the command line option
>>>>>>>> validation test framework which allow to run test with 
>>>>>>>> different GCs.
>>>>>>>> Few comments:
>>>>>>>> 1) Code which executed for testing was moved from
>>>>>>>> JVMOptionsUtils.java to separate class(JVMStartup.java) to avoid
>>>>>>>> overhead at java start-up for determining vm and gc type.
>>>>>>>> 2) runJavaWithParam method in JVMOption.java was refactored to 
>>>>>>>> avoid
>>>>>>>> code duplication.
>>>>>>>>
>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8144578
>>>>>>>> webrev.00: 
>>>>>>>> http://cr.openjdk.java.net/~ddmitriev/8144578/webrev.00/
>>>>>>>> <http://cr.openjdk.java.net/%7Eddmitriev/8144578/webrev.00/>
>>>>>>>> Testing: tested on all platforms with different gc by RBT, failed
>>>>>>>> flags were temporary removed from testing in
>>>>>>>> TestOptionsWithRanges.java
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Dmitry
>>>>>>>
>>>>>>
>>>>>
>>>
>