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

Thu Nov 28 15:15:40 UTC 2019

On 2019-11-28 13:28, Vladimir Ivanov wrote:
>
>>> 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)
>
> Actually the header is copied, but then cleared.
>
>
> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/oops/accessBackend.inline.hpp#l363 
>

Let me correct myself:

For the intrinsic case - we are copying part of the header - the klass, 
but not the markword.

For the runtime call that is used by native clone, and ZGCs clone_inst - 
the entire Object is cloned. This seems unnecessary to me - but that is 
how it is done when the intrinsic is disabled too. This could probably 
be changed, but then we would need to verify it everywhere.

>
>
>>> -  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
>
> I'm looking at webrev.05:
>
> src/hotspot/share/gc/shared/c2/barrierSetC2.cpp:
>
> -  ArrayCopyNode* ac = ArrayCopyNode::make(kit, false, src_base, NULL, 
> dst_base, NULL, countx, true, false);
> -  ac->set_clonebasic();
> +  ArrayCopyNode* ac = ArrayCopyNode::make(kit, false, payload_src, 
> NULL, payload_dst, NULL, payload_size, true, false);
> +  if (is_array) {
> +    ac->set_clone_prim_array();
> +  } else {
> +    ac->set_clone_inst();
> +  }
>
> I'm looking at LibraryCallKit::inline_native_clone():
>
>
> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/library_call.cpp#l4323 
>
>
> It looks like object arrays are filtered out only if 
> array_copy_requires_gc_barriers() == true:
>
>
> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/library_call.cpp#l4333 
>
>
> But your change sets set_clone_prim_array() irrespective of whether 
> object arrays are specially treated or not.
>
> It looks like a naming problem, but still.

Yes, you are right, for GCs without a barriers on oop arrays the naming 
will be off.

The right thing to do would probably be to have one state for inst or 
array, and another state for what type of array.

And it gets more complicated, at all the places where 
is_clone_basic()/is_clone_inst_or_prim_array() is used, different GCs 
would need different answers. ZGC can treat prim and oop arrays the 
same, until when choosing the right type of acopy, while other GCs that 
expand barriers early, can't do that.

After talking to Per I suggest to revert is_clone_inst_or_prim_array() 
to is_clonebasic() and give it a proper comment.

I will need to revisit this and continue to simplify things and clean it up.

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

One small diff - src/hotspot/share/opto/arraycopynode.cpp only had a 
formatting change - so I have reverted it completely.

// Nils

>
> PS: and extract_base_offset is still there:
>
> -void BarrierSetC2::clone(GraphKit* kit, Node* src, Node* dst, Node* 
> size, bool is_array) const {
> +int BarrierSetC2::extract_base_offset(bool is_array) {

ah - good - missed that one when merging with Pers stuff.

Thanks for you feedback.

/ Nils

>
> Best regards,
> Vladimir Ivanov
>
>
>>
>> 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
>>>>>>