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

Tue Jun 5 14:32:16 UTC 2018

Hi Roman,

A few comments have dropped in that we still haven't addressed, but 
we're working on those and will re-spin a webrev shortly.

Btw, tomorrow is a national holiday here in Sweden, so some of us might 
be off-line.

Thanks!

/Per

On 2018-06-05 16:03, Roman Kennke wrote:
> 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
> 
> 