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

[Nils Eliasson]

On 2019-11-04 10:26, Per Liden wrote:
> 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

No,

Lets go with this.

Thanks!

// Nils

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