RFR(L): 6912521: System.arraycopy works slower than the simple loop for little lengths

[Roland Westrelin]

Thanks for the review, Vladimir.

Roland.

> On Feb 16, 2015, at 1:50 PM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
> 
> Looks good.
> 
> Best regards,
> Vladimir Ivanov
> 
> On 2/6/15 4:23 PM, Roland Westrelin wrote:
>>> The comment in library_call.cpp has strange letters "we’re":
>>> 
>>> +       // ArrayCopyNode:Ideal may transform the ArrayCopyNode to
>>> +       // loads/stores but it is legal only if we’re sure the
>>> +       // Arrays.copyOf would succeed. So we need all input arguments
>>> +       // to the copyOf to be validated, including that the copy to the
>>> +       // new array won’t trigger an ArrayStoreException. That subtype
>>> +       // check can be optimized if we know something on the type of
>>> +       // the input array from type speculation.
>>> 
>>> Otherwise looks good.
>> 
>> Thanks for the review, Vladimir. I’ll fix the comment.
>> 
>> I need another review. Any taker?
>> 
>> Roland.
>> 
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>> On 1/19/15 1:47 AM, Roland Westrelin wrote:
>>>> Here is a new webrev:
>>>> 
>>>> http://cr.openjdk.java.net/~roland/6912521/webrev.01/
>>>> 
>>>> As suggested by Vladimir:
>>>> - the ArrayCopyNode stuff is its own file
>>>> - I added comments for library_call.cpp Arrays.CopyOf changes
>>>> 
>>>> As suggested by John privately:
>>>> - I moved conv_I2X_offset to Compile
>>>> - I now use Node::find_int_con and find_intptr_t_con in ArrayCopyNode::get_length_if_constant
>>>> 
>>>> I also modified the test cases to check that once a copy is compiled as a series of loads/stores, we don’t allow copies that should throw ArrayStoreException to proceed.
>>>> 
>>>> Roland.
>>>> 
>>>> 
>>>>> On Jan 15, 2015, at 12:29 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>>> 
>>>>> On 1/14/15 1:32 PM, Roland Westrelin wrote:
>>>>>> Hi Vladimir. Thanks for taking a look at this.
>>>>>> 
>>>>>>> The logic which choose direction of coping in ArrayCopyNode::Ideal() is strange. I would like to see more explicit checks there. Something like:
>>>>>>> 
>>>>>>> if (is_array_copy_overlap()) {
>>>>>>>  array_copy_backward()
>>>>>>> } else {
>>>>>>>  array_copy_forward()
>>>>>>> }
>>>>>> 
>>>>>> I’m not following you. In the general case, the test needs to be done at runtime. Your code above seems to imply that we would always decide at compile time?
>>>>> 
>>>>> My bad, you are right.
>>>>> 
>>>>>> 
>>>>>>> Can you move ArraCopy class code from callnode.?pp to new arraycopynode.?pp files? the code become too large.
>>>>>> 
>>>>>> Ok.
>>>>>> 
>>>>>>> Can you add comment in library_call.cpp explaining new validation/casting logic? Why you do that?
>>>>>> 
>>>>>> I will add comments.
>>>>>> To be legal, the transformation of the ArrayCopyNode to loads/stores can only happen if we’re sure the Arrays.copyOf would succeed. So we need all input arguments to the copyOf to be validated, including that the copy to the new array won’t trigger an ArrayStoreException. That’s why there’s a subtype check. That subtype check can be optimized if we know something on the type of the input array from type speculation.
>>>>> 
>>>>> Okay.
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>>> 
>>>>>> Roland.
>>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>> 
>>>>>>> On 1/14/15 1:34 AM, Roland Westrelin wrote:
>>>>>>>> http://cr.openjdk.java.net/~roland/6912521/webrev.00/
>>>>>>>> 
>>>>>>>> Follow up to 6700100 (instance clone as series of loads/stores): convert ArrayCopyNode for small array copies (clone of arrays, System.arraycopy, Arrays.copyOf) to series of loads and stores.
>>>>>>>> 
>>>>>>>> Roland.
>>>>>>>> 
>>>>>> 
>>>> 
>>