Request for reviews (M): 6833129 : specjvm98 fails with NullPointerException in the compiler with -XX:DeoptimizeALot

[changpeng fang - Sun Microsystems - Santa Clara United States]

On 07/07/09 13:33, Tom Rodriguez wrote:
> I think the term reexecute should be used in place of restart since 
> that terminology is used elsewhere.  Actually I think should_reexecute 
> is better than is_reexecute as well.
>
I will use reexecute and should_reexecute throughout the change. Thanks.

> I don't really like that the restart bit is associated with the bci.  
> It implies that any scope can be reexecuted when it fact it's only the 
> topmost one that can be.  Given how these describing scopes is 
> structured I'm not sure I see a better way though.  I don't see any 
> asserts to enforce this for the scopeDescs either.
Do you mean assert like this :
assert(!_reexecute || is_top(), "reexecute only applies to top scope");

If yes, then I must have something wrong in setting up the reexecute bit 
in JVMState in C2.

>
> The printing forms for ScopeDesc and JVMState should indicate if this 
> is a restarted bytecode or not.  The SA also needs to be updated to 
> read these modified ScopeDescs.
>
There may be some bugs in JVMState::dump()! I will investigate it and 
add the reexecute bit.

>>> We talked about using  PreserveJVMState pjvms(this) instead
>>> of restoring stack and restart flag in intrinsics?
>>> Why you did not use it?
>> Just as we discussed a moment ago, I added an assert restart false 
>> before
>> we set it to true.
>
> I think manually toggling the restart bit back and forth should be 
> avoided.  Preserve the original and pass on the modified one or have a 
> helper object that toggles the bit in it's constructor/destructor.
>
I prefer  "Preserve the original and pass on the modified one".   We can 
not  use  PreserveJVMState only for the purpose of preserving  _sp (the 
map will be changed during the scope of preservation, and
may cause undesired consequence).

Thanks,

Changpeng 


> tom
>
>>
>> Here is the updated webrev link!
>> http://cr.openjdk.java.net/~cfang/6833129/webrev.01/
>>
>> Thanks,
>>
>> Changpeng
>>
>>> thanks,
>>> Vladimir
>>>
>>> changpeng fang - Sun Microsystems - Santa Clara United States wrote:
>>>> http://cr.openjdk.java.net/~cfang/6833129/webrev.00/
>>>>
>>>> Problem:
>>>> The problem is in intrinsics Object.clone and Arrays.copyOf. When 
>>>> de-optimization occurs on the slow path for array/instance
>>>> allocation, the Interpreter will continue execution in next bc 
>>>> after the slow allocation calls without actual copying.
>>>>
>>>> Solution:
>>>> Add a restart bit in debuginfo to direct the deopt to restart 
>>>> execution of the bytecode  that invokes  Object.clone/Arrays.copyOf.
>>>> The restart bit is set up in Inline Intrinsics of 
>>>> Object.clone/Arrays.copyOf.
>>>>
>>>> Tests:
>>>> Passed specjvm98, JPRT and the test case of clone in the bug report.
>>>>
>>>> Thanks,
>>>>
>>>> Changpeng
>>>>
>>
>