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

Tue Jun 5 14:03:00 UTC 2018

Hi Per and all,

I would like to review the changeset(s), but I see that many issues have
already been addressed. Would it be possible to post an updated webrev,
so that I see the most up-to-date version?

Thanks,
Roman

> 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