RFR (round 1), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector
leonid.mesnik at oracle.com
leonid.mesnik at oracle.com
Wed Nov 28 21:23:58 UTC 2018
Hi
Thank you for your fixes and additional testing. I'll check new version
in the round-2.
Leonid
On 11/28/18 10:36 AM, Roman Kennke wrote:
> 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
>>>
More information about the build-dev
mailing list