RFR: 8204210: Implementation: JEP 333: ZGC: A Scalable Low-Latency Garbage Collector (Experimental)
Rickard Bäckman
rickard.backman at oracle.com
Thu Jun 7 06:10:56 UTC 2018
Per,
I agree with your comments. Consider it reviewed.
/R
On 06/06, Per Liden wrote:
> Hi Rickard,
>
> Thanks a lot for reviewing, much appreciated! Comments below.
>
> On 06/06/2018 11:48 AM, Rickard Bäckman wrote:
> > Hi,
> >
> > I've looked at the C2 parts of things with Nils by my side.
> > There are a couple of small things to note.
> >
> > classes.cpp misses an undef for optionalmacro.
>
> Will fix.
>
> >
> > compile.cpp the print_method should probably be within the {} of
> > macroExpand.
>
> Will fix.
>
> >
> > escape.cpp has two else if cases where the code looks very common.
> > Please make this into a function if possible?
>
> It would be possible, but I'm not sure it's a great idea in this case. The
> reason is that this seems to be the style in which these switch-statements
> are written here. Just looking at the case statements immediately above and
> below, they follow the same (duplication) pattern.
>
> In the first switch is looks like this:
>
> [...]
> case Op_Proj: {
> // we are only interested in the oop result projection from a call
> if (n->as_Proj()->_con == TypeFunc::Parms && n->in(0)->is_Call() &&
> n->in(0)->as_Call()->returns_pointer()) {
> add_local_var_and_edge(n, PointsToNode::NoEscape,
> n->in(0), delayed_worklist);
> }
> #if INCLUDE_ZGC
> else if (UseZGC) {
> if (n->as_Proj()->_con == LoadBarrierNode::Oop &&
> n->in(0)->is_LoadBarrier()) {
> add_local_var_and_edge(n, PointsToNode::NoEscape,
> n->in(0)->in(LoadBarrierNode::Oop), delayed_worklist);
> }
> }
> #endif
> break;
> }
> case Op_Rethrow: // Exception object escapes
> case Op_Return: {
> if (n->req() > TypeFunc::Parms &&
> igvn->type(n->in(TypeFunc::Parms))->isa_oopptr()) {
> // Treat Return value as LocalVar with GlobalEscape escape state.
> add_local_var_and_edge(n, PointsToNode::GlobalEscape,
> n->in(TypeFunc::Parms), delayed_worklist);
> }
> break;
> }
> [...]
>
> And in the second switch it looks like this:
>
> [...]
> case Op_Proj: {
> // we are only interested in the oop result projection from a call
> if (n->as_Proj()->_con == TypeFunc::Parms && n->in(0)->is_Call() &&
> n->in(0)->as_Call()->returns_pointer()) {
> add_local_var_and_edge(n, PointsToNode::NoEscape, n->in(0), NULL);
> break;
> }
> #if INCLUDE_ZGC
> else if (UseZGC) {
> if (n->as_Proj()->_con == LoadBarrierNode::Oop &&
> n->in(0)->is_LoadBarrier()) {
> add_local_var_and_edge(n, PointsToNode::NoEscape,
> n->in(0)->in(LoadBarrierNode::Oop), NULL);
> break;
> }
> }
> #endif
> ELSE_FAIL("Op_Proj");
> }
> case Op_Rethrow: // Exception object escapes
> case Op_Return: {
> if (n->req() > TypeFunc::Parms &&
> _igvn->type(n->in(TypeFunc::Parms))->isa_oopptr()) {
> // Treat Return value as LocalVar with GlobalEscape escape state.
> add_local_var_and_edge(n, PointsToNode::GlobalEscape,
> n->in(TypeFunc::Parms), NULL);
> break;
> }
> ELSE_FAIL("Op_Return");
> }
> [...]
>
> So it would maybe look a bit odd if we use a different style for the code we
> add, wouldn't you agree?
>
> Also, since our code is in #if INCLUDE_ZGC blocks, breaking this out would
> mean we would have to add a few more #if INCLUDE_ZGC blocks in hpp/cpp to
> protect the new function. So, unless you strongly object, I'd like to
> suggest that we keep it as is.
>
> >
> > opcodes.cpp misses an undef for optionalmacro.
>
> Will fix.
>
> >
> > In C2 in general, maybe BarrierSet::barrier_set()->barrier_set_c2()
> > coule be Compile::barrier_set()?
>
> I agree that these names are a bit long, but may I suggest that we don't do
> this as part of the ZGC patch? The reason is that there are already 21
> pre-existing calls to BarrierSet::barrier_set()->barrier_set_c2() in
> src/hotspot/share/opto code (we're adding 4 more in our patch). There are
> another ~70 calls to the
> BarrierSet::barrier_set()->barrier_set_{c1,assembler}() functions throughout
> compiler/asm-related code. While shortening these names might be a good
> idea, I'd prefer if that was handled separately from the ZGC patch. Makes
> sense?
>
> >
> > Looks good, great work everyone!
>
> Thanks! And again, thanks for reviewing!
>
> /Per
>
> >
> > /R
> >
> > On 06/01, Per Liden wrote:
> > > Hi,
> > >
> > > Please review the implementation of JEP 333: ZGC: A Scalable Low-Latency
> > > Garbage Collector (Experimental)
> > >
> > > Please see the JEP for more information about the project. The JEP is
> > > currently in state "Proposed to Target" for JDK 11.
> > >
> > > https://bugs.openjdk.java.net/browse/JDK-8197831
> > >
> > > Additional information in can also be found on the ZGC project wiki.
> > >
> > > https://wiki.openjdk.java.net/display/zgc/Main
> > >
> > >
> > > Webrevs
> > > -------
> > >
> > > To make this easier to review, we've divided the change into two webrevs.
> > >
> > > * ZGC Master: http://cr.openjdk.java.net/~pliden/8204210/webrev.0-master
> > >
> > > This patch contains the actual ZGC implementation, the new unit tests and
> > > other changes needed in HotSpot.
> > >
> > > * ZGC Testing: http://cr.openjdk.java.net/~pliden/8204210/webrev.0-testing
> > >
> > > This patch contains changes to existing tests needed by ZGC.
> > >
> > >
> > > Overview of Changes
> > > -------------------
> > >
> > > Below follows a list of the files we add/modify in the master patch, with a
> > > short summary describing each group.
> > >
> > > * Build support - Making ZGC an optional feature.
> > >
> > > make/autoconf/hotspot.m4
> > > make/hotspot/lib/JvmFeatures.gmk
> > > src/hotspot/share/utilities/macros.hpp
> > >
> > > * C2 AD file - Additions needed to generate ZGC load barriers (adlc does not
> > > currently offer a way to easily break this out).
> > >
> > > src/hotspot/cpu/x86/x86.ad
> > > src/hotspot/cpu/x86/x86_64.ad
> > >
> > > * C2 - Things that can't be easily abstracted out into ZGC specific code,
> > > most of which is guarded behind a #if INCLUDE_ZGC and/or if (UseZGC)
> > > condition. There should only be two logic changes (one in idealKit.cpp and
> > > one in node.cpp) that are still active when ZGC is disabled. We believe
> > > these are low risk changes and should not introduce any real change i
> > > behavior when using other GCs.
> > >
> > > src/hotspot/share/adlc/formssel.cpp
> > > src/hotspot/share/opto/*
> > > src/hotspot/share/compiler/compilerDirectives.hpp
> > >
> > > * General GC+Runtime - Registering ZGC as a collector.
> > >
> > > src/hotspot/share/gc/shared/*
> > > src/hotspot/share/runtime/vmStructs.cpp
> > > src/hotspot/share/runtime/vm_operations.hpp
> > > src/hotspot/share/prims/whitebox.cpp
> > >
> > > * GC thread local data - Increasing the size of data area by 32 bytes.
> > >
> > > src/hotspot/share/gc/shared/gcThreadLocalData.hpp
> > >
> > > * ZGC - The collector itself.
> > >
> > > src/hotspot/share/gc/z/*
> > > src/hotspot/cpu/x86/gc/z/*
> > > src/hotspot/os_cpu/linux_x86/gc/z/*
> > > test/hotspot/gtest/gc/z/*
> > >
> > > * JFR - Adding new event types.
> > >
> > > src/hotspot/share/jfr/*
> > > src/jdk.jfr/share/conf/jfr/*
> > >
> > > * Logging - Adding new log tags.
> > >
> > > src/hotspot/share/logging/*
> > >
> > > * Metaspace - Adding a friend declaration.
> > >
> > > src/hotspot/share/memory/metaspace.hpp
> > >
> > > * InstanceRefKlass - Adjustments for concurrent reference processing.
> > >
> > > src/hotspot/share/oops/instanceRefKlass.inline.hpp
> > >
> > > * vmSymbol - Disabled clone intrinsic for ZGC.
> > >
> > > src/hotspot/share/classfile/vmSymbols.cpp
> > >
> > > * Oop Verification - In four cases we disabled oop verification because it
> > > do not makes sense or is not applicable to a GC using load barriers.
> > >
> > > src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp
> > > src/hotspot/cpu/x86/stubGenerator_x86_64.cpp
> > > src/hotspot/share/compiler/oopMap.cpp
> > > src/hotspot/share/runtime/jniHandles.cpp
> > >
> > > * StackValue - Apply a load barrier in case of OSR. This is a bit of a hack.
> > > However, this will go away in the future, when we have the next iteration of
> > > C2's load barriers in place (aka "C2 late barrier insertion").
> > >
> > > src/hotspot/share/runtime/stackValue.cpp
> > >
> > > * JVMTI - Adding an assert() to catch problems if the tagmap hashing is
> > > changed in the future.
> > >
> > > src/hotspot/share/prims/jvmtiTagMap.cpp
> > >
> > > * Legal - Adding copyright/license for 3rd party hash function used in
> > > ZHash.
> > >
> > > src/java.base/share/legal/c-libutl.md
> > >
> > > * SA - Adding basic ZGC support.
> > >
> > > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/*
> > >
> > >
> > > Testing
> > > -------
> > >
> > > * Unit testing
> > >
> > > A number of new ZGC specific gtests have been added, in
> > > test/hotspot/gtest/gc/z/
> > >
> > > * Regression testing
> > >
> > > No new failures in Mach5, with ZGC enabled, tier{1,2,3,4,5,6}
> > > No new failures in Mach5, with ZGC disabled, tier{1,2,3}
> > >
> > > * Stress testing
> > >
> > > We have been continuously been running a number stress tests throughout
> > > the development, these include:
> > >
> > > specjbb2000
> > > specjbb2005
> > > specjbb2015
> > > specjvm98
> > > specjvm2008
> > > dacapo2009
> > > test/hotspot/jtreg/gc/stress/gcold
> > > test/hotspot/jtreg/gc/stress/systemgc
> > > test/hotspot/jtreg/gc/stress/gclocker
> > > test/hotspot/jtreg/gc/stress/gcbasher
> > > test/hotspot/jtreg/gc/stress/finalizer
> > > Kitchensink
> > >
> > >
> > > Thanks!
> > >
> > > /Per, Stefan & the ZGC team
More information about the hotspot-dev
mailing list