RFR: 8160088: update hotspot tests depending on GC to use @requires vm.gc.X

[Dmitry Fazunenko]

Hi David,

On 24.06.2016 9:48, David Holmes wrote:
> Thanks for the info Dima! I may have missed it but I think this new 
> @requires functionality needs some broader advertising. :)

That functionality was introduced on this alias as well:
http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-April/022443.html
But I agree, "Implement setting jtreg @requires properties vm.flavor, 
vm.bits, vm.compMode" doesn't
attract much attention.
If I were from sales, I would write: "Braking news: jtreg introduces 
smart keywords"  :)

>
> I'm still not 100% sure what something like vm.gc.G1 means though. 
> Looking at this comment:
>
> /**
> * For all existing GC sets vm.gc.X property.
> * Example vm.gc.G1=true means:
> *    VM supports G1
> *    User either set G1 explicitely (-XX:+UseG1GC) or did not set any GC
>
> I think it would have been more accurate to add "and the selected GC 
> is G1" - right? 

No. vm.gc.G1 means:
The test needs to set G1 GC via -XX:+UseG1GC
It requires:
1) vm supports G1 (minimal VM doesn't)
2) User didn't say: -XX:+UseSerialGC  ,-XX:+UseParallelGC -XX:+UseCMSGC 
(otherwise it will be a flag conflict)
Note:  User may say: -XX:+UseG1GC

> That is what the logic seems to do (though I'm not sure why it checks 
> both current and currentSetByErgo ??)

There is a class of machines where ergonomics selects Serial GC if no 
other GC is specified explicitly.
For such machines currentGC might be "Serial" but setting G1 is still 
possible.
currentSetByErgo == false means: no other collector but currently 
selected is available for setting via -XX:UseXGC.

I know that the current description is hard to understand, but I didn't 
come up with a better wording.

>
> Anyway for this set of test changes everything seems fine.

Thanks a lot!

-- Dima

>
> Thanks,
> David
>
> On 23/06/2016 10:52 PM, Dmitry Fazunenko wrote:
>> David,
>>
>> On 23.06.2016 14:00, David Holmes wrote:
>>> Hi Dmitry,
>>>
>>> On 23/06/2016 6:52 PM, Dmitry Fazunenko wrote:
>>>> Hi David,
>>>>
>>>> thanks for looking!
>>>>
>>>> On 23.06.2016 3:17, David Holmes wrote:
>>>>> Hi Dmitry,
>>>>>
>>>>> On 23/06/2016 4:06 AM, Dmitry Fazunenko wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I'm looking for Reviewers for relatively simple fix which affects 59
>>>>>> tests.
>>>>>>   https://bugs.openjdk.java.net/browse/JDK-8160088
>>>>>> http://cr.openjdk.java.net/~dfazunen/8160088/webrev.00/
>>>>>>
>>>>>> The fix allows to skip execution of tests requiring a specific GC in
>>>>>> case of the required GC is not supported by VM.
>>>>>> Old variant:
>>>>>>   @requires vm.gc == null | vm.gc == "G1"
>>>>>>      A test will be executed if no GC is specified externally or
>>>>>> -XX:+UseG1GC flag is given.
>>>>>>      This test will be executed even if VM doesn't support G1 and
>>>>>> fail.
>>>>>> New variant:
>>>>>>   @requires vm.gc.G1
>>>>>>      This test will not be executed if VM doesn't support G1.
>>>>>
>>>>> That doesn't seem sufficient. What you want is that if the test
>>>>> requires G1 then it also supports G1, so it seems to me the correct
>>>>> formulation is:
>>>>>
>>>>> @requires (vm.gc == null |   // null -> default -> g1 (usually)
>>>>>            vm.gc == G1 ) &
>>>>>            vm.gc.G1
>>>>
>>>> No, vm.gc.G1 is an alias to: (vm.gc == null | vm.gc == g1) &
>>>> vm.gc.supportsG1.
>>>
>>> Wow - on the one hand that is a compact/succinct syntax. On the other
>>> hand - no one will recognize what it actually means! I think I have
>>> missed a discussed I would like to have given input on!
>>
>> It was discussed on hotspot-gc-dev alias as part of '8151283' review:
>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2016-June/018352.html 
>>
>>
>> Originally it was proposed names like: vm.gc.G1.acceptable
>> but after input from GC dev, vm.gc.G1 was selected.
>> Documentation how the properties are set what that mean is available in
>> the class source (jtreg plugin):
>>  <jdk-root>/test/jtreg-ext/requires/VMProps.java
>>
>>
>>>
>>>>>
>>>>> Aside: how does jtreg determine which GC's are supported?
>>>> Sorry for not providing the full history here:
>>>>
>>>> CODETOOLS-7901583: jtreg should provide extensible mechanism for
>>>> @requires properties
>>>> - change to jtreg that allows a Test Suite to introduce its own
>>>> @requires props
>>>>
>>>> JDK-8152432: Implement setting jtreg @requires properties vm.flavor,
>>>> vm.bits, vm.compMode
>>>> - implementation that mechanism in hotspot
>>>>
>>>> JDK-8154096: Extend WhiteBox API with methods which retrieve from VM
>>>> information about available GC
>>>> - fix which allows to know if a GC is supported
>>>>
>>>> JDK-8151283: Implement setting jtreg @requires property
>>>> vm.isG1Supported.
>>>> - introduction vm.gc.G1, vm.gc.Serial, vm.gc.Parallel and
>>>> vm.gc.ConcMarkSweep
>>>>
>>>> JDK-8160088 (this one): update hotspot tests depending on GC to use
>>>> @requires vm.gc.X
>>>> - culmination: update tests to use new functionality
>>>
>>> Wow I have missed out on a lot it seem! :(
>>>
>>> What version of jtreg supports this level of customized @requires?
>>> Someone has already encountered the following error:
>>>
>>> Error. Parse Exception: Syntax error in @requires expression: invalid
>>> name: XXX
>>>
>>> for a new @requires XXX clause.
>>
>> It was implemented in the final build of jtreg4.1 and of course it's
>> available in jtreg4.2
>> Quiting jtreg/doc/jtreg/tag-spec.html:
>>
>> |requires.extraPropDefns <source-files>|
>>
>>     This option is used to provide source files for classes that will be
>>     used at the beginning of each test suite run, to determine
>>     additional characteristics of the system for use with the
>>     |@requires| tag. Each class must implement
>>     |java.util.concurrent.Callable<java.util.Map<String, String>>|. When
>>     invoked, the |call()| method should return properties that can be
>>     referenced in an expression in a |@requires| tag. /Note:/ the names
>>     of the new properties that may be returned by calling these classes
>>     must also be declared in a |requires.properties| entry.
>>
>>     If this option is specified, the following additional options may
>>     also be specified:
>>
>>       * |requires.extraPropDefns.libs <source-files>| — source files for
>>         classes that will be put on the classpath when the primary
>>         classes are run.
>>       * |requires.extraPropDefns.bootlibs <source-files>| — source files
>>         for classes that will be put on the bootclasspath when the
>>         primary classes are run.
>>       * |requires.extraPropDefns.javacOpts <options>| — options that
>>         will be passed to javac when the source files are compiled.
>>       * |requires.extraPropDefns.vmOpts <options>| — options that will
>>         be passed to VM when the classes are executed.
>>
>>     In this family of options, if a source file is enclosed in square
>>     brackets, no error message will be given if the file is not 
>> available.
>>
>> The following properties may appear in either TEST.ROOT or any
>> TEST.properties file:
>>
>> The reason, why @requires XXX is not recognized by jtreg - XXX is not
>> registered in TEST.ROOT.
>> See: hotspot/test/TEST.ROOT for example on how to register new props.
>>
>> For the time being recently introduced requires properties are available
>> only in the hotspot repository.
>> To introduce them in jdk - a few lines from hotspot/test/TEST.ROOT
>> should be placed in jdk/test/TEST.ROOT
>> We haven't done this yet, because we are considering a possibility to
>> move VM specific tests from jdk into hotspot repo.
>>
>> Thanks,
>> Dima
>>
>>
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Dima
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Testing:
>>>>>> 1) starting jtreg with various collectors with "-c" option to verify
>>>>>> correctness of test descriptions.
>>>>>>     Number of selected tests before and after change is the same:
>>>>>> -XX:+UseG1GC:  1,456
>>>>>> -XX:+UseSerialGC: 1,366
>>>>>> -XX:+UseParallelGC: 1,369
>>>>>> -XX:+UseConcMarkSweepGC: 1,368
>>>>>> Default:  1,483; error
>>>>>>
>>>>>> 2) RBT (in progress)
>>>>>>
>>>>>> 3) Diff is analyzed manually (only necessary lines are affected):
>>>>>> #> hg diff |grep "^- " |sort -u
>>>>>> - * @requires (vm.gc == "G1" | vm.gc == "null")
>>>>>> - * @requires (vm.gc=="G1" | vm.gc=="null")
>>>>>> - * @requires vm.gc == "G1" | vm.gc == "null"
>>>>>> - * @requires vm.gc=="ConcMarkSweep" | vm.gc == "null"
>>>>>> - * @requires vm.gc=="ConcMarkSweep" | vm.gc=="null"
>>>>>> - * @requires vm.gc=="G1" | vm.gc =="null"
>>>>>> - * @requires vm.gc=="G1" | vm.gc=="null"
>>>>>> - * @requires vm.gc=="Parallel" | vm.gc=="null"
>>>>>> - * @requires vm.gc=="Serial" | vm.gc=="null"
>>>>>> - * @requires vm.gc=="null" | vm.gc=="G1"
>>>>>> - * Copyright (c) 2013, 2014, Oracle and/or its affiliates. All 
>>>>>> rights
>>>>>> reserved.
>>>>>> - * Copyright (c) 2013, 2015, Oracle and/or its affiliates. All 
>>>>>> rights
>>>>>> reserved.
>>>>>> - * Copyright (c) 2014, 2015, Oracle and/or its affiliates. All 
>>>>>> rights
>>>>>> reserved.
>>>>>> - * Copyright (c) 2015, Oracle and/or its affiliates. All rights
>>>>>> reserved.
>>>>>> #> hg diff |grep "^+ " |sort -u
>>>>>> + * @requires vm.gc.ConcMarkSweep
>>>>>> + * @requires vm.gc.G1
>>>>>> + * @requires vm.gc.Parallel
>>>>>> + * @requires vm.gc.Serial
>>>>>> + * Copyright (c) 2013, 2016, Oracle and/or its affiliates. All 
>>>>>> rights
>>>>>> reserved.
>>>>>> + * Copyright (c) 2014, 2016, Oracle and/or its affiliates. All 
>>>>>> rights
>>>>>> reserved.
>>>>>> + * Copyright (c) 2015, 2016, Oracle and/or its affiliates. All 
>>>>>> rights
>>>>>> reserved.
>>>>>>
>>>>>> Thanks,
>>>>>> Dima
>>>>
>>