RFR: 8160088: update hotspot tests depending on GC to use @requires vm.gc.X
David Holmes
david.holmes at oracle.com
Tue Jun 28 04:26:19 UTC 2016
On 24/06/2016 7:52 PM, Dmitry Fazunenko wrote:
> 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.
Right - of course. Thanks for clarifying.
David
-----
> 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