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

Vladimir Kozlov vladimir.kozlov at oracle.com
Tue Nov 27 17:31:57 UTC 2018


On 11/27/18 1:37 AM, Roman Kennke wrote:
> Hi Vladimir,
> 
>> You need to check if shenandoahgc is disabled first - check
>> DISABLED_JVM_FEATURES list (see code for jvmci).
> 
> Why? If Shenandoah is disabled, it will be on this list, and therefore
> not be built (see JvmFeatures.gmk).

Did you test with --with-jvm-variants=-shenandoahgc to make sure it is disabled?

May be I needed explicitly check jvmci because I need it early to check it for enable Graal build. So it is different 
from your case.

> 
>> Do you support 32-bit x86? You have OPENJDK_TARGET_CPU == xx86 check.
>> Do you support all x86 OSs?
> 
> We can build with 32bit, but it will not run. It seems indeed curious to
> enable it. It's probably only there to allow 32bit builds with
> Shenandoah enabled, just to verify that we have all the relevant LP64
> checks in code in place. I will check it with my collegues.

What about OS? Do you support Windows, MacOS? I did not see any OS specific changes. So may be it is alright.

> 
>> Why you set VM CFLAGS when shenandoahgc is not enabled? It is in
>> JvmFeatures.gmk.
> 
> This *disables* and excludes Shenandoah if not enabled.
> 
> +ifneq ($(call check-jvm-feature, shenandoahgc), true)
> +  JVM_CFLAGS_FEATURES += -DINCLUDE_SHENANDOAHGC=0
> +  JVM_EXCLUDE_PATTERNS += gc/shenandoah
> 
> pretty much the same pattern as zgc and other features.
> 
> and then we add some flags when Shenandoah is enabled:
> 
> +else
> +  JVM_CFLAGS_FEATURES += -DSUPPORT_BARRIER_ON_PRIMITIVES
> -DSUPPORT_NOT_TO_SPACE_INVARIANT
> +endif
> 
> ... which are required for building with Shenandoah enabled.

My bad. Somehow I thought it was reverse. Too much coffee at the morning. :(
Looks good.

> 
>> I looked on C2 changes. It has INCLUDE_SHENANDOAHGC, checks and new gc
>> specific nodes. This looks intrusive. I hope you clean it up.
> 
> There are a few places with INCLUDE_SHENANDOAHGC checks where it seemed
> excessive to add a whole API just for a tiny, very GC specific check. We
> still do have intrusive changes in loop*, which we are working on to
> resolve. We declare+define all our GC specific nodes in
> shenandoahBarrierSetC2.hpp and related Shenandoah-specific files. The
> only things in shared code is the usual declarations in classes.hpp/cpp
> and node.hpp to get is_ShenandoahBarrier() an similar queries. This
> seems in-line with what other GCs do (look for e.g. LoadBarrier). Please
> be specific which changes in opto you'd like to see resolved (and don't
> look at loop* source files at this point ;-) ).

Is it possible to reuse LoadBarrier by adding GC specific flag to it?
I am not against adding new nodes if really needed. My concern is about using GC name in node's names.
Yes, I am fine with very few INCLUDE_SHENANDOAHGC.

Thanks,
Vladimir

> 
> Thanks for reviewing!
> Roman
> 
>> Thanks,
>> Vladimir
>>
>> On 11/26/18 2:47 PM, Erik Joelsson wrote:
>>> Build changes look ok to me.
>>>
>>> /Erik
>>>
>>> On 2018-11-26 13:39, 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