Round 5 of Shenandoah review includes: - A fix for the @requires tag in TestFullGCCountTest.java. It should be correct now. We believe the CMS @requires was also not quite right and fixed it the same.
It reads now: Don't run this test if: - Actual GC set by harness is CMS *and* ExplicitGCInvokesConcurrent is true, as set by harness - Actual GC set by harness is Shenandoah *and* ExplicitGCInvokesConcurrent is not set false by harness (it's true by default in Shenandoah, so this needs to be double-inverteed). The @requires for CMS was wrong before (we think), because it would also filter defaultGC + ExplicitGCInvokesConcurrent. - Sorting of macros was fixed, as was pointed out by Per - Some stuff was added to SA, as suggested by Jini - Rebased on most current jdk/jdk code Webrevs: http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/05/ I also need reviews from GC reviewers for the CSR: https://bugs.openjdk.java.net/browse/JDK-8214349 I already got reviews for: [x] shared-runtime (coleenp) [x] shared-compiler (kvn) I got reviews for shared-build, but an earlier version, so maybe makes sense to look over this again. Erik J, Magnus? I still need approvals for: [ ] shared-build (kvn, erikj, ihse, pliden) [ ] shared-gc (pliden, kbarrett) [ ] shared-serviceability (jgeorge, pliden) [ ] shared-tests (lmesnik, pliden) [ ] shenandoah-gc [ ] shenandoah-tests Thanks for your patience and ongoing support! Cheers, Roman > 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 UINT64_FORMAT_HEX_W to UINT64_FORMAT_X_W > > 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 INCLUDE_SHENANDOAHGC 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 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 >>>> >>> >> >