RFR: JDK-8220813: update hotspot tier1_gc tests depending on GC to use @requires vm.gc.X
Leonid Mesnik
leonid.mesnik at oracle.com
Thu Mar 28 21:29:08 UTC 2019
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.
> test/hotspot/jtreg/gc/TestAgeOutput.java
Should not be ParallelGC also tested? Might be separate rfe for this
should be filed.
> 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
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.TestSystemGCYou added execution of
G1 + LargePages in "SerialGC" test? I think that it is enough to un
them only when G1 is tested.
> 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.
> 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 .
> 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> wrote:
> > Hi
> >
> >
> > On Mar 19, 2019, at 6:36 PM, Ao Qi <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> 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 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> 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
> >
> >
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20190328/69f9b1e1/attachment.htm>
More information about the hotspot-gc-dev
mailing list