RFR: JDK-8220813: update hotspot tier1_gc tests depending on GC to use @requires vm.gc.X
Leonid Mesnik
leonid.mesnik at oracle.com
Wed Apr 3 19:56:59 UTC 2019
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.
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.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
> 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?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.
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 ofthe test is "@summary Test selection of GC when no GC
> > > > option isspecified", I think "@requires vm.gc.Serial &
> > > > vm.gc.G1" would be morereasonable. 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> wrote:
> > > > > > > Hi
> > > > > > >
> > > > > > > On Apr 1, 2019, at 9:26 PM, Igor Ignatyev <
> > > > > > > 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> wrote:
> > > > > > >
> > > > > > > Hi
> > > > > > >
> > > > > > > On Apr 1, 2019, at 8:53 PM, Igor Ignatyev <
> > > > > > > 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/TestDynamicNumberOfGCThr
> > > > > > > eads.java
> > > > > > > test/hotspot/jtreg/gc/ergonomics/TestInitialGCThreadLoggi
> > > > > > > ng.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>
> > > > > > > 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> 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>
> > > > > > > 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> 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/TestCMSClassUnloadi
> > > > > > > ngEnabledHWM.java
> > > > > > > test/hotspot/jtreg/gc/class_unloading/TestG1ClassUnloadin
> > > > > > > gHWM.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.ja
> > > > > > > va
> > > > > > > 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.jav
> > > > > > > a
> > > > > > > test/hotspot/jtreg/gc/startup_warnings/TestParallelScaven
> > > > > > > geSerialOld.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/TestAlignmentToUseLargePa
> > > > > > > ges.java.
> > > > > > > Sorry for these paste-and-copy errors.
> > > > > > > In
> > > > > > > test/hotspot/jtreg/gc/arguments/TestAlignmentToUseLargePa
> > > > > > > ges.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.j
> > > > > > > ava
> > > > > > > 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/TestAlignmentToUseLargePa
> > > > > > > ges.java
> > > > > > > test/hotspot/jtreg/gc/arguments/TestMaxNewSize.java
> > > > > > > test/hotspot/jtreg/gc/arguments/TestUseCompressedOopsErgo
> > > > > > > .java
> > > > > > > test/hotspot/jtreg/gc/class_unloading/TestClassUnloadingD
> > > > > > > isabled.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/TestDynamicNumberOfGCThr
> > > > > > > eads.java
> > > > > > > test/hotspot/jtreg/gc/ergonomics/TestInitialGCThreadLoggi
> > > > > > > ng.java
> > > > > > > test/hotspot/jtreg/gc/logging/TestGCId.java
> > > > > > >
> > > > > > > G4 list [4]:
> > > > > > > test/hotspot/jtreg/gc/arguments/TestParallelGCThreads.jav
> > > > > > > a
> > > > > > >
> > > > > > > 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> 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 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>
> > > > > > > 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/20190403/cace428d/attachment.htm>
More information about the hotspot-gc-dev
mailing list