RFR (round 1), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector (original) (raw)

leonid.mesnik at oracle.com leonid.mesnik at oracle.com
Tue Nov 27 23:36:24 UTC 2018

Hi

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>

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.   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 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>

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/
 other-tests

tier2_gc_shenandoah =
   gc/shenandoah/
  -:tier1_gc_shenandoah

tier3_gc_shenandoah =
   gc/shenandoah/ \  //all-other-tests   -:tier2_gc_shenandoah

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.

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].

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.

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

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

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.

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 isstable() 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 UINT64FORMATHEXW 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 SUPPORTBARRIERONPRIMITIVES and SUPPORTNOTTOSPACEINVARIANT 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 {x8664, x8632, armhf, aarch64, ppc64el, s390x} - {Windows} x {x8664}, - {MacOS X} x {x8664} 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

More information about the build-dev mailing list