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

Tom Rodriguez Thomas.Rodriguez at Sun.COM
Thu Jul 30 11:53:25 PDT 2009

On Jul 29, 2009, at 7:04 PM, Changpeng Fang wrote:

> 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.

I'm very leery of explicitly clearing the reexecute state.  It's just  
seems likely to be buggy.  It seems to me that once we set the value  
of the reexecute bit for a particular bci in a JVMState, every use of  
that state at that must respect the reexecute bit.  Any use where we  
deopt must reexecute that bytecode. If an exception is actually thrown  
then the existence of the exception will override any reexecute logic,  
which is correct and fine.  So given all that why do we ever need to  
explicitly clear the reexecute bit? I guess I'd like a more complete  
explanation of what will go wrong if we don't clear the reexecute bit  
in the cases where you are currently doing it.

By the way, I noticed that set_bci requires that the reexecute state  
be undefined, which seems wrong to me.  Shouldn't it change the state  
to undefined since we're no longer on the original bytecode?


> 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.

How exactly does it leak into catch block?  Wouldn't that fix this  

> (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.

I'm not sure I understand why this is needed.

> Thanks,
> Changpeng
> On 07/24/09 10:41, changpeng fang - Sun Microsystems - Santa Clara  
> United States wrote:
>> 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:
>>>> Please take a final look at the implementation of reexecute bit.
>>>> Changes from :
>>>> (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:
>>>>> 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

More information about the hotspot-compiler-dev mailing list