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