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

[Dmitry Dmitriev]

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

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