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 Apr 4 19:20:02 UTC 2019
The proposal looks good once we could just run gc(ons some other) tests
with different VM flags.
Does it make a sense to complete this RFE now or withdrwan it and go
with new proposal directly?
Leonid
On Thu, 2019-04-04 at 08:43 +0200, Per Liden wrote:
> 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 ishow we would like it to work. Of course, thinking through
> > > all use casesis hard, so if people can spot serious problems with
> > > this approach, thenplease speak up. I think this would simplify
> > > things quite a bit andlower the risk of running the same test
> > > redundantly.
> > > Comments?
> > > If we agree that this sounds like a good approach I can volunteer
> > > totake 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.jav
> > > > > > > > a. 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 <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.j
> > > > > > > > > ava
> > > > > > > > > test/hotspot/jtreg/gc/ergonomics/TestDynamicNumberOfG
> > > > > > > > > CThreads.java
> > > > > > > > > test/hotspot/jtreg/gc/ergonomics/TestInitialGCThreadL
> > > > > > > > > ogging.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/TestMinInitialErgonom
> > > > > > > > > ics.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/TestMinInitialErgonom
> > > > > > > > > ics.java
> > > > > > > > > test/hotspot/jtreg/gc/arguments/TestParallelHeapSizeF
> > > > > > > > > lags.java
> > > > > > > > > test/hotspot/jtreg/gc/arguments/TestSelectDefaultGC.j
> > > > > > > > > ava
> > > > > > > > > test/hotspot/jtreg/gc/class_unloading/TestCMSClassUnl
> > > > > > > > > oadingEnabledHWM.java
> > > > > > > > > test/hotspot/jtreg/gc/class_unloading/TestG1ClassUnlo
> > > > > > > > > adingHWM.java
> > > > > > > > > test/hotspot/jtreg/gc/cms/GuardShrinkWarning.java
> > > > > > > > > test/hotspot/jtreg/gc/g1/TestShrinkDefragmentedHeap.j
> > > > > > > > > ava
> > > > > > > > > test/hotspot/jtreg/gc/logging/TestPrintReferences.jav
> > > > > > > > > a
> > > > > > > > > test/hotspot/jtreg/gc/metaspace/TestMetaspaceCMSCance
> > > > > > > > > l.java
> > > > > > > > > test/hotspot/jtreg/gc/parallel/AdaptiveGCBoundary.jav
> > > > > > > > > a
> > > > > > > > > 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/TestParallelSc
> > > > > > > > > avengeSerialOld.java
> > > > > > > > >
> > > > > > > > > test/hotspot/jtreg/gc/arguments/TestSelectDefaultGC.j
> > > > > > > > > ava 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.ja
> > > > > > > > > va
> > > > > > > > >
> > > > > > > > > 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/TestAlignmentToUseLar
> > > > > > > > > gePages.java.
> > > > > > > > > Sorry for these paste-and-copy errors.
> > > > > > > > > In
> > > > > > > > > test/hotspot/jtreg/gc/arguments/TestAlignmentToUseLar
> > > > > > > > > gePages.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/TestMemoryMXBeansAndPoolsPresen
> > > > > > > > > ce.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/TestAlignmentToUseLar
> > > > > > > > > gePages.java
> > > > > > > > > test/hotspot/jtreg/gc/arguments/TestMaxNewSize.java
> > > > > > > > > test/hotspot/jtreg/gc/arguments/TestUseCompressedOops
> > > > > > > > > Ergo.java
> > > > > > > > > test/hotspot/jtreg/gc/class_unloading/TestClassUnload
> > > > > > > > > ingDisabled.java
> > > > > > > > > test/hotspot/jtreg/gc/metaspace/TestMetaspacePerfCoun
> > > > > > > > > ters.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/TestDynamicNumberOfG
> > > > > > > > > CThreads.java
> > > > > > > > > test/hotspot/jtreg/gc/ergonomics/TestInitialGCThreadL
> > > > > > > > > ogging.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.j
> > > > > > > > > ava
> > > > > > > > > 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 f
> > > > > > > > > ail 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 f
> > > > > > > > > ail 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
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20190404/0b43aa01/attachment.htm>
More information about the hotspot-gc-dev
mailing list