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

[Dmitry Fazunenko]

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