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

[David Holmes]

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

---

JVMOptionsUtils.java

Is there not a more direct way to get the current GC argument from jtreg ?

---

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

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