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

[Per Liden]

Hi Erik,

On 06/04/2018 03:47 PM, Erik Helin wrote:
> 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. 

Thanks a lot for reviewing!

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

Fixed.

http://hg.openjdk.java.net/zgc/zgc/rev/a81777811000

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

Fixed.

http://hg.openjdk.java.net/zgc/zgc/rev/3f6db622400c

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

I agree, filed an RFE.

https://bugs.openjdk.java.net/browse/JDK-8204324

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

The "No newline at end of file" comment is actually generated by hg diff 
and is not in the file itself. I think vim added it automatically, and I 
think we probably should have a new line there, but I'll revert it from 
this change.

http://hg.openjdk.java.net/zgc/zgc/rev/a8e1aec31efa

> 
> Small nit in src/hotspot/share/opto/node.hpp:
> 
>     virtual       uint  ideal_reg() const;
> +
>   #ifndef PRODUCT
> 
> Was the extra newline here added intentionally?

Fixed.

http://hg.openjdk.java.net/zgc/zgc/rev/6d6259917ded

> 
> 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 :)?

Fixed.

http://hg.openjdk.java.net/zgc/zgc/rev/b2e3b7c012af

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

This is certainly up for discussion, but the model I think we've been 
shooting for is that we don't have INCLUDE_ZGC only if there's a !UseZGC 
condition. Some of the "if (UseZGC)" then have ZGC specific code inside 
the scope, so you need the INCLUDE_ZGC anyway. In this particular case 
we don't have any ZGC specific code in the true path, but we might in 
the future.

This is the model we're trying to follow, but as I said, we can discuss 
if this is good or not.

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

Fixed this and another similar thing in c1_LIRAssembler_x86.cpp.

http://hg.openjdk.java.net/zgc/zgc/rev/2cf588273130

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

Fixed.

http://hg.openjdk.java.net/zgc/zgc/rev/42cd3b259870


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

Fixed.

http://hg.openjdk.java.net/zgc/zgc/rev/ff780fec8423


Thanks again for reviewing, Erik!

/Per

> 
> Thanks,
> Erik