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

[David Holmes]

Thanks for the info Dima! I may have missed it but I think this new 
@requires functionality needs some broader advertising. :)

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? That is what the logic seems to do (though I'm not sure why 
it checks both current and currentSetByErgo ??)

Anyway for this set of test changes everything seems fine.

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