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

[Roman Kennke]

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.

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

Thanks,
 Roman