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

[Changpeng Fang]

http://cr.openjdk.java.net/~cfang/6833129/webrev.06/

Hopefully this is the last version of update!

It seems we clear the reexecute bit too earlier on the execution path in 
the last update (webrev.05).
My original intention is not to let the reexecute state for the current 
bytecode to leak into the
exception block. But I do this in the wrong place 
(**GraphKit::make_exception_state**). Later
in the parser, when we "do_exception", we still need the current bci and 
associated reexecute
bit before handling the catch block.

webrev.06 does the reexecute bit clean up in two places:

(1) Block::set_start_map() -- reexecute bit should always be initialized 
or reset the undefined at the block start
                                             For the case of exception, 
we will prevent the reexecute bit of the current map
                                             leaking into the catch block.

(2) The place after make_runtime_call of rethrow in 
Parse::catch_inline_exceptions where we are sure that
   the reexecute bit is no longer needed.

Thanks,

Changpeng

 
On 07/24/09 10:41, 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
>>>>
>>>>
>>>
>
>