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/hotspot-gc-dev/attachments/20181127/f1234d1d/signature.asc>


More information about the hotspot-gc-dev mailing list