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.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/20090716/1feb2d25/attachment.html