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

[Dmitry Dmitriev]

David, thank you for the review!

Dmitry

On 15.02.2016 13:44, David Holmes wrote:
> On 15/02/2016 8:06 PM, 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 :) Shorter local variable names are okay.
>
> Cheers,
> David
>
>> 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>