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

[Dmitry Dmitriev]

David,
I removed duplicated 
getErrorMessageCommandLine(value):http://cr.openjdk.java.net/~ddmitriev/8144578/webrev.03/ 
<http://cr.openjdk.java.net/%7Eddmitriev/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
>>>>>>
>>>>>
>>>>
>>