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