RFR: 8160088: update hotspot tests depending on GC to use @requires vm.gc.X
Dmitry Fazunenko
dmitry.fazunenko at oracle.com
Fri Jun 24 09:52:44 UTC 2016
David,
On 24.06.2016 12:20, David Holmes wrote:
> Hi Dima,
>
> This is diverging somewhat but ... :)
>
> On 24/06/2016 6:54 PM, Dmitry Fazunenko wrote:
>> 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'm more confused than I thought I was then. :(
>
> Let's pretend that default GC selection is random. If vm.gc == null we
> are letting the VM choose the GC by whatever means. If the test
> requires G1 then we need to know that currentGC() == G1 - if it is
> something else then we can't run this test. It doesn't seem to matter
> how the currentGC was selected as long as it is G1 - so I don't see
> why we need to query if the currentGC was selected by ergo ??
Okay :)
'vm.gc' is set by jtreg by parsing VM flags (jtreg is not aware of any
VM specific)
isSelectedByErgo is an implementation detail hidden from test developer.
I used it to avoid parsing VM flags and set vm.gc.X properly.
If I develop a test for G1 I'm going to specify -XX:+UseG1GC and I want
it has an effect.
Previously I used:
@requires vm.gc == null | vm.gc == "G1"
It protected from cases when jtreg was started with
vmoptins:"-XX:+UseSerialGC", but
didn't work if G1 wasn't supported: tests started and failed.
Now if I need such test I still need to specify -XX:+UseG1GC and to
protect it from
running in non applicable configurations I use:
@requires vm.gc.G1
So, developers should not care about how the current GC was selected.
We don't write tests that require a particular GC is by default, so
current GC for the VM under test is not exposed in the @requires properties.
All GC specific tests set GC explicitly. vm.gc.X is set to false, when
it's not possible.
Thanks,
Dima
>
> Cheers,
> David
>
>> 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
>>>>>>
>>>>
>>
More information about the hotspot-dev
mailing list