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

[Vladimir Kozlov]

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