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

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

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20090721/02b68294/attachment.html