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

[Per Liden]

Hi Roman,

On 06/06/2018 04:36 PM, Roman Kennke wrote:
> Am 06.06.2018 um 16:16 schrieb Per Liden:
>> Hi Roman,
>>
>> On 06/06/2018 11:32 AM, Roman Kennke wrote:
>>> I'm looking mostly at shared code changes. I can't really say much about
>>> C2, JFR, SA, tests and ZGC itself.
>>>
>>> Some comments/questions:
>>>
>>> - src/hotspot/share/classfile/vmSymbols.cpp:
>>>    why are you enabling the clone intrinsic unconditionally and not under
>>> the usual:
>>> if (!InlineObjectCopy || !InlineArrayCopy) return true;
>>> ?
>>
>> Please note that the function is called is_disabled_by_flags(). So we're
>> actually disabling (not enabling) it unconditionally for ZGC.
> 
> Ah. Confusing double-negation. Might want to turn the whole thing around
> (not here). Also, I wonder if GCs might want to have a say about which
> intrinsics to enable/disable generally. Probably not very important.

I would kind of think that all GCs want to support all intrinsics, with 
the exception (like in our case) of some being temporarily disabled 
until the proper support is in place.

> 
>>> - src/hotspot/share/oops/instanceRefKlass.inline.hpp
>>> I wonder if this makes sense to upstream separately? Also, I'm curious
>>
>> Yes, I think you're right. I don't see a reason why this couldn't be
>> upstreamed separately.
>>
>> I filed https://bugs.openjdk.java.net/browse/JDK-8204474 and sent an RFR
>> to hotspot-dev.
> 
> Thanks!
> 
> 
>>> why we need to distinguish between weak and phantom?
>>
>> Generally speaking, we should always annotate oop accesses with the
>> proper strengths, which is why we distinguish between weak and phantom
>> here. For ZGC, in this very specific case, we will deep down in the
>> barrier code eventually do the same thing in both cases. However, such
>> decisions should be made by the GC/BarrierSet and not the access
>> call-site. A different GC might want to make a different decision.
>>
>> An alternative would be to use ON_UNKNOWN_OOP_REF, which tells the
>> BarrierSet that is needs to apply additional logic to figure out what
>> the strength really is. However, this comes with a run-time overhead,
>> and since we already have the type in try_discover() we can use that to
>> do the proper access and avoid that overhead.
> 
> Ok, good. I was only asking out of curiousity.
> 
>>> - src/hotspot/share/runtime/stackValue.cpp
>>> There's no reasonable way to abstract this via GC interface?
>>
>> This code is there to solve a very ZGC specific issue, which is that a
>> deopt can happens between a load and its load barrier. As I mentioned in
>> the initial RFR mail, this will go away in our next iteration of our C2
>> load barriers [1], which will by design make sure that this can't
>> happen. We therefore didn't think it was worth the effort to create a
>> abstraction for this, since it's highly ZGC specific and such an
>> abstraction would become useless/unused pretty soon anyway.
> 
> Ah yes, very good then.
> 
>>> Very nice work!
>>
>> Thanks Roman! And thanks a lot for reviewing!
> 
> I intend to make one or more additional passes soon (over zgc itself,
> and probably skimming c2 land).

Ok!

thanks,
Per

> 
> Thanks,
>   Roman
>