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

Per Liden per.liden at oracle.com
Mon Dec 3 12:45:31 UTC 2018

Hi Roman,

On 11/30/18 10:00 PM, Roman Kennke wrote:

Hi all,

here comes round 4 of Shenandoah upstreaming review: This includes fixes for the issues that Per brought up: - Verify and gracefully reject dangerous flags combinations that disables required barriers - Revisited @requires filters in tests - Trim unused code from Shenandoah's SA impl - Move ShenandoahGCTracer to gc/shenandoah - Fix ordering of GC names in various files - Rename UINT64FORMATHEXW to UINT64FORMATXW

Thanks for fixing. Looks good to me, except it looks like you missed adjusting the macro order in the following files: src/hotspot/share/gc/shared/gc_globals.hpp src/hotspot/share/gc/shared/vmStructs_gc.hpp

cheers, Per

http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/04/ Thanks everybody for taking time to review this! Roman

Hello all,

Thanks so far for all the reviews and support! I forgot to update the 'round' yesterday. We are in round 3 now :-) Also, I fixed the numbering of my webrevs to match the review-round. ;-) Things we've changed today: - We moved shenandoah-specific code out of .ad files into our own .ad files under gc/shenandoah (in shenandoah-gc), how cool is that? This requires an addition in build machinery though, see make/hotspot/gensrc/GensrcAdlc.gmk (in shared-build). - Improved zero-disabling and build-code-simplification as suggested by Magnus and Per - Cleaned up some leftovers in C2 - Improved C2 loop opts code by introducing another APIs in BarrierSetC2. See the new APIs in shared-gc under BarrierSetC2.hpp. - I don't see where it makes sense to put INCLUDESHENANDOAHGC guards now. - We would all very much prefer to keep ShenandoahXYZNode names, as noted earlier. This stuff is Shenandoah-specific, so let's just call it that. - Rehashed Shenandoah tests (formatting, naming, directory layout, etc) - Rebased on jdk-12+22 - Question: let us know if you need separate RFE for the new BSC2 APIs? http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/03/ Thanks, Roman

Alright, we fixed: - The minor issues that Kim reported in shared-gc - A lot of fixes in shared-tests according to Leonid's review - Disabled SA heapdumping similar to ZGC as Per suggested

Some notes: Leonid: test/hotspot/jtreg/gc/TestFullGCCount.java was actually correct. The @requires there means to exclude runs with both CMS and ExplicitGCInvokesConcurrent at the same time, because that would be (expectedly) failing. It can run CMS, default GC and any other GC just fine. Adding the same clause for Shenandoah means the same, and filters the combination (+UseShenandoahGC)+(+ExplicitGCInvokesConcurrent). I made the condition a bit clearer by avoiding triple-negation. See: http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008457.html Per: Disabling the SA part for heapdumping makes 2 tests fail: - test/hotspot/jtreg/serviceability/sa/ClhsdbJhisto.java - test/hotspot/jtreg/serviceability/sa/TestHeapDumpForLargeArray.java we filter them for Shenandoah now. I'm wondering: how do you get past those with ZGC? See: http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008466.html (Note to Leonid and tests reviewers: I'll add those related filters in next round). Vladimir: Roland integrated a bunch of changes to make loop* code look better. I can tell that we're not done with C2 yet. Can you look over the code and see what is ok, and especially what is not ok, so that we can focus our efforts on the relevant parts? Updated set of webrevs: http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/01/ Thanks, Roman

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 isstable() 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 UINT64FORMATHEXW 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 SUPPORTBARRIERONPRIMITIVES and SUPPORTNOTTOSPACEINVARIANT 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 {x8664, x8632, armhf, aarch64, ppc64el, s390x} - {Windows} x {x8664}, - {MacOS X} x {x8664} 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