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

[Erik Helin]

On 06/01/2018 11:41 PM, Per Liden wrote:
> 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

First of all: great work! Thanks for pushing so many patches upstream 
ahead of this patch, that makes the review of this patch so much easier 
:) I have looked at all the shared changes, but I can't be counted as a 
reviewer for the C2 stuff, I don't have enough experience in that area. 
I can see what the build changes are doing, but Magnus and/or Erik 
should probably review that part. Ok, now for my comments:

Small nit in make/autoconf/hotspot.m4:

+  # Only enable ZGC on Linux x86

Could you please change the comment to say x86_64 or x64 (similar to 
other such comments in that file)? x86 is a bit ambiguous (could mean a 
32-bit x86 CPU).

Small nit in src/hotspot/share/compiler/oopMap.cpp:

+        if (ZGC_ONLY(!UseZGC &&)
+            ((((uintptr_t)loc & (sizeof(*loc)-1)) != 0) ||
+             !Universe::heap()->is_in_or_null(*loc))) {

Do we really need ZGC_ONLY around !UseZGC && here? The code is in an 
#ifdef ASSERT so it doesn't seem performance sensitive, and UseZGC will 
be just be false if ZGC isn't compiled, right? Or have I gotten this 
backwards?

Regarding src/hotspot/share/gc/shared/gcName.hpp, should we introduce a 
GCName class so that we can limit the scope of the Z och NA symbols? 
(Then GCNameHelper::to_string could also be moved into that class). 
Could also be done as a follow-up patch (if so, please file a bug).

Small nit in src/hotspot/share/jfr/metadata/metadata.xml:
-</Metadata>
\ No newline at end of file
+</Metadata>

Did you happen to add a newline here (I don't know why there should not 
be a newline, but the comment indicates so)?

Small nit in src/hotspot/share/opto/node.hpp:

    virtual       uint  ideal_reg() const;
+
  #ifndef PRODUCT

Was the extra newline here added intentionally?

In src/hotspot/share/prims/jvmtiTagMap.cpp, do you need to add an 
include of gc/z/zGlobals.hpp for ZAddressMetadataShift? Like

+#if INCLUDE_ZGC
+  #include "gc/z/c2/zGlobals.hpp"
+#endif

Or did I miss an include somewhere (wouldn't be the first time :)?

In src/hotspot/share/prims/whitebox.cpp, do we need the #if INCLUDE_ZGC 
guards or is `if (UseZGC)` enough?

Same comment for src/hotspot/share/runtime/jniHandles.cpp, do we need 
the #if INCLUDE_ZGC guard?

>    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

Again, great work here, particularly with upstreaming so many patches 
ahead of this one. I only have two small comments regarding the test 
changes:

Small nit in 
est/hotspot/jtreg/vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects001/referringObjects001.java:

+        // G1 fails, just like ZGC, if en explicitly GC is done here.

May I suggest s/en explicitly/an explicit/ ?
Also maybe remove the comment `// forceGC();`, because it might later 
look like your comment commented out an earlier, pre-existing call to 
forceGC().

Same comment as above for instances003.java, instances001.java, 
instanceCounts001.java.

In jdk/java/lang/management/MemoryMXBean/MemoryTestZGC.sh you probably 
want to remove "@bug     4530538", the empty "@summary" and "@author 
Mandy Chung"

Thanks,
Erik