RFR(S): ZGC: C2: Oop instance cloning causing skipped compiles

[Nils Eliasson]

Hi,

On 2019-11-26 13:02, Vladimir Ivanov wrote:
>
>> http://cr.openjdk.java.net/~neliasso/8234520/webrev.03/
>
> ====================================================================
>
> src/hotspot/share/gc/z/c2/zBarrierSetC2.cpp:
>
> +  // The currently modeled arraycopy-clone_basic doesn't have the 
> base pointers for src and dst,
> +  // rather point at the start of the payload.
> +  Node* src_base = get_base_for_arracycopy_clone(phase, src);
> +  Node* dst_base = get_base_for_arracycopy_clone(phase, dst);
> +
> +  // The size must also be increased to match the instance size.
> +  int base_off = BarrierSetC2::extract_base_offset(false);
> +  Node* full_size = phase->transform_later(new AddLNode(size, 
> phase->longcon(base_off >> LogBytesPerLong)));
>    Node* const call = phase->make_leaf_call(ctrl,
>                                             mem,
>                                             clone_type(),
>
> ZBarrierSetRuntime::clone_addr(),
> "ZBarrierSetRuntime::clone",
>                                             TypeRawPtr::BOTTOM,
> -                                           src,
> -                                           dst,
> -                                           size);
> +                                           src_base,
> +                                           dst_base,
> +                                           full_size,
> +                                           phase->top());
>
>
> Do you see any problems with copying object header?

It won't be copied. It's just that the runtime call expects the 
arguments to be pointers to the objects, and the size of the object. 
It's the same function that is used by a call to the native clone impl. 
(jvm.cpp:720)

>
> ====================================================================
>
> The rest are minor comments:
>
> src/hotspot/share/gc/shared/c2/barrierSetC2.cpp:
>
> -void BarrierSetC2::clone(GraphKit* kit, Node* src, Node* dst, Node* 
> size, bool is_array) const {
> -  // Exclude the header but include array length to copy by 8 bytes 
> words.
> -  // Can't use base_offset_in_bytes(bt) since basic type is unknown.
> +int BarrierSetC2::extract_base_offset(bool is_array) {
>
> ...
>
> +void BarrierSetC2::clone(GraphKit* kit, Node* src, Node* dst, Node* 
> size, bool is_array) const {
> +  // Exclude the header but include array length to copy by 8 bytes 
> words.
> +  // Can't use base_offset_in_bytes(bt) since basic type is unknown.
> +  int base_off = extract_base_offset(is_array);
>
>
> I'd leave the comment in BarrierSetC2::extract_base_offset(). After 
> the refactoring it looks confusing in BarrierSetC2::clone().
>
> Also, considering it's used from zBarrierSetC2.cpp, it would be nice 
> to have a more descriptive name. 
> BarrierSetC2::arraycopy_payload_base_offset(bool is_array) maybe?

Sounds reasonable.

>
>
> ====================================================================
>
> src/hotspot/share/gc/z/c2/zBarrierSetC2.cpp:
>
> +Node* get_base_for_arracycopy_clone(PhaseMacroExpand* phase, Node* n) {
>
> Should get_base_for_arracycopy_clone be static? Also, would be nice if 
> the name reflects that it works on instances and not arrays.
Fixed
>
> ====================================================================
>
> +  // The currently modeled arraycopy-clone_basic doesn't have the 
> base pointers for src and dst,
> +  // rather point at the start of the payload.
> +  Node* src_base = get_base_for_arracycopy_clone(phase, src);
> +  Node* dst_base = get_base_for_arracycopy_clone(phase, dst);
>
> Another thing: "base" in zBarrierSetC2.cpp and in BarrierSetC2 has 
> opposite meaning which is confusing.
>
> Renaming src/dst<->src_base/dst_base in 
> ZBarrierSetC2::clone_at_expansion() would improve things.

Done.

>
> ====================================================================
>
> -  if (src->bottom_type()->isa_aryptr()) {
> +  if (ac->is_clone_array()) {
>      // Clone primitive array
>
> Is the comment valid? Doesn't it cover object array case as well?

Nope - object arrays will be handled as clone_oop_array which uses the 
normal object copy which already applies the appropriate load barriers.

The special case for ZGC is the cloning of instances because we don't 
know where to apply load barriers without looking up the type. (Except 
for clone on small objects and short arrays that are transformed to a 
series of load-stores.)

http://cr.openjdk.java.net/~neliasso/8234520/webrev.04

Thank you for the feedback!

// Nils

>
>
> Best regards,
> Vladimir Ivanov
>
>>
>> Regards,
>>
>> Nils
>>
>>
>> On 2019-11-21 12:53, Nils Eliasson wrote:
>>> I updated this to version 2.
>>>
>>> http://cr.openjdk.java.net/~neliasso/8234520/webrev.02/
>>>
>>> I found a problen running 
>>> compiler/arguments/TestStressReflectiveCode.java
>>>
>>> Even though the clone was created as a oop clone, the type node type 
>>> returns isa_aryprt. This is caused by the src ptr not being the base 
>>> pointer. Until I fix that I wanted a more robust test.
>>>
>>> In this webrev I split up the is_clonebasic into is_clone_oop and 
>>> is_clone_array. (is_clone_oop_array is already there). Having a 
>>> complete set with the three clone types allows for a robust test and 
>>> easy verification. (The three variants end up in different paths 
>>> with different GCs).
>>>
>>> Regards,
>>>
>>> Nils
>>>
>>>
>>> On 2019-11-20 15:25, Nils Eliasson wrote:
>>>> Hi,
>>>>
>>>> I found a few bugs after the enabling of the clone intrinsic in ZGC.
>>>>
>>>> 1) The arraycopy clone_basic has the parameters adjusted to work as 
>>>> a memcopy. For an oop the src is pointing inside the oop to where 
>>>> we want to start copying. But when we want to do a runtime call to 
>>>> clone - the parameters are supposed to be the actual src oop and 
>>>> dst oop, and the size should be the instance size.
>>>>
>>>> For now I have made a workaround. What should be done later is 
>>>> using the offset in the arraycopy node to encode where the payload 
>>>> is, so that the base pointers are always correct. But that would 
>>>> require changes to the BarrierSet classes of all GCs. So I leave 
>>>> that for next release.
>>>>
>>>> 2) The size parameter of the TypeFunc for the runtime call has the 
>>>> wrong type. It was originally Long but missed the upper Half, it 
>>>> was fixed to INT (JDK-8233834), but that is wrong and causes the 
>>>> compiles to be skipped. We didn't notice that since they failed 
>>>> silently. That is also why we didn't notice problem #1 too.
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8234520
>>>>
>>>> http://cr.openjdk.java.net/~neliasso/8234520/webrev.01/
>>>>
>>>> Please review!
>>>>
>>>> Nils
>>>>