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

[Vladimir Ivanov]

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


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

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

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