RFR(M): 6700100: optimize inline_native_clone() for small objects with exact klass

[Vladimir Kozlov]

On 10/9/14 2:08 PM, Roland Westrelin wrote:
> Hi Vladimir,
>
> Thanks for reviewing this.
>
>> Sorry it took so long.
>
> I sent this one yesterday so no, it didn’t take long! Maybe you’re thinking about 8054478 which has been out for a bit longer. I’m curious what you think about that one.

Yes, that one. I am not happy about those changes so I need time to 
think about it and may have some suggestions.

>
>> Could you explain changes in fieldStreams.hpp? How it worked before?
>> I see that the change only affects code in FieldStreamBase(Array<u2>* fields, constantPoolHandle constants, int start, int limit).
>
> InternalFieldStream was not used anywhere and it’s broken. That change to FieldStreamBase fixes it.
> InternalFieldStream starts iterating from k->java_fields_count() up to num_fields so num_fields must be the total number of fields.

Good.

>
>> And can it be simple done as?:
>>
>> return num_fields + _index;
>
> I use the same code pattern as the next loop:
>
> 70     for (int i = _index; i*FieldInfo::field_slots < length; i++) {
>
> 76       num_fields ++;
> 77     }
>
> But that doesn’t make much sense, you’re right. I’ll fix it.
>
>> StressArrayCopyMacroNode flag is used in product code. Make it diagnostic.
>>
>> Why you skip klasses with injected fields? I thought nof_nonstatic_fields() includes them. Add comment.
>
> I skip them because they are rare and it seems simpler to simply skip them.

But you do the check (scanning all fields) for all classes. That is what 
I am concern about. May be it is better to add a flag to InstanceKlass 
during class parsing when fields are injected?

> ciInstanceKlass::compute_nonstatic_fields_impl() uses JavaFieldStream that only iterates over regular java fields.

Okay.

>
>>
>> How we benefit from doing this optimization during parsing? Why you need StressArrayCopyMacroNode?
>
> It’s a stress option. The Ideal transformation is often applied at parse time and I wanted to make sure I could stress test the Ideal transformation during IGVN because it performs somewhat trickier graph transformations. Why would I need to make it diagnostic?

My question was why not to do that always after parse, during IGVN? You 
expand arraycopy node to several memory nodes which increase nodes count 
and complicate the graph. It may be premature to do that during parsing. 
On other hand ClearArrayNode::Ideal() is executed during parsing too.

Yes, you don't need StressArrayCopyMacroNode be diagnostic if you don't 
want to stress product VM.

Vladimir

>
> Rolaand.
>
>>
>> Otherwise changes looks reasonable.
>>
>> Thanks,
>> Vladimir
>>
>> On 10/8/14 5:33 AM, Roland Westrelin wrote:
>>> http://cr.openjdk.java.net/~roland/6700100/webrev.00/
>>>
>>> Converts ArrayCopy nodes for small instance clones to series of loads/stores + a little bit of cleanup.
>>>
>>> Small array clones & copies as loads/stores are coming next in another change.
>>>
>>> Roland.
>>>
>