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

[David Holmes]

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