Request for reviews (S): 6880053

[Tom Rodriguez]

Why not require instance_id to be specified instead of it being  
optional?

tom

On Sep 10, 2009, at 6:08 PM, Vladimir Kozlov wrote:

> Tom,
>
> I forgot to add instance_id meet changes for TypeOopPtr
> in the last webrev. I added it:
>
> http://cr.openjdk.java.net/~kvn/6880053/webrev.01
>
> Thanks,
> Vladimir
>
> Vladimir Kozlov wrote:
>> Thanks Tom
>> Vladimir
>> Tom Rodriguez wrote:
>>> I like that much better.
>>>
>>> tom
>>>
>>> On Sep 10, 2009, at 2:35 PM, Vladimir Kozlov wrote:
>>>
>>>> I used MembarCPUOrder, removed second CheckCastPP and
>>>> ran through CTW, JPRT and refworkload.
>>>> I don't see performance difference.
>>>>
>>>> http://cr.openjdk.java.net/~kvn/6880053/webrev.01
>>>>
>>>> Vladimir
>>>>
>>>> Vladimir Kozlov wrote:
>>>>> I am taking my previous statement back.
>>>>> We are already generating MembarCPUOrder after generic arraycopy
>>>>> under flag InsertMemBarAfterArraycopy which is true by default.
>>>>> The only place where we miss it is in clone() intrinsic.
>>>>> I will check the performance with it. Also we may need to
>>>>> generate it always (not under flag) when it is used for
>>>>> zeroing reduction.
>>>>> Vladimir
>>>>> Vladimir Kozlov wrote:
>>>>>> The problem with membar is that it is barrier for all memory,
>>>>>> we can't set it to slices corresponding only to this object.
>>>>>> It will prevent memory optimizations for other objects.
>>>>>> I don't remember other problems then this performance concern.
>>>>>>
>>>>>> Vladimir
>>>>>>
>>>>>> Tom Rodriguez wrote:
>>>>>>> At one point in the original discussion you suggested using  
>>>>>>> MembarCPUOrder to enforce the ordering which allows us to  
>>>>>>> leave the CheckCastPP in it's original location without adding  
>>>>>>> another CheckCastPP.  I'd prefer that to playing weird games  
>>>>>>> with the types.  Is that a viable fix?
>>>>>>>
>>>>>>> tom
>>>>>>>
>>>>>>> On Sep 9, 2009, at 5:32 PM, Vladimir Kozlov wrote:
>>>>>>>
>>>>>>>> It is valid C2 type since all type methods (xmeet) process it
>>>>>>>> but it should not exist from java point of view. And I'm  
>>>>>>>> using it
>>>>>>>> with assumption that none java programs use such type.
>>>>>>>>
>>>>>>>> I am also not fully comfortable with it but I can't find
>>>>>>>> other type which could be used (I tried).
>>>>>>>> I talked with John and he agreed with this change.
>>>>>>>>
>>>>>>>> I ran CTW and it did not find problems with this change.
>>>>>>>> But we may still hit some problems where we would expect
>>>>>>>> a real java type and will get this one. But such cases
>>>>>>>> should be caught by asserts we have.
>>>>>>>>
>>>>>>>> The other solution (which we rejected before) is to modify
>>>>>>>> oop map build to list raw pointers which have CheckCastPP
>>>>>>>> users. Then we will not need the second CheckCastPP.
>>>>>>>>
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> Tom Rodriguez wrote:
>>>>>>>>> I suspected that was the purpose but TypeOopPtr::NOTNULL  
>>>>>>>>> seems nonsensical to me.  Is that really a valid type?
>>>>>>>>> tom
>>>>>>>>> On Sep 9, 2009, at 4:43 PM, Vladimir Kozlov wrote:
>>>>>>>>>> As you remember we have 2 CheckCastPP nodes in clone()  
>>>>>>>>>> intrinsic.
>>>>>>>>>> One is original which we move and pin below arraycopy call
>>>>>>>>>> to guaranty that all memory accesses to the cloned object
>>>>>>>>>> are placed below arraycopy.
>>>>>>>>>> And the second is used to have a live oop across arraycopy  
>>>>>>>>>> call.
>>>>>>>>>> If both have the same type (bug's case) the  
>>>>>>>>>> CheckCastPPNode::identity()
>>>>>>>>>> will eliminate the original CheckCastPP since it references  
>>>>>>>>>> the second:
>>>>>>>>>>
>>>>>>>>>> // Replace raw memory edge with new CheckCastPP to have a  
>>>>>>>>>> live oop
>>>>>>>>>> // at safepoints instead of raw value.
>>>>>>>>>> assert(new_obj->is_CheckCastPP() && new_obj->in(1) ==  
>>>>>>>>>> alloc_obj->in(1), "sanity");
>>>>>>>>>> alloc_obj->set_req(1, new_obj);
>>>>>>>>>>
>>>>>>>>>> Vladimir
>>>>>>>>>>
>>>>>>>>>> Tom Rodriguez wrote:
>>>>>>>>>>> I'm don't understand this.  What is TypeOopPtr::NOTNULL  
>>>>>>>>>>> supposed to represent?  That doesn't seem like a valid  
>>>>>>>>>>> type in the lattice since it would have to correspond to  
>>>>>>>>>>> Object:NotNull which is already represented by  
>>>>>>>>>>> TypeInstPtr::NOTNULL.  Why will the graph be incorrect if  
>>>>>>>>>>> the type is TypeInstPtr::NOTNULL?
>>>>>>>>>>> tom
>>>>>>>>>>> On Sep 9, 2009, at 3:35 PM, Vladimir Kozlov wrote:
>>>>>>>>>>>> http://cr.openjdk.java.net/~kvn/6880053/webrev.00
>>>>>>>>>>>>
>>>>>>>>>>>> Fixed 6880053: assert(alloc_obj->as_CheckCastPP()- 
>>>>>>>>>>>> >type() != TypeInstPtr::NOTNULL)
>>>>>>>>>>>>
>>>>>>>>>>>> Problem:
>>>>>>>>>>>> I added this assert in 6875577 fix to catch cases when
>>>>>>>>>>>> the type of the cloned object is TypeInstPtr::NOTNULL
>>>>>>>>>>>> since it will produce incorrect graph.
>>>>>>>>>>>> And Nightly testing found such case: the method
>>>>>>>>>>>> sun.reflect.GeneratedMethodAccessor4.invoke(Object o,  
>>>>>>>>>>>> Object[] ao)
>>>>>>>>>>>> returns the clone of the Object o argument.
>>>>>>>>>>>>
>>>>>>>>>>>> Solution:
>>>>>>>>>>>> Use more general oop type TypeOopPtr::NOTNULL for
>>>>>>>>>>>> additional CheckCastPP node in clone() instrinsic and  
>>>>>>>>>>>> arraycopy.
>>>>>>>>>>>> Also fix instance_id meet for TypeOopPtr.
>>>>>>>>>>>>
>>>>>>>>>>>> Reviewed by:
>>>>>>>>>>>>
>>>>>>>>>>>> Fix verified (y/n): y, test
>>>>>>>>>>>>
>>>>>>>>>>>> Other testing:
>>>>>>>>>>>> JPRT
>>>>>>>>>>>>
>>>>>>>
>>>