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

Tue Jun 5 11:48:56 UTC 2018

On 06/05/2018 01:37 PM, Erik Helin wrote:
> 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 

Thanks for reviewing, Erik!

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

Rickard will be looking at the C2 parts (and maybe others too).

cheers,
Per