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

Tue Jun 5 11:37:52 UTC 2018

On 06/05/2018 09:21 AM, Per Liden wrote:> On 06/04/2018 03:47 PM, Erik 
Helin wrote:
>> 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

Looks good, thanks.

On 06/05/2018 09:21 AM, Per Liden wrote:
> On 06/04/2018 03:47 PM, Erik Helin wrote:
>> 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

Also good, thanks.

On 06/05/2018 09:21 AM, Per Liden wrote:
> On 06/04/2018 03:47 PM, Erik Helin wrote:
>> 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

Ok, lets tackle this in a separate patch, thanks for filing the RFE.

On 06/05/2018 09:21 AM, Per Liden wrote:
> On 06/04/2018 03:47 PM, Erik Helin wrote:
>> 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

Ah, alright, I thought it was a comment in the source code file. Thanks 
for reverting this part of the patch, we can discuss later if we can 
(should?) add a newline to that file.

On 06/05/2018 09:21 AM, Per Liden wrote:
> On 06/04/2018 03:47 PM, Erik Helin wrote:
>> 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

Looks good, thanks.

On 06/05/2018 09:21 AM, Per Liden wrote:
> On 06/04/2018 03:47 PM, Erik Helin wrote:
>> 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

Also good, thanks.

On 06/05/2018 09:21 AM, Per Liden wrote:
> On 06/04/2018 03:47 PM, Erik Helin wrote:
>> 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.

Hmm, ok, I see what you mean. I agree that for !UseZGC we should skip 
INCLUDE_ZGC guards and I see that you for the `if (UseZGC)` case. I 
would probably have skipped the guards even for this `if (UseZGC)` case, 
but I'm fine to leave them in.

On 06/05/2018 09:21 AM, Per Liden wrote:
> On 06/04/2018 03:47 PM, Erik Helin wrote:
>> 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

Good, thanks.

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

The updated version in your follow-up email looks good :)

On 06/05/2018 09:21 AM, Per Liden wrote:
> On 06/04/2018 03:47 PM, Erik Helin wrote:
>> 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

Also good, thanks.

The shared parts looks good to me now, consider those parts Reviewed by 
me (but don't count me as a formal reviewer for the C2 parts, someone 
with more C2 experience needs to look at those changes).

Thanks,
Erik