RFR(M): 8232896: ZGC: Enable C2 clone intrinsic

[Per Liden]

Hi Nils,

On 11/4/19 10:16 AM, Nils Eliasson wrote:
> 
> On 2019-11-01 12:50, Per Liden wrote:
>> Hi Nils,
>>
>> On 10/31/19 5:37 PM, Nils Eliasson wrote:
>>> Hi,
>>>
>>> This patch fixes and enables the clone intrinsic for C2 with ZGC.
>>>
>>> The main thing added is an implementation of the 
>>> ZBarrierSetC2::clone_at_expansion method. This method handles clone 
>>> expansion for regular objects and primitive arrays that hasn't 
>>> already been reduced by optimizations. (Oop array clones doesn't go 
>>> through this path.)
>>>
>>> The code switches on the type of the source to either make a leaf 
>>> call to a runtime clone for regular objects, or delegate to 
>>> BarrierSetC2::clone_at_expansion for primitive arrays.
>>>
>>> Updated micro benchmark shows great gains, especially for small 
>>> objects that now will be reduced to inlined move-store sequences.
>>
>> Sweet! The speedup of micro:Clone is impressive!
>>
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8232896
>>>
>>> Webrev: http://cr.openjdk.java.net/~neliasso/8232896/webrev.02/
>>
>> My review comments summarized in a patch on top of yours. Let me know 
>> if there's something you don't agree with. The changes are:
>> * Changed the type of the runtime function's size arguments from 
>> TypeInt::INT to TypeLong::LONG, to make it 64 bits, rather than 32.
>> * Changed the new runtime function to call HeapAccess::clone() rather 
>> than ZBarrier::clone_oop(), since ZBarrier is the backend for the 
>> access layer, not the frontend.
>> * Renamed clone_oop() to clone(), as we're cloning an object rather 
>> than an oop.
> 
> Overall, I like your suggestions.
> 
> One problem with using "clone" is that is suggest that it will create a 
> new Object too (like how java.lang.Object.clone does). But this function 
> only copies the contents. I am open to suggestions, and even keeping the 
> clone name, but feel a slight bit of unease.

I see your points. However, I see two reasons to keep that name:
1) In the Access API it's called Access::clone(src, dst, size), and 
since ZBarrierRuntime::clone() is a bridge to that function it might be 
good to use the same name.
2) The clone() function has a dst argument, implying that dst already 
exists.

Objections?

cheers,
Per

> 
> Thanks!
> 
> /Nils
> 
>> * Added const where applicable.
>> * Adjusted an include line.
>> * Moved the clone_at_expansion() function up in the file.
>>
>> http://cr.openjdk.java.net/~pliden/8232896/webrev.pliden_review.0
>>
>> I ran micro:Clone to verify my changes.
>>
>> cheers,
>> Per
>>
>>>
>>>
>>> Please review,
>>>
>>> Nils Eliasson
>>>