RFR (round 1), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector
Roman Kennke
rkennke at redhat.com
Tue Nov 27 17:43:42 UTC 2018
Hi Vladimir,
> 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?
I tested with --with-jvm-features=-shenandoahgc (and will do so again
right after finishing this email, just to be sure). (Note: shenandoahgc
is a feature, not a variant).
> 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.
Probably.
>>> 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.
Shenandoah is OS agnostic and we compile + run it on Windows
successfully. Adopters tell us it's fine on MacOS too.
>>
>>> 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. :(
Yeah, maybe it's written the wrong way around (double-negation), but
that's how it is for other similar blocks too.
> Looks good.
Thanks!
>>> 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.
That would be very weird. Shenandoah's barrier is *not* a load barrier.
GC's names in node name makes sense (to me) because the generated code
is GC specific, and it's used in .ad files to match. I guess we could
give it a more generic names (ReadBarrier, WriteBarrier,
GCCompareAndSwap, ..) and then match them in .ad and call via
BarrierSetAssembler to generate GC specific code. But it seems odd and
hard to read+understand to me.
> Yes, I am fine with very few INCLUDE_SHENANDOAHGC.
Ok. Let's see how it looks like after Roland's latest changes are in.
> Thanks,
> Vladimir
Thanks for reviewing!
Roman
>> 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
>>>>>
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181127/f1234d1d/signature.asc>
More information about the serviceability-dev
mailing list