Hi Leonid, > I looked at the tests changes only. It seems that a some tests might > start failing if JDK built without Shenandoah GC. Also some test are not > going to be selected as expected. > > Unfortunately logic of '@requires' and @run in jtreg tests doesn't > always work well for specific cases of different GC selection. The > '@requires' tags are combined with '&' and whole test is selected or > not. Test always execute ALL @run actions so it fails if option > -XX:+UseShenandoahGC is not supported (not valid). The only way to split > 'run' actions is to add more @test with same sources. They could be in > the same file. > > See detailed info about jtreg tags > here:http://openjdk.java.net/jtreg/tag-spec.html > <http://openjdk.java.net/jtreg/tag-spec.html>
Thanks for pointing this out. I fixed all of what you pointed out (I think) and some more: http://cr.openjdk.java.net/~rkennke/fix-shared-tests/webrev.02/ It will show up in round2 of this review. Some more comments inline: > Could you please run your tests with with JDK which built without > Shenandoah GC. It helps to verify that Shenandoah-specific tests/runs > are not selected when this GC is not supported. I did now, and they are good (with above changes). > Also it would be nice > to verify that there are no any valid tests which became filtered out > with your patch. The running of all test suite with available but not > selected Shenandoah GC and enabled Graal also might help to verify > @requires settings. (It doesn't help when Shenandoah GCis not supported > though.) I'll see into running more combinations. So far I did hotspot_gc and a few others with and without Shenandoah. > I haven't looked at the tests in directory gc/shenandoah in details. But > all of them should be guarded with @requires. Placing them in separate > directory is not enough. See G1 tests as example: > > http://hg.openjdk.java.net/jdk/jdk/file/10c6e9066819/test/hotspot/jtreg/gc/g1/TestEagerReclaimHumongousRegions.java > <http://hg.openjdk.java.net/jdk/jdk/file/10c6e9066819/test/hotspot/jtreg/gc/g1/TestStringDeduplicationFullGC.java#l29> Right. We've done that now. > See more detailed comments about shared tests: > > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/TEST.groups.sdiff.html > > 219: tier2_gc_shenandoah = \ > > Usually tierN doesn't include tests from tierN-1. TierN is executed > after TierN-1 completed so no need to re-run the same tests. The > typical groups might looks like: > > tier1_gc_shenandoah = \ > gc/shenandoah/<tier1-tests> \ > other-tests > > tier2_gc_shenandoah = \ > gc/shenandoah/<tier2-tests>\ > -:tier1_gc_shenandoah > > tier3_gc_shenandoah = \ > gc/shenandoah/ \ //all-other-tests > -:tier2_gc_shenandoah We fixed that. > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/CriticalNativeArgs.java.html > > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/stress/CriticalNativeStress.java.html > > So your test will be skipped if any of -XX:+UseEpsilonGC or > -XX:+UseShenandoahGC is set. Also test might run only all run actions or > none of them. It would be better to split this test into 2 tests. So > epsilon tests might be executed if Shenandoah is absent. > > I think that (vm.bits == "64") is redundant in tag (you set x64 or arm64). > > Please use 4-space indentation in java code. All fixed. > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/TestFullGCCount.java.sdiff.html > > Even original requires seems confusing to me (but it works for CMS/G1 pair) > > 28 * @requires !(vm.gc.ConcMarkSweep & > vm.opt.ExplicitGCInvokesConcurrent == true) > > So currently test is executed if GC is CMS or default GC and > ExplicitGCInvokesConcurrent is not set to true. > > With your additional requirements 'vm.gc == "Shenandoah"' test is not > selected if ANY GC is set. Test doesn't set any GC itself so only > default GC might be tested. See [1]. > I split them and fixed the @requires to run with Shenandoah but only with -ExplicitGCInvokesConcurrent. > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/TestHumongousReferenceObject.java.sdiff.html > > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/TestSystemGC.java.sdiff.html > > Tests will always just fail if -XX:+UseShenandoahGC is not supported. > (invalid option) You might want to split test is it done for CMS GC. Done. > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/arguments/TestAlignmentToUseLargePages.java.sdiff.html > > I think > 56 * @requires vm.gc=="null" & !vm.graal.enabled > should be something like @requires vm.gc.Shenandoah & !vm.graal.enabled Yes. Fixed them. > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/arguments/TestUseCompressedOopsErgo.java.sdiff.html > > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/class_unloading/TestClassUnloadingDisabled.java.sdiff.html > > The same for 62 * @requires vm.gc=="null" & !vm.graal.enabled > and > 72 * @requires vm.gc=="null" & !vm.graal.enabled > Fixed them too. > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/ergonomics/TestDynamicNumberOfGCThreads.java.sdiff.html > > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/logging/TestGCId.java.sdiff.html > > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/metaspace/TestMetaspacePerfCounters.java.sdiff.html > > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/runtime/CompressedOops/UseCompressedOops.java.sdiff.html > > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/runtime/MemberName/MemberNameLeak.java.sdiff.html > > http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/test/hotspot/jtreg/gc/metaspace/TestMetaspacePerfCounters.java.sdiff.html > Also these tests are going to be run with all GC and fails if Shenandoah > is not supported. Right. I fixed those and a few others I have found. Those which drive Shenandoah at runtime now have a runtime check (GC.Shenandoah.isSupported() which uses WB). Others have split test sections. I'll upload round 2 of review changesets, which contains the fixes in a bit. Thanks a *LOT* for detailed review. Roman > Leonid > > [1] http://openjdk.java.net/jtreg/tag-spec.html > > On 11/26/18 1:39 PM, Roman Kennke wrote: >> >> Hi, >> >> This is the first round of changes for including Shenandoah GC into >> mainline. >> I divided the review into parts that roughly correspond to the mailing >> lists >> that would normally review it, and I divided it into 'shared' code >> changes and >> 'shenandoah' code changes (actually, mostly additions). The intend is >> to eventually >> push them as single 'combined' changeset, once reviewed. >> >> JEP: >> https://openjdk.java.net/jeps/189 >> Bug entry: >> >> https://bugs.openjdk.java.net/browse/JDK-8214259 >> >> Webrevs: >> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/ >> >> For those who want to see the full change, have a look at the >> shenandoah-complete >> <http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shenandoah-complete/> >> directory, >> it contains the full combined webrev. Alternatively, there is the file >> shenandoah-master.patch >> <http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shenandoah-master.patch>, >> which is what I intend to commit (and which should be equivalent to >> the 'shenandoah-complete' webrev). >> >> Sections to review (at this point) are the following: >> *) shenandoah-gc >> <http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shenandoah-gc/> >> - Actual Shenandoah implementation, almost completely residing in >> gc/shenandoah >> >> *) shared-gc >> <http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-gc/> >> - This is mostly boilerplate that is common to any GC >> - referenceProcessor.cpp has a little change to make one assert >> not fail (next to CMS and G1) >> - taskqueue.hpp has some small adjustments to enable subclassing >> >> *) shared-serviceability >> <http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-serviceability/> >> - The usual code to support another GC >> >> *) shared-runtime >> <http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-runtime/> >> - A number of friends declarations to allow Shenandoah iterators >> to hook up with, >> e.g. ClassLoaderData, CodeCache, etc >> - Warning and disabling JFR LeakProfiler >> - fieldDescriptor.hpp added is_stable() accessor, for use in >> Shenandoah C2 optimizations >> - Locks initialization in mutexLocker.cpp as usual >> - VM operations defines for Shenandoah's VM ops >> - globalDefinitions.hpp added UINT64_FORMAT_HEX_W for use in >> Shenandoah's logging >> - The usual macros in macro.hpp >> >> *) shared-build >> <http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-build/> >> - Add shenandoah feature, enabled by default, as agreed with >> Vladimir K. beforehand >> - Some flags for shenandoah-enabled compilation to get >> SUPPORT_BARRIER_ON_PRIMITIVES >> and SUPPORT_NOT_TO_SPACE_INVARIANT which is required for >> Shenandoah's barriers >> - --param inline-unit-growth=1000 settings for 2 shenandoah source >> files, which is >> useful to get the whole marking loop inlined (observed >> significant regression if we >> don't) >> >> *) shared-tests >> <http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shared-tests/> >> - Test infrastructure to support Shenandoah >> - Shenandoah test groups >> - Exclude Shenandoah in various tests that can be run with selected GC >> - Enable/add configure for Shenandoah for tests that make sense to >> run with it >> >> *) shenandoah-tests >> <http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/shenandoah-tests/> >> - Shenandoah specific tests, most reside in gc/shenandoah subdirectory >> - A couple of tests configurations have been added, e.g. >> TestGCBasherWithShenandoah.java >> >> I intentionally left out shared-compiler for now, because we have some >> work left to do >> there, but if you click around you'll find the patch anyway, in case >> you want to take >> a peek at it. >> >> We have regular builds on: >> - {Linux} x {x86_64, x86_32, armhf, aarch64, ppc64el, s390x} >> - {Windows} x {x86_64}, >> - {MacOS X} x {x86_64} >> >> This also routinely passes: >> - the new Shenandoah tests >> - jcstress with/without aggressive Shenandoah verification >> - specjvm2008 with/without aggressive Shenandoah verification >> >> >> I'd like to thank my collegues at Red Hat: Christine Flood, she >> deserves the credit for being the original inventor of Shenandoah, >> Aleksey Shiplëv, Roland Westrelin & Zhengyu Gu for their countless >> contributions, everybody else in Red Hat's OpenJDK team for testing, >> advice and support, my collegues in Oracle's GC, runtime and compiler >> teams for tirelessly helping with and reviewing all the GC interface >> and related changes, and of course the many early adopters for >> reporting bugs and success stories and feature requests: we wouldn't >> be here without any of you! >> >> Best regards, >> Roman >>
