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

[Vladimir Kozlov]

Looks good.
Did you verified that jvms in GraphKit::make_exception_state()
is different from jvms (with Reexecute_True) of new_instance call
in clone intrinsic ?

Thanks,
Vladimir

changpeng fang - Sun Microsystems - Santa Clara United States wrote:
> http://cr.openjdk.java.net/~cfang/6833129/webrev.05/
> 
> I updated the change with the addition of the assert of undefined 
> reexecute state
> in set_bci(). This assert caught the problem of reexecute state leaking 
> to the exception
> path. Here I propose to clear the reexecute state when the control goes 
> to the exception
> path because the parser will stop parsing the current bytecode (it will 
> either parses
> the catch block or goes to process exit).
> 
> Thanks,
> 
> Changpeng
> 
> 
> 
> 
> On 07/22/09 13:41, Vladimir Kozlov wrote:
>> I would use REState name instead of REBit.
>> Also why you did not add the assert we talked about in set_bci()?
>> Otherwise looks good.
>>
>> Vladimir
>>
>> changpeng fang - Sun Microsystems - Santa Clara United States wrote:
>>> http://cr.openjdk.java.net/~cfang/6833129/webrev.04/
>>>
>>> Please take a final look at the implementation of reexecute bit.
>>>
>>> Changes from http://cr.openjdk.java.net/~cfang/6833129/webrev.03/ :
>>>
>>> (1) In C2 JVMState,  use  a three state field (instead of a boolean) 
>>> to represent  the  reexecute bit:  *  class JVMState : public 
>>> ResourceObj {*
>>> *+ public:*
>>> *+   typedef enum {*
>>> *+     RE_Undefined = -1, // not defined -- will be translated into 
>>> false later*
>>> *+     RE_False     =  0, // false       -- do not reexecute*
>>> *+     RE_True      =  1  // true        -- reexecute the bytecode*
>>> *+   } REBit; //ReExecution Bit*
>>> *+ *
>>>
>>>  In this way, we can keep the original defined reexecute bit (true or 
>>> false) when we are going to determine
>>> the reexecute bit by bytecode.
>>>
>>> *+   if (out_jvms->should_reexecute() == JVMState::RE_Undefined && 
>>> //don't touch defined values*
>>> *+       should_reexecute_implied_by_bytecode(out_jvms)) {*
>>> *+     out_jvms->set_reexecute(JVMState::RE_True);*
>>> *+   }*
>>> *+ *
>>>
>>> (2)  Renamed Intepreter::continuation_for to be Interpreter:: 
>>> deopt_continue_after_entry.
>>>       Renamed Interpreter::bytecodes_to_reexecute to 
>>> Interpreter::bytecode_should_reexecute
>>>
>>>
>>> Thanks,
>>>
>>> Changpeng
>>>
>>>
>>> On 07/16/09 17:16, changpeng fang - Sun Microsystems - Santa Clara 
>>> United States wrote:
>>>> http://cr.openjdk.java.net/~cfang/6833129/webrev.03/
>>>>
>>>> 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 bytecode 
>>>> after the slow allocation calls without actual copying. The
>>>> fundamental problem is that the current vm does not have the logic 
>>>> for the compilers to specify reexecution for the interpreter
>>>> if deoptimization happens for cases like  intrinsics of Object.clone 
>>>> and Arrays.copyOf.
>>>>
>>>> Solution:
>>>> We add such logic in the vm.
>>>> (1) For C2, we add an "reexecute" field in JVMState to specify 
>>>> whether to reexecute this bytecode. This reexecute bit will be
>>>>      recorded as debug information through **describe_scope. At 
>>>> runtime, the deoptimizer will get the reexecution information
>>>>     at the time of unpack_frame through reading the scope.
>>>> (2) There are some bytecodes that we have already known that the 
>>>> interpreter should re-execute them when deoptimization
>>>>    happens (e.g.  _getfield and _putfield). For C2, we set the 
>>>> reexecute bit for the out JVMS (youngest one) for them at the
>>>>    time of adding safepoint edges. For C1, we simply set the 
>>>> reexecute bit for them at the time of ** **describe_scope.
>>>> (3) For **Object.clone and Arrays.copyOf., we set the reexecute bit 
>>>> and pop off the argument when we intrinsify them.
>>>>
>>>> Tests:
>>>> Passed specjvm98, JPRT and the test case of clone in the bug report.
>>>>
>>>>
>>>> Since there are several previous email exchanges, I collect the 
>>>> questions from them and give a brief answer here:
>>>>
>>>> ============================
>>>> >From kvn:
>>>> ============================
>>>> >Also in src/share/vm/opto/callnode.hpp add an assert
>>>> >to check that bci is no changed when _restart is set
>>>> >void set_bci(int bci) { assert(!_restart, ""); _bci = bci; }
>>>>
>>>> I  could not do this assertion here because sync_jvms() will set_bci 
>>>> and we have already set the restart (reexecute) before
>>>> some sync_jvms calls.  Do you think it ok for an assertion like 
>>>> assert(_bci== bci || !_restart, ""); whih means for a new bci the 
>>>> restart
>>>> should be false? Thanks.
>>>>
>>>> ===================
>>>> From jrose:
>>>> ===================
>>>> >Here's a nit:  is_restart() in scopeDesc.hpp should be a const 
>>>> function.
>>>> >I'd like to see comment in each case demonstrating why that the 
>>>> values captured in dummy_restart, and the throwaway restart in 
>>>> javaClasses.cpp (which should be called dummy_restart also) don't 
>>>> matter.
>>>> Done! I appreciate if you can double check to see whether the 
>>>> comments are appropriate or not. Thanks.
>>>>
>>>> >I'd still like to see the restart decision made by continuation_for 
>>>> turned into an assert.  At least compare it with is_restart():
>>>>>    pc = Interpreter::continuation_for(method(), bcp, 
>>>>> callee_parameters, is_top_frame, use_next_mdp);
>>>>> + assert(is_restart() == !use_next_mdp, "");
>>>> >If the assert fails, there may be a bug.
>>>> Thanks for pointing out this. Now, after the redesign,  
>>>> continuation_for does not make decision whether to reexecute or 
>>>> continue. It is just used to compute the address
>>>> for the continuation (next bytecode). I add a new function to 
>>>> compute the reexecution address 
>>>> Interpreter::deopt_reexecute_entry(...). What do you think of the new
>>>> design? Thanks.
>>>>
>>>> =========================
>>>> >From never:
>>>> =========================
>>>> >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.
>>>>   Yes, reexecute better than restart here! 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.
>>>>   Done ! Thanks
>>>> 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.
>>>> Done! Thanks.
>>>>
>>>> >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 just design a new class "PreserveReexecuteAndSP" to save and 
>>>> restore the reexecute bit and sp. Thanks.
>>>>
>>>>
>>>> Thank you so much for your help and inputs.
>>>>
>>>> Changpeng
>>>>
>>>>
>>>
>