<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
  <meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
<font color="#333333"><a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~cfang/6833129/webrev.03/">http://cr.openjdk.java.net/~cfang/6833129/webrev.03/</a><br>
<br>
Problem:
<br>
The problem is in intrinsics Object.clone and Arrays.copyOf. When
de-optimization occurs on the slow path for array/instance
<br>
allocation, the Interpreter will continue execution in next bytecode
after the slow allocation calls without actual copying. The<br>
fundamental problem is that the current vm does not have the logic for
the compilers to specify reexecution for the interpreter<br>
if deoptimization happens for cases like&nbsp; intrinsics of Object.clone
and Arrays.copyOf.<br>
<br>
Solution:
<br>
We add such logic in the vm.<br>
(1) For C2, we add an "reexecute" field in JVMState to specify whether
to reexecute this bytecode. This reexecute bit will be<br>
&nbsp;&nbsp;&nbsp;&nbsp; recorded as debug information through </font>
<font color="#333333"><b><b>describe_scope. At runtime, the deoptimizer
will get the reexecution information<br>
&nbsp;&nbsp;&nbsp; at the time of unpack_frame through reading the scope.<br>
(2) There are some bytecodes that we have already known that the
interpreter should re-execute them when deoptimization<br>
&nbsp;&nbsp; happens (e.g.&nbsp; _getfield and _putfield). For C2, we set the
reexecute bit for the out JVMS (youngest one) for them at the <br>
&nbsp;&nbsp; time of adding safepoint edges. For C1, we simply set the reexecute
bit for them at the time of </b></b></font>
<font color="#333333"><b><b>describe_scope.<br>
(3) For </b></b></font><font color="#333333">Object.clone and
Arrays.copyOf., we set the reexecute bit and pop off the argument when
we intrinsify them.<br>
</font><font color="#333333"><br>
Tests:
<br>
Passed specjvm98, JPRT and the test case of clone in the bug report.
<br>
<br>
<br>
Since there are several previous email exchanges, I collect the
questions from them and give a brief answer here:<br>
<br>
============================<br>
>From kvn:<br>
============================<br>
</font>&gt;Also in src/share/vm/opto/callnode.hpp add an assert
<br>
&gt;to check that bci is no changed when _restart is set
<br>
&gt;void set_bci(int bci) { assert(!_restart, ""); _bci = bci; }
<br>
<font color="#333333"><br>
I&nbsp; could not do this assertion here because sync_jvms() will set_bci
and we have already set the restart (reexecute) before<br>
some sync_jvms calls.&nbsp; Do you think it ok for an assertion like </font>assert(_bci==
bci || !_restart, ""); whih means for a new bci the restart<br>
should be false? Thanks.<br>
<font color="#333333"><br>
===================<br>
</font>From jrose: <br>
===================<br>
&gt;Here's a nit:&nbsp; is_restart() in scopeDesc.hpp should be a const
function.
<br>
&gt;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.
<br>
Done! I appreciate if you can double check to see whether the comments
are appropriate or not. Thanks.<br>
<br>
&gt;I'd still like to see the restart decision made by continuation_for
turned into an assert.&nbsp; At least compare it with is_restart():
<br>
<blockquote type="cite">&nbsp;&nbsp; pc = Interpreter::continuation_for(method(),
bcp, callee_parameters, is_top_frame, use_next_mdp);
  <br>
+ assert(is_restart() == !use_next_mdp, "");
  <br>
</blockquote>
&gt;If the assert fails, there may be a bug.<br>
Thanks for pointing out this. Now, after the redesign,&nbsp;
continuation_for does not make decision whether to reexecute or
continue. It is just used to compute the address<br>
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<br>
design? Thanks.<br>
<br>
=========================<br>
>From never:<br>
=========================<br>
&gt;I think the term reexecute should be used in place of restart since
that terminology is used elsewhere.&nbsp; Actually I think should_reexecute
is better than is_reexecute as well.
<br>
&nbsp;
<font color="#000000">Yes, reexecute better than restart here! Thanks.<br>
</font><br>
&gt;I don't really like that the restart bit is associated with the
bci.&nbsp; It implies that any scope can be reexecuted when it fact it's
only the topmost one that can be.&nbsp; Given how these describing scopes is
structured I'm not sure I see a better way though.&nbsp; I &gt;don't see any
asserts to enforce this for the scopeDescs either.
<br>
&nbsp;
Done ! Thanks<br>
The printing forms for ScopeDesc and JVMState should indicate if
this is a restarted bytecode or not.&nbsp; The SA also needs to be updated
to read these modified ScopeDescs.
<br>
Done! Thanks.<br>
<br>
&gt;I think manually toggling the restart bit back and forth should be
avoided.&nbsp; Preserve the original and pass on the modified one or have a
helper object that toggles the bit in it's constructor/destructor.
<br>
I just design a new class "PreserveReexecuteAndSP" to save and restore
the reexecute bit and sp. Thanks.<br>
<br>
<br>
Thank you so much for your help and inputs.<br>
<br>
Changpeng<br>
<br>
<br>
</body>
</html>