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 >>>>> >>
