RFR: JDK-8220813: update hotspot tier1_gc tests depending on GC to use @requires vm.gc.X
Per Liden
per.liden at oracle.com
Thu Apr 4 06:43:44 UTC 2019
Hi Leonid,
Thanks for your input Leonid, comment inline.
On 4/3/19 9:56 PM, Leonid Mesnik wrote:
> Hi
>
> The updating semantic of 'vm.gc.X' options is a huge change which is
> going to affect a lot of tests in all hotspot,svc, jfr and jmx areas.
> I think your propsal requires a much more of efforts and communications
> rather then problem in JDK-8220813 and proposed fix.
I agree. It's potentially a big effort. If we were to go in this
direction then it would be under a separate RFE.
>
> The major drawback which I see is that tier1 testing is significantly
> reduced by proposed changes. It covers only G1 and none of any other
> collectors as I understand.
That might certainly be true, but I'm thinking that can be compensated
for by changing the task-definitions for tier1, right?
> See more comments inline, mostly for 4):
>
>> Hmm, hold on a second. After looking at this again, I'm thinking this
>> isn't quite right after all. The meaning of the vm.gc/vm.gc.X properties
>> is really messy, and it's so easy to get this wrong. After some internal
>> discussions I think we might want to fix this once and for all.
>>
>> So, here's a proposal:
>>
>> 1) When executing a test, say test/hotspot/jtreg/gc/TestSystemGC.java,
>> we want it to be executed with the GC that was specified by the user,
>> and if no GC was specified the test should use the default GC.
>>
>> 2) We don't want a test that was spawned with say G1 as the GC, go off
>> and run tests with e.g. Serial. That just tends to waste CPU cycles, as
>> that testing will be covered when this tests is executed with Serial as
>> the selected GC.
> Basically I am fine with 1) and 2) it doesn't make a sense to run the
> same test with different GC.
> Sometimes ago it was decided that test should define it's options while
> sometime it would be better to only reject unsupported combinations.
>>
>> 3) We should completely ditch (or at least stop using the vm.gc
>> property). What was selected on the command-line is rarely interesting,
>> when matters is what GC is actually used.
>
> Do you mean that you are supposing to convert it to "new" vm.gc.X?
> Saying
> @requires vm.gc != "Z"
>
>
> to
> @requires !vm.gc.Z
Right. That's what we would require if the test should not be run with
ZGC, regardless of how ZGC was enabled, i.e. via the command-line or
because if was selected as the default by the VM (which doesn't happen
for ZGC today, but anyway...).
If a test for some reason requires different @run command for different
GCs, then we would have multiple @test/@requires/@run sections in to
deal with that. For example, assume some test needs a bigger heap when
running with ZGC, and a special option when running with G1, then it
would look something like this:
/*
* @test TestSomething
* @requires !vm.gc.Z & !vm.gc.G1
* @run main/othervm -Xmx64M gc.TestSomething
*/
/*
* @test TestSomething
* @requires vm.gc.G1
* @run main/othervm -Xmx64M -XX:+G1SpecialOption gc.TestSomething
*/
/*
* @test TestSomething
* @requires vm.gc.Z
* @run main/othervm -Xmx128M gc.TestSomething
*/
Makes sense?
>
>> 4) We should redefine the meaning of vm.gc.X, so that vm.gc.X is true
>> only when X is the selected GC, either explicitly (user chose to run the
>> test with X) or because no GC was explicitly selected and X was chosen
>> as the default GC by the VM.
> Does it mean that execution of all tests without any additional options
> doesn't cover any GC except default?
That would be a consequence of this. I'm thinking we control the test
coverage for different flag combinations on the task-definition level,
so I kind of think this is a good thing. But we there might be issues
with this that I'm not seeing here. So feel free to object and point out
those issues.
> They all are guarded by 'vm.gc.X'. So it means that tier1 testing is
> significantly reduced.
>
> As example: we are not going to test any non G1 GC while running JFR
> tests without any options? I don't think that it is something that expected.
As I mentioned above. I'm thinking we can adjust the task-definition for
tier1 in the case, to rotate in more GC combination if we want, no?
cheers,
Per
>
> Leonid
>>
>> I discussed this with a few people internally, and we think that this is
>> how we would like it to work. Of course, thinking through all use cases
>> is hard, so if people can spot serious problems with this approach, then
>> please speak up. I think this would simplify things quite a bit and
>> lower the risk of running the same test redundantly.
>>
>> Comments?
>>
>> If we agree that this sounds like a good approach I can volunteer to
>> take a stab at it and see how it works out.
>>
>
>> cheers,
>> Per
>>
>>>
>>> Cheers,
>>> Ao Qi
>>>
>>>>
>>>> /Per
>>>>
>>>>>>
>>>>>> Looks ok.
>>>>>>
>>>>>>> 2. about test/hotspot/jtreg/gc/arguments/TestSelectDefaultGC.java. I
>>>>>>> think the test will be skipped, when Serial or CMS is the default GC
>>>>>>> and G1 GC is not supported. If the G1 GC is supported and not the
>>>>>>> default, the test will be executed. Do you think this reasonable?
>>>>>>
>>>>>> I think you at least want to have "@requires vm.gc.Serial &
>>>>>> vm.gc.G1" in
>>>>>> there. Otherwise that test would fail is Serial isn't supported.
>>>>>
>>>>> This will skip the test if any GC is passed by parameter (e.g.
>>>>> -vmoptions:-XX:+UseG1GC). "@requires vm.gc.G1" will let
>>>>> "-vmoptions:-XX:+UseG1GC" work at least. However, since the purpose of
>>>>> the test is "@summary Test selection of GC when no GC option is
>>>>> specified", I think "@requires vm.gc.Serial & vm.gc.G1" would be more
>>>>> reasonable. I have updated:
>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.04/test/hotspot/jtreg/gc/arguments/TestSelectDefaultGC.java.udiff.html
>>>>> (webrev.04 only includes this change)
>>>>>
>>>>> Cheers,
>>>>> Ao Qi
>>>>>
>>>>>>
>>>>>> cheers,
>>>>>> Per
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Apr 2, 2019 at 12:41 PM Leonid Mesnik
>>>>>>> <leonid.mesnik at oracle.com <mailto:leonid.mesnik at oracle.com>> wrote:
>>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> On Apr 1, 2019, at 9:26 PM, Igor Ignatyev
>>>>>>>> <igor.ignatyev at oracle.com <mailto:igor.ignatyev at oracle.com>> wrote:
>>>>>>>>
>>>>>>>> Leonid,
>>>>>>>>
>>>>>>>> I understand that in some cases it might be appropriate to use
>>>>>>>> SkippedException (and TestDynamicNumberOfGCThreads.java might
>>>>>>>> be one of such case), but I don't see any reasons why
>>>>>>>> TestSmallHeap, for example, isn't run w/ Shenandoah GC or
>>>>>>>> Epsilon GC, and using SkippedException will just hide that fact.
>>>>>>>>
>>>>>>>> There might be different reasons. I don't know details of these
>>>>>>>> GC but might be they have different requirements
>>>>>>>> for expectedMaxHeap rather then 'pageSize * heapBytesPerCard'.
>>>>>>>> Why epsilongc should have same logic? It doesn't have any
>>>>>>>> generations and doesn't need any card table. It is a not test
>>>>>>>> bug to don't run with epsilonggc test in such case.
>>>>>>>>
>>>>>>>> I don't think that it makes a sense to change behavior of
>>>>>>>> existing tests to fail because some of GC are not supposed to be
>>>>>>>> tested. There are a lot of tests which are not executed with
>>>>>>>> ZGC,EpsilonGC, Shenandoah and it is a separate large issue.
>>>>>>>>
>>>>>>>> Leonid
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> -- Igor
>>>>>>>>
>>>>>>>> On Apr 1, 2019, at 9:06 PM, Leonid Mesnik
>>>>>>>> <leonid.mesnik at oracle.com <mailto:leonid.mesnik at oracle.com>> wrote:
>>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> On Apr 1, 2019, at 8:53 PM, Igor Ignatyev
>>>>>>>> <igor.ignatyev at oracle.com <mailto:igor.ignatyev at oracle.com>> wrote:
>>>>>>>>
>>>>>>>> Hi Ao,
>>>>>>>>
>>>>>>>> thank you a lot for taking care of this, in general it looks
>>>>>>>> good to me, I have 2 comments though:
>>>>>>>
>>>>>>> Thanks for your review, Igor.
>>>>>>>
>>>>>>>>
>>>>>>>> 1.
>>>>>>>> test/hotspot/jtreg/gc/TestSmallHeap.java
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestParallelRefProc.java
>>>>>>>> test/hotspot/jtreg/gc/ergonomics/TestDynamicNumberOfGCThreads.java
>>>>>>>> test/hotspot/jtreg/gc/ergonomics/TestInitialGCThreadLogging.java
>>>>>>>> test/hotspot/jtreg/gc/logging/TestMetaSpaceLog.java
>>>>>>>>
>>>>>>>> to me, noneGCSupported being true means we have a test bug, b/c
>>>>>>>> we don't cover all GCs, hence I'd prefer to throwing an
>>>>>>>> j.l.Error instead of jtreg.SkippedException.
>>>>>>>>
>>>>>>>> I asked Ao to add SkippedException, so I provide my comments and
>>>>>>>> reasons for this.
>>>>>>>> noneGCSupported means that we don't support any of GC supposed
>>>>>>>> to be testing
>>>>>>>> Test
>>>>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.02/test/hotspot/jtreg/gc/ergonomics/TestDynamicNumberOfGCThreads.java.udiff.html
>>>>>>>> supposed to test only GC with Parallel threads and not Serial
>>>>>>>> GC. So this test should be skipped when Serial GC (minimal VM)
>>>>>>>> only is supported but not fails.
>>>>>>>>
>>>>>>>> The same for other tests. Might be the name of variable
>>>>>>>> 'noneGCSupported' is confusing and should like
>>>>>>>> noGCselectedForTesting or something like this.
>>>>>>>>
>>>>>>>> Leonid
>>>>>>>>
>>>>>>>>
>>>>>>>> 2. test/hotspot/jtreg/gc/arguments/TestMinInitialErgonomics.java
>>>>>>>> as this test tests only Parallel GC, it would be nice to have it
>>>>>>>> reflected in either test name or @summary or in both.
>>>>>>>
>>>>>>> Done that. I added the info in @summary field. Please see:
>>>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.03/test/hotspot/jtreg/gc/arguments/TestMinInitialErgonomics.java.udiff.html
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Ao Qi
>>>>>>>
>>>>>>> [1]
>>>>>>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2019-April/025329.html
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> -- Igor
>>>>>>>>
>>>>>>>> On Apr 1, 2019, at 7:47 PM, Ao Qi <aoqi at loongson.cn
>>>>>>>> <mailto:aoqi at loongson.cn>> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Any other Reviewer(s)?
>>>>>>>>
>>>>>>>> Hi Leonid,
>>>>>>>>
>>>>>>>> Thank you very much!
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Ao Qi
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Apr 2, 2019 at 3:59 AM Leonid Mesnik
>>>>>>>> <leonid.mesnik at oracle.com <mailto:leonid.mesnik at oracle.com>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> Thank you, fix looks good for me now.
>>>>>>>>
>>>>>>>> Do you mean "none of GC was selected" or "none of GC was
>>>>>>>> supported"? I
>>>>>>>> have updated the test: if none of GC was supported, SkippedException
>>>>>>>> will be thrown.
>>>>>>>>
>>>>>>>>
>>>>>>>> I meant here that none of GC was selected by test logic, not set
>>>>>>>> by VM command-line. Your fix is fine.
>>>>>>>>
>>>>>>>> Leonid
>>>>>>>>
>>>>>>>> On Apr 1, 2019, at 5:21 AM, Ao Qi <aoqi at loongson.cn
>>>>>>>> <mailto:aoqi at loongson.cn>> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I updated the fix, please see:
>>>>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.02
>>>>>>>>
>>>>>>>> On Fri, Mar 29, 2019 at 5:29 AM Leonid Mesnik
>>>>>>>> <leonid.mesnik at oracle.com <mailto:leonid.mesnik at oracle.com>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> The overall changes looks much better now. See my comments inline.
>>>>>>>>
>>>>>>>> Please note that you need to get review approval from 'R'eviewer
>>>>>>>> from GC team.
>>>>>>>>
>>>>>>>> On Thu, 2019-03-28 at 18:43 +0800, Ao Qi wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I updated the fix, please see:
>>>>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.01
>>>>>>>>
>>>>>>>> I divided the modified tests into four groups, G1, G2, G3 and G4.
>>>>>>>>
>>>>>>>> G1: tests with minor fix of @requires field. G1 list [1]:
>>>>>>>> test/hotspot/jtreg/gc/TestNumWorkerOutput.java
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestMinInitialErgonomics.java
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestParallelHeapSizeFlags.java
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestSelectDefaultGC.java
>>>>>>>> test/hotspot/jtreg/gc/class_unloading/TestCMSClassUnloadingEnabledHWM.java
>>>>>>>> test/hotspot/jtreg/gc/class_unloading/TestG1ClassUnloadingHWM.java
>>>>>>>> test/hotspot/jtreg/gc/cms/GuardShrinkWarning.java
>>>>>>>> test/hotspot/jtreg/gc/g1/TestShrinkDefragmentedHeap.java
>>>>>>>> test/hotspot/jtreg/gc/logging/TestPrintReferences.java
>>>>>>>> test/hotspot/jtreg/gc/metaspace/TestMetaspaceCMSCancel.java
>>>>>>>> test/hotspot/jtreg/gc/parallel/AdaptiveGCBoundary.java
>>>>>>>> test/hotspot/jtreg/gc/startup_warnings/TestCMS.java
>>>>>>>> test/hotspot/jtreg/gc/startup_warnings/TestG1.java
>>>>>>>> test/hotspot/jtreg/gc/startup_warnings/TestParallelGC.java
>>>>>>>> test/hotspot/jtreg/gc/startup_warnings/TestParallelScavengeSerialOld.java
>>>>>>>>
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestSelectDefaultGC.java is a little
>>>>>>>> special. It assumes G1 is default for server class machines, and I
>>>>>>>> added @requires vm.gc.G1.
>>>>>>>>
>>>>>>>> Reasonalbe fix for now.
>>>>>>>>
>>>>>>>>
>>>>>>>> G2: tests were splited in @run field according to parameters to
>>>>>>>> cover
>>>>>>>> GCs separately. G2 list [2]:
>>>>>>>>
>>>>>>>> Could you please give different test names for these tests. Like
>>>>>>>> testG1, TestSerial.
>>>>>>>>
>>>>>>>>
>>>>>>>> Done that. Also fixed test/hotspot/jtreg/gc/TestNumWorkerOutput.java
>>>>>>>> in G1 (Group 1).
>>>>>>>>
>>>>>>>>
>>>>>>>> test/hotspot/jtreg/gc/TestAgeOutput.java
>>>>>>>>
>>>>>>>>
>>>>>>>> Should not be ParallelGC also tested? Might be separate rfe for
>>>>>>>> this should be filed.
>>>>>>>>
>>>>>>>>
>>>>>>>> No, I think. ".*GC\\(0\\) .*Age table with threshold.*" and
>>>>>>>> ".*GC\\(0\\) .*- age 1:.*" are printed in print_age_table, and I
>>>>>>>> think only SerialGC and G1GC will call it. I also tried to test
>>>>>>>> ParallelGC, and the test failed.
>>>>>>>>
>>>>>>>>
>>>>>>>> test/hotspot/jtreg/gc/TestGenerationPerfCounter.java
>>>>>>>> test/hotspot/jtreg/gc/TestHumongousReferenceObject.java
>>>>>>>>
>>>>>>>> Please slightly update comments where they become irrelevant.
>>>>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.01/test/hotspot/jtreg/gc/TestHumongousReferenceObject.java.udiff.html
>>>>>>>>
>>>>>>>>
>>>>>>>> Done that. Also fixed test/hotspot/jtreg/gc/TestAgeOutput.java and
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestAlignmentToUseLargePages.java.
>>>>>>>> Sorry for these paste-and-copy errors.
>>>>>>>> In
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestAlignmentToUseLargePages.java,
>>>>>>>> "@summary All parallel GC variants may use large pages without the
>>>>>>>> requirement that the" and "heap alignment is large page aligned.
>>>>>>>> Other
>>>>>>>> collectors also need to start up with odd sized heaps." should be in
>>>>>>>> one line, also fixed this.
>>>>>>>>
>>>>>>>>
>>>>>>>> 28 /*
>>>>>>>> 29 * @test
>>>>>>>> 30 * @summary Test that verifies that iteration over large,
>>>>>>>> plain Java objects, that potentially cross region boundaries on
>>>>>>>> G1, with references in them works.
>>>>>>>> 31 * @requires vm.gc.Parallel
>>>>>>>> 32 * @bug 8151499 8153734
>>>>>>>> 33 * @modules java.base/jdk.internal.vm.annotation
>>>>>>>> 34 * @run main/othervm -XX:+EnableContended
>>>>>>>> -XX:-RestrictContended -Xmx128m -XX:+UseParallelGC
>>>>>>>> -XX:ContendedPaddingWidth=8192 gc.TestHumongousReferenceObject
>>>>>>>> 35 */
>>>>>>>> 36
>>>>>>>>
>>>>>>>> test/hotspot/jtreg/gc/TestMemoryMXBeansAndPoolsPresence.java
>>>>>>>> test/hotspot/jtreg/gc/TestPolicyNamePerfCounter.java
>>>>>>>> test/hotspot/jtreg/gc/TestSystemGC.java
>>>>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.01/test/hotspot/jtreg/gc/TestSystemGC.java.udiff.html
>>>>>>>>
>>>>>>>> 33 * @run main/othervm -XX:+UseLargePages gc.TestSystemGC
>>>>>>>> 34 * @run main/othervm -XX:+UseLargePages
>>>>>>>> -XX:+UseLargePagesInMetaspace gc.TestSystemGC
>>>>>>>>
>>>>>>>> You added execution of G1 + LargePages in "SerialGC" test? I
>>>>>>>> think that it is enough to un them only when G1 is tested.
>>>>>>>>
>>>>>>>>
>>>>>>>> Fixed. Please see:
>>>>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.02/test/hotspot/jtreg/gc/TestSystemGC.java.udiff.html
>>>>>>>>
>>>>>>>>
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestAlignmentToUseLargePages.java
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestMaxNewSize.java
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestUseCompressedOopsErgo.java
>>>>>>>> test/hotspot/jtreg/gc/class_unloading/TestClassUnloadingDisabled.java
>>>>>>>> test/hotspot/jtreg/gc/metaspace/TestMetaspacePerfCounters.java
>>>>>>>>
>>>>>>>> G3 and G4: tests used GC.XX.isSupported() to cover GCs separately.
>>>>>>>> Tests in G3 include minor change, and tests in G4 do not.
>>>>>>>>
>>>>>>>> These tests should throw skipped expcetion if none of GC was
>>>>>>>> selected.
>>>>>>>>
>>>>>>>>
>>>>>>>> Do you mean "none of GC was selected" or "none of GC was
>>>>>>>> supported"? I
>>>>>>>> have updated the test: if none of GC was supported, SkippedException
>>>>>>>> will be thrown.
>>>>>>>>
>>>>>>>> I also updated the indent according to the test (two spaces or four
>>>>>>>> spaces, previous patch are all four spaces.).
>>>>>>>>
>>>>>>>>
>>>>>>>> G3 list [3]:
>>>>>>>> test/hotspot/jtreg/gc/TestSmallHeap.java
>>>>>>>> test/hotspot/jtreg/gc/ergonomics/TestDynamicNumberOfGCThreads.java
>>>>>>>> test/hotspot/jtreg/gc/ergonomics/TestInitialGCThreadLogging.java
>>>>>>>> test/hotspot/jtreg/gc/logging/TestGCId.java
>>>>>>>>
>>>>>>>> G4 list [4]:
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestParallelGCThreads.java
>>>>>>>>
>>>>>>>> 45 import java.util.List;
>>>>>>>>
>>>>>>>> 46 import java.util.ArrayList;
>>>>>>>>
>>>>>>>> Please organize imports in alphabetical order.
>>>>>>>>
>>>>>>>>
>>>>>>>> 95 String[] supported_gc = (String[]) list.toArray(new
>>>>>>>> String[0]);
>>>>>>>>
>>>>>>>> You don't need to convert list to array to iterate using for
>>>>>>>> (elem : lis). Also please use java naming convention like
>>>>>>>> supportedGC .
>>>>>>>>
>>>>>>>>
>>>>>>>> Fixed. Please see:
>>>>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.02/test/hotspot/jtreg/gc/arguments/TestParallelGCThreads.java.udiff.html
>>>>>>>>
>>>>>>>>
>>>>>>>> test/hotspot/jtreg/gc/arguments/TestParallelRefProc.java
>>>>>>>> test/hotspot/jtreg/gc/logging/TestMetaSpaceLog.java
>>>>>>>>
>>>>>>>> Test results:
>>>>>>>>
>>>>>>>> I tested the tests modified by this patch, using:
>>>>>>>> jtreg -jdk:${path}/jdk -dir:/home/aoqi/jdk/test/hotspot/jtreg/
>>>>>>>> -verbose:summary ${g1_list} ${g2_list} ${g3_list} ${g4_list}
>>>>>>>>
>>>>>>>> before fix:
>>>>>>>> javaoption pass fail error
>>>>>>>> defaut 49 0 0
>>>>>>>> -XX:+UseG1GC 14 0 0
>>>>>>>> -XX:+UseSerialGC 14 0 0
>>>>>>>> -XX:+UseParallelGC 14 0 0
>>>>>>>> -XX:+UseConcMarkSweepGC 14 0 0
>>>>>>>>
>>>>>>>> after fix:
>>>>>>>> javaoption pass fail error
>>>>>>>> defaut 69 0 0
>>>>>>>> -XX:+UseG1GC 23 0 0
>>>>>>>> -XX:+UseSerialGC 16 0 0
>>>>>>>> -XX:+UseParallelGC 21 0 0
>>>>>>>> -XX:+UseConcMarkSweepGC 21 0 0
>>>>>>>>
>>>>>>>> The differences are mainly because:
>>>>>>>> 1. Some tests are splited.
>>>>>>>> 2. @requires vm.gc=="null" => @requires vm.gc.XX, when using
>>>>>>>> -XX:+UseXXGC, more test will be executed after fix.
>>>>>>>> 3. Some tests used -XX:+UseXXGC in the test code, not in the @run
>>>>>>>> field. For example, gc/startup_warnings/TestG1.java will not be
>>>>>>>> executed when using -javaoption:-XX:+UseConcMarkSweepGC after
>>>>>>>> the fix.
>>>>>>>> It was executed when using -javaoption:-XX:+UseConcMarkSweepGC
>>>>>>>> before
>>>>>>>> the fix.
>>>>>>>>
>>>>>>>> I also did some test using jdk which doesn't support a
>>>>>>>> particular GC.
>>>>>>>> The method is using configure --with-jvm-features to disable a
>>>>>>>> particular GC.
>>>>>>>>
>>>>>>>> before (configure using --with-jvm-features):
>>>>>>>> pass fail error
>>>>>>>> default 49 0 0
>>>>>>>> -cmsgc 28 21 0
>>>>>>>> -epsilongc 49 0 0
>>>>>>>> -g1gc,-jfr 25 24 0
>>>>>>>> -parallelgc 27 22 0
>>>>>>>> -shenandoahgc 43 0 0
>>>>>>>> -zgc 49 0 0
>>>>>>>>
>>>>>>>> after (configure using --with-jvm-features):
>>>>>>>> pass fail error
>>>>>>>> default 69 0 0
>>>>>>>> -cmsgc 55 0 0
>>>>>>>> -epsilongc 69 0 0
>>>>>>>> -g1gc,-jfr 52 0 0
>>>>>>>> -parallelgc 54 0 0
>>>>>>>> -shenandoahgc 63 0 0
>>>>>>>> -zgc 69 0 0
>>>>>>>>
>>>>>>>> Some more comments are inline.
>>>>>>>>
>>>>>>>> On Wed, Mar 20, 2019 at 10:45 AM Leonid Mesnik
>>>>>>>> <leonid.mesnik at oracle.com <mailto:leonid.mesnik at oracle.com>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mar 19, 2019, at 6:36 PM, Ao Qi <aoqi at loongson.cn
>>>>>>>> <mailto:aoqi at loongson.cn>> wrote:
>>>>>>>>
>>>>>>>> Hi Leonid Mesnik,
>>>>>>>>
>>>>>>>> Thanks for you comments. Please see inline.
>>>>>>>>
>>>>>>>> See my answers inline.
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Mar 20, 2019 at 4:02 AM Leonid Mesnik
>>>>>>>> <leonid.mesnik at oracle.com <mailto:leonid.mesnik at oracle.com>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> Using 'requires vm.gc.XXX' is correct way to select supported
>>>>>>>> GC. However I think your fix is incomplete. Your fix skips tests
>>>>>>>> if any of tested GC is not supported. I think that it would be
>>>>>>>> better to test supported configurations rather then skip whole test.
>>>>>>>>
>>>>>>>> Let see test
>>>>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.00/test/hotspot/jtreg/gc/TestAgeOutput.java.udiff.html
>>>>>>>>
>>>>>>>> If G1 is not build then whole test is skipped. So it makes sense
>>>>>>>> to split following test
>>>>>>>>
>>>>>>>> 26 /*
>>>>>>>> 27 * @test TestAgeOutput
>>>>>>>> 28 * @bug 8164936
>>>>>>>> 29 * @summary Check that collectors using age table based aging
>>>>>>>> print an age table even for the first garbage collection
>>>>>>>> 30 * @key gc
>>>>>>>> 31 * @requires vm.gc.Serial & vm.gc.G1
>>>>>>>> 32 * @modules java.base/jdk.internal.misc
>>>>>>>> 33 * @library /test/lib
>>>>>>>> 34 * @build sun.hotspot.WhiteBox
>>>>>>>> 35 * @run driver ClassFileInstaller sun.hotspot.WhiteBox
>>>>>>>> 36 * @run main/othervm -XX:+UseSerialGC gc.TestAgeOutput
>>>>>>>> UseSerialGC
>>>>>>>> 37 * @run main/othervm -XX:+UseG1GC gc.TestAgeOutput UseG1GC
>>>>>>>> 38 */
>>>>>>>> 39
>>>>>>>>
>>>>>>>> into 2 tests to cover Serial/G1 GC separately.
>>>>>>>>
>>>>>>>> /*
>>>>>>>> * @test TestAgeOutput
>>>>>>>> * @bug 8164936
>>>>>>>> * @summary Check that collectors using age table based aging
>>>>>>>> print an age table even for the first garbage collection
>>>>>>>> * @key gc
>>>>>>>> * @requires vm.gc.Serial
>>>>>>>> ...
>>>>>>>> * @run main/othervm -XX:+UseSerialGC gc.TestAgeOutput UseSerialGC
>>>>>>>>
>>>>>>>> ...
>>>>>>>>
>>>>>>>> /*
>>>>>>>> * @test TestAgeOutput
>>>>>>>> * @bug 8164936
>>>>>>>> * @summary Check that collectors using age table based aging
>>>>>>>> print an age table even for the first garbage collection
>>>>>>>> * @key gc
>>>>>>>> * @requires vm.gc.G1
>>>>>>>> ...
>>>>>>>> * @run main/othervm -XX:+UseG1GC gc.TestAgeOutput UseG1GC
>>>>>>>> ...
>>>>>>>>
>>>>>>>> Good suggest. I will try that.
>>>>>>>>
>>>>>>>>
>>>>>>>> Done that. It is in G2.
>>>>>>>>
>>>>>>>>
>>>>>>>> The same for tests which execute VM with different arguments and
>>>>>>>> don't use external arguments. Test
>>>>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.00/test/hotspot/jtreg/gc/arguments/TestParallelRefProc.java.html
>>>>>>>> is not going to be executed if any of GC is not supported.
>>>>>>>>
>>>>>>>> Indeed, after the fix these tests would be skipped, but before
>>>>>>>> the fix
>>>>>>>> these tests would fail. I am not sure which is better. I think the
>>>>>>>> best way is also to split the test, but I cannot find a easy way
>>>>>>>> like
>>>>>>>> above. Could you give some hint?
>>>>>>>>
>>>>>>>> Tests might better reorganized like TestAgeOutput.java so VM
>>>>>>>> options are given as test parameters in @run or as vm parameters.
>>>>>>>> As another way you could check if GC is supported using
>>>>>>>> sun.hotspot.gc.GC.isSupported/isSelected and
>>>>>>>> throw SkippedException if tests should not be executed. The
>>>>>>>> example is checking Shenandoah in:
>>>>>>>> ./ergonomics/TestDynamicNumberOfGCThreads.java.
>>>>>>>>
>>>>>>>>
>>>>>>>> Done that. It is in G3 and G4.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> The another question is that before fix it was enough to run
>>>>>>>> tests with default GC and all supported GC to get full coverage
>>>>>>>> without significant duplication. But now it seems that a lot of
>>>>>>>> tests will be executed twice.
>>>>>>>> Might be after complete fix it would be enough to run tests with
>>>>>>>> selected GC only? Could you please provide some summary of test
>>>>>>>> coverage/duplication after your fix.
>>>>>>>>
>>>>>>>>
>>>>>>>> I didn't notice that (a lot of tests will be executed twice). Could
>>>>>>>> you give me one example?
>>>>>>>>
>>>>>>>> The test
>>>>>>>> http://cr.openjdk.java.net/~aoqi/8220813/webrev.00/test/hotspot/jtreg/gc/arguments/TestParallelRefProc.java.html
>>>>>>>> is example.
>>>>>>>> If you run all tests with default GC + with G1, CMS, Parallel &
>>>>>>>> Serial GC this test will be executed 5 times with all GC. While
>>>>>>>> previously it has been executed only once.
>>>>>>>>
>>>>>>>>
>>>>>>>> Sorry, I still don't understand very much. Does the problem still
>>>>>>>> exist in the new version of this patch?
>>>>>>>>
>>>>>>>> The idea is that currently it is enough to run all tests with
>>>>>>>> default GC + with specific GC (CMS for example) to cover this GC
>>>>>>>> completely. And some tests will be executed twice.
>>>>>>>> Really after yuor patch it should have been needed to run all GC
>>>>>>>> tests with specific GC to cover it completely.
>>>>>>>>
>>>>>>>>
>>>>>>>> I am afraid I still don't understand very much. I tried these before
>>>>>>>> and after the patch:
>>>>>>>>
>>>>>>>> jtreg -jdk:... -nativepath:... -verbose:summary,fail,error -c
>>>>>>>> :tier1_gc
>>>>>>>> jtreg -jdk:... -nativepath:... -verbose:summary,fail,error
>>>>>>>> -XX:+UseConcMarkSweepGC -c :tier1_gc
>>>>>>>>
>>>>>>>> I did not find unusual thing. Is my method right? How could I
>>>>>>>> reproduce the issue you mentioned?
>>>>>>>>
>>>>>>>> Thank you very much Leonid. You help me a lot!
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Ao Qi
>>>>>>>>
>>>>>>>>
>>>>>>>> I think that intention would be to test only specified GC and
>>>>>>>> some reasonable combination for default GC.
>>>>>>>>
>>>>>>>>
>>>>>>>> I am not sure if this fix is worth doing, so comments are welcome:)
>>>>>>>>
>>>>>>>> It certainly worth if anyone is going to build and test JDK
>>>>>>>> without Parallel/G1/CMS GC. The one example is minimal VM which
>>>>>>>> supports SerialGC only. I don't know anything about other examples.
>>>>>>>> Appropriate fix also might reduce required testing by better
>>>>>>>> test selection and improve test maintenance.
>>>>>>>> It might helps if you provide motivation for this fix as a part
>>>>>>>> of review.
>>>>>>>>
>>>>>>>>
>>>>>>>> The initial motivation I fix this is because I am porting jdk to
>>>>>>>> mips
>>>>>>>> and the default GC of this mips port is not G1 (G1 crashes) at
>>>>>>>> present. When I did jtreg test, some tests failed because G1 is not
>>>>>>>> supported, or even is not default. I have to find which failure is
>>>>>>>> because of G1 and which is not every time.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thank you for explanation.
>>>>>>>>
>>>>>>>> Leonid
>>>>>>>>
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Ao Qi
>>>>>>>>
>>>>>>>> [1] http://cr.openjdk.java.net/~aoqi/8220813/01/g1
>>>>>>>> [2] http://cr.openjdk.java.net/~aoqi/8220813/01/g2
>>>>>>>> [3] http://cr.openjdk.java.net/~aoqi/8220813/01/g3
>>>>>>>> [4] http://cr.openjdk.java.net/~aoqi/8220813/01/g4
>>>>>>>>
>>>>>>>> Leonid
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Ao Qi
>>>>>>>>
>>>>>>>> Leonid
>>>>>>>>
>>>>>>>> On Mar 19, 2019, at 2:48 AM, Ao Qi <aoqi at loongson.cn
>>>>>>>> <mailto:aoqi at loongson.cn>> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8220813
>>>>>>>> Webrev: http://cr.openjdk.java.net/~aoqi/8220813/webrev.00/
>>>>>>>>
>>>>>>>> If a particular GC is not supported, some hotspot tier1_gc tests
>>>>>>>> fails. The patch allows tests to be skipped on platforms where a
>>>>>>>> GC is
>>>>>>>> not supported.
>>>>>>>>
>>>>>>>> Tested:
>>>>>>>>
>>>>>>>> before (configure using --with-jvm-features):
>>>>>>>> configure TOTAL PASS FAIL ERROR
>>>>>>>> default 251 251 0 0
>>>>>>>> -cmsgc 243 222 21 0
>>>>>>>> -epsilongc 229 229 0 0
>>>>>>>> -g1gc,-jfr 181 157 24 0
>>>>>>>> -parallelgc 241 219 22 0
>>>>>>>> -shenandoahgc 206 206 0 0
>>>>>>>> -zgc 251 251 0 0
>>>>>>>>
>>>>>>>> after (configure using --with-jvm-features):
>>>>>>>> configure TOTAL PASS FAIL ERROR
>>>>>>>> default 251 251 0 0
>>>>>>>> -cmsgc 222 222 0 0
>>>>>>>> -epsilongc 229 229 0 0
>>>>>>>> -g1gc,-jfr 157 157 0 0
>>>>>>>> -parallelgc 219 219 0 0
>>>>>>>> -shenandoahgc 206 206 0 0
>>>>>>>> -zgc 251 251 0 0
>>>>>>>>
>>>>>>>> before:
>>>>>>>> VM_OPTIONS TOTAL PASS FAIL ERROR
>>>>>>>> default 251 251 0 0
>>>>>>>> -XX:+UseG1GC 129 129 0 0
>>>>>>>> -XX:+UseSerialGC 65 65 0 0
>>>>>>>> -XX:+UseParallelGC 69 69 0 0
>>>>>>>> -XX:+UseConcMarkSweepGC 67 67 0 0
>>>>>>>>
>>>>>>>> after:
>>>>>>>> VM_OPTIONS TOTAL PASS FAIL ERROR
>>>>>>>> default 251 251 0 0
>>>>>>>> -XX:+UseG1GC 121 121 0 0
>>>>>>>> -XX:+UseSerialGC 51 51 0 0
>>>>>>>> -XX:+UseParallelGC 60 60 0 0
>>>>>>>> -XX:+UseConcMarkSweepGC 66 66 0 0
>>>>>>>>
>>>>>>>> The difference in VM_OPTIONS tests mainly comes from two reasons:
>>>>>>>> 1. This patch make this kind of change:
>>>>>>>>
>>>>>>>> @requires vm.gc=="null" & !vm.graal.enabled
>>>>>>>> =>
>>>>>>>> @requires vm.gc.ConcMarkSweep & !vm.graal.enabled
>>>>>>>>
>>>>>>>> Take gc/arguments/TestAlignmentToUseLargePages.java#id1 for example.
>>>>>>>> Before the patch, even if VM_OPTIONS=-XX:+UseConcMarkSweepGC is set,
>>>>>>>> gc/arguments/TestAlignmentToUseLargePages.java#id1 will not be
>>>>>>>> executed (but the test is intended to test -XX:+UseConcMarkSweepGC).
>>>>>>>> After the patch, the test will be executed if
>>>>>>>> VM_OPTIONS=-XX:+UseConcMarkSweepGC is set.
>>>>>>>>
>>>>>>>> 2. If a test use a particular GC in the code (not in the jtreg tag
>>>>>>>> fields) and another GC is passed by -XX:UseXGC, the test will be
>>>>>>>> skipped.
>>>>>>>> For example, gc/startup_warnings/TestG1.java uses -XX:+UseG1GC
>>>>>>>> in the
>>>>>>>> main method. This test will be skipped, when
>>>>>>>> VM_OPTIONS=-XX:+UseConcMarkSweepGC is used. Before this patch,
>>>>>>>> it will
>>>>>>>> be executed.
>>>>>>>>
>>>>>>>> I am doing more tests and checking test results above. Could someone
>>>>>>>> give some advice on this change?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Ao Qi
>>>>>>>>
>>>>>>>>
>>>>>>>>
More information about the hotspot-gc-dev
mailing list