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

[Roman Kennke]

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