RFR: 8204210: Implementation: JEP 333: ZGC: A Scalable Low-Latency Garbage Collector (Experimental)

Mon Jun 4 09:35:19 UTC 2018

Hi guys,

On 2018-06-01 23:41, 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.
> 
I've looked at the shared GC parts and some other related code that I 
feel comfortable reviewing. The changes looks good in general, but a few 
comments:

src/hotspot/share/gc/shared/specialized_oop_closures.hpp
   39 #include "gc/z/zOopClosures.specialized.hpp"

Any good reason for not following the naming convention used by all 
other collectors? Same goes for zFlags.hpp in gc_globals.hpp.
---

src/hotspot/share/jfr/metadata/metadata.xml
  897   <Event name="ZThreadPhase" category="Java Virtual Machine, GC, 
Detailed" label="ZGC Thread Phase" thread="true">
  898     <Field type="uint" name="gcId" label="GC Identifier" 
relation="GcId"/>
  899     <Field type="string" name="name" label="Name" />
  900   </Event>
  901
  902   <Event name="ZStatisticsCounter" category="Java Virtual Machine, 
GC, Detailed" label="Z Statistics Counter" thread="true">
  903     <Field type="ZStatisticsCounter" name="id" label="Id" />
  904     <Field type="ulong" name="increment" label="Increment" />
  905     <Field type="ulong" name="value" label="Value" />
  906   </Event>
  907
  908   <Event name="ZStatisticsSampler" category="Java Virtual Machine, 
GC, Detailed" label="Z Statistics Sampler" thread="true">
  909     <Field type="ZStatisticsSampler" name="id" label="Id" />
  910     <Field type="ulong" name="value" label="Value" />
  911   </Event>
  912
  913   <Type name="ZStatCounterType" label="Z Stat Counter">
  914     <Field type="string" name="counter" label="Counter" />
  915   </Type>
  916
  917   <Type name="ZStatSamplerType" label="Z Stat Sampler">
  918     <Field type="string" name="sampler" label="Sampler" />
  919   </Type>

The fields in the events doesn't match the type definitions below, 
Statistics vs. Stat.
----

src/hotspot/share/oops/instanceRefKlass.inline.hpp
   61     if (type == REF_PHANTOM) {
   62       referent = HeapAccess<ON_PHANTOM_OOP_REF | 
AS_NO_KEEPALIVE>::oop_load(java_lang_ref_Reference::referent_addr_raw(obj));
   63     } else {
   64       referent = HeapAccess<ON_WEAK_OOP_REF | 
AS_NO_KEEPALIVE>::oop_load(java_lang_ref_Reference::referent_addr_raw(obj));
   65     }

Extract this to a load_referent() helper function to help readability.
---

src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp
src/hotspot/share/runtime/jniHandles.cpp
#if INCLUDE_ZGC
   // ...
   if (!UseZGC)
#endif

The UseZGC-flag is always available so the #if INCLUDE_ZGC can be 
skipped in the two places where this pattern is used.
---

Cheers,
Stefan

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