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

[leonid.mesnik at oracle.com]

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


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