>> Hello Vladimir, >> >>> Why you need to include shenandoahBarrierSetC2.hpp in /opto/classes.cpp >>> ? Why not include only shenandoahSupport.hpp when new nodes are defined? >> >> I discussed this with my team. shenandoahBarrierSetC2.hpp is supposed to >> be the entry point for external C2 code. No C2 code is supposed to >> include shenandoahSupport.hpp, this is just an implementation detail of >> shenandoahBarrierSetC2.hpp. This seems symmetrical to how ZGC includes >> zBarrierSetC2.hpp from external C2 code. Is that ok to leave as it is? > > Yes, it is fine.
Ok great! Thanks! Can I consider the shared-compiler part reviewed by you then? Roman > > Vladimir > >> >> Cheers, >> Roman >> >> >>> >>> I think it is fine to not use #ifdef in loopopts.cpp when you check >>> is_ShenandoahBarrier(). And you don't do that in other files. >>> >>> Code in opto/macro.cpp is ugly but it is only the place so we can live >>> with it I think. >>> >>> Thanks, >>> Vladimir >>> >>> On 11/30/18 1: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 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 >>>>>>> >>>>>> >>>>> >>>> >>
signature.asc
Description: OpenPGP digital signature