<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">It looks good.<br>
      Thank you for the nit fixes!<br>
      <br>
      Thanks,<br>
      Serguei<br>
      <br>
      <br>
      On 1/17/13 10:49 AM, Coleen Phillimore wrote:<br>
    </div>
    <blockquote cite="mid:50F847B3.7080305@oracle.com" type="cite">
      <meta content="text/html; charset=ISO-8859-1"
        http-equiv="Content-Type">
      <br>
      <div class="moz-cite-prefix">On 01/17/2013 12:50 PM, <a
          moz-do-not-send="true" class="moz-txt-link-abbreviated"
          href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
        wrote:<br>
      </div>
      <blockquote cite="mid:50F839DD.60203@oracle.com" type="cite">
        <meta content="text/html; charset=ISO-8859-1"
          http-equiv="Content-Type">
        <div class="moz-cite-prefix">Coleen,<br>
          <br>
          <br>
          Looked at this version of webrev:<br>
          <b>&nbsp; <a moz-do-not-send="true" class="moz-txt-link-freetext"
              href="http://cr.openjdk.java.net/%7Ecoleenp/7174978_6/">http://cr.openjdk.java.net/~coleenp/7174978_6/</a></b><br>
          <br>
          <br>
          The changes look good, great work!<br>
          Sorry for the latency.<br>
          <br>
          I've a couple of nits and one question, all about this file:<br>
          <b>&nbsp; src/share/vm/classfile/javaClasses.cpp</b><br>
          <br>
          Ii is Ok to skip my nits though.<br>
          <br>
          <br>
          <b>Nit 1:</b><br>
          Three functions (<b><span class="changed">get_methods</span></b><b>,
            get_bcis, get_mirrors</b>) have the same<br>
          assert message <b>"sanity check"</b>:<br>
          <pre><span class="changed">1309   // get info out of chunks</span>
<span class="changed">1310   static typeArrayOop <b>get_methods</b>(objArrayHandle chunk) {</span>
<span class="changed">1311     typeArrayOop methods = typeArrayOop(chunk-&gt;obj_at(trace_methods_offset));</span>
<span class="changed">1312     assert(methods != NULL, <b>"sanity check"</b>);</span>
<span class="changed">1313     return methods;</span>
1314   }</pre>
          It is more helpful if the messages are more specific.<br>
        </div>
      </blockquote>
      <br>
      Okay, I also don't like "sanity check" as an assertion message.&nbsp;&nbsp;
      I'll say "methods|bcis|mirror array should be initialized in
      backtrace"<br>
      <blockquote cite="mid:50F839DD.60203@oracle.com" type="cite">
        <div class="moz-cite-prefix"> <br>
          <b>Nit 2:</b><br>
          It'd simplify code a little bit it the second of the two <b>print_stack_element</b>()


          functions<br>
          makes a call to the first one instead of the <b>print_stack_element_to_buffer</b>.<br>
          It'd save just two lines of code: <b>ResourceMark</b> and <b>print_cr</b>
          call.<br>
          <meta http-equiv="content-type" content="text/html;
            charset=ISO-8859-1">
          <pre><span class="new">1383 void java_lang_Throwable::<b>print_stack_element</b>(outputStream *st, Handle mirror,</span>
<span class="new">1384                                               int method_id, int version, int bci) {</span>
<span class="new">1385   ResourceMark rm;</span>
<span class="new">1386   char* buf = print_stack_element_to_buffer(mirror, method_id, version, bci);</span>
<span class="new">1387   st-&gt;print_cr("%s", buf);</span>
<span class="new">1388 }</span>
<span class="new">1389 </span>
<span class="new">1390 void java_lang_Throwable::<b>print_stack_element</b>(outputStream *st, methodHandle method, int bci) {</span>
<span class="new">1391   ResourceMark rm;</span>
<span class="new">1392   Handle mirror = method-&gt;method_holder()-&gt;java_mirror();</span>
<span class="new">1393   int method_id = method-&gt;method_idnum();</span>
<span class="new">1394   int version = method-&gt;constants()-&gt;version();</span>
<span class="new">1395   char* buf = print_stack_element_to_buffer(mirror, method_id, version, bci);</span>
<span class="new">1396   st-&gt;print_cr("%s", buf);</span>
<span class="new">1397 }</span></pre>
        </div>
      </blockquote>
      <br>
      Yes, that's good.&nbsp; I did that.<br>
      <blockquote cite="mid:50F839DD.60203@oracle.com" type="cite">
        <div class="moz-cite-prefix"> <br>
          <b>Nit 3:</b><br>
          It seems as possible to rearrange the code a little bit so
          that the constructor<br>
          <b>BacktraceBuilder</b><b>(ObjArrayOop backtrace)</b> could
          call the functions<br>
          <b>get_methods()</b>, <b>get_bcis()</b> and <b>get_mirror()</b>
          instead of doing the same on its own.<br>
          The functions will need to accept <b>Oop</b> arguments
          instead of <b>Handle</b>'s.<br>
        </div>
      </blockquote>
      <br>
      Thanks!&nbsp; That's a nice suggestion.&nbsp;&nbsp; I made the
      BacktraceBuilder(objArrayHandle) argument a handle and took out
      some oop-&gt;handle conversions in the caller.<br>
      <br>
      <blockquote cite="mid:50F839DD.60203@oracle.com" type="cite">
        <div class="moz-cite-prefix"> <br>
          <b>Question 1</b><b>:</b><br>
          Is it Ok that some of the <b>BacktraceBuilder</b> fields are
          <b>Oops</b>, not <b>Handles</b>?<br>
          <br>
        </div>
      </blockquote>
      <br>
      Yes, it's disturbing but there is a No_Safepoint_Verifier in
      BacktraceBuilder so it asserts that you won't hit a safepoint and
      can hold oops in it.&nbsp;&nbsp; Someone must have decided this was more
      performant.<br>
      <br>
      I made your suggested changes and the new webrev is at <a
        moz-do-not-send="true" class="moz-txt-link-freetext"
        href="http://cr.openjdk.java.net/%7Ecoleenp/7174978_7">http://cr.openjdk.java.net/~coleenp/7174978_7</a><br>
      <br>
      Thank you for the review and improvements.<br>
      <br>
      Coleen<br>
      <br>
      <blockquote cite="mid:50F839DD.60203@oracle.com" type="cite">
        <div class="moz-cite-prefix"> <br>
          <br>
          Thanks,<br>
          Serguei<br>
          <br>
          <br>
          On 1/14/13 6:36 PM, Coleen Phillimore wrote:<br>
        </div>
        <blockquote cite="mid:50F4C0A5.4070705@oracle.com" type="cite">Summary:


          Remove Method* from backtrace but save version so redefine
          classes doesn't give inaccurate line numbers.&nbsp; Removed old
          Merlin API with duplicate code. <br>
          <br>
          Sorry, this is so long but I want to explain this change
          properly. <br>
          <br>
          When exceptions are thrown, the VM saves a backtrace for each
          which may or may not be printed or saved for later.&nbsp;&nbsp; For
          speed, the information that the VM saves are arrays 32 of
          Method* and bci. For permgen elimination we added an array of
          mirrors (instances of java_lang_Class), so that the class
          loader owning the Method* pointer doesn't get unloaded and
          deallocated. <br>
          <br>
          The problem with redefine classes is that the method can be
          redefined and the Method* pointer can be deallocated even if
          the class loader owning the Method* is kept alive.&nbsp;&nbsp;&nbsp; Printing
          or touching the backtrace later will crash the VM. <br>
          <br>
          This is my 4th attempt at fixing this problem.&nbsp;&nbsp; The first
          that was reviewed, saved a side structure for each backtrace
          created so that we can walk the Method* pointers and keep them
          from being deallocated.&nbsp;&nbsp; This change had an unfortunate side
          structure to manage, and had also a 6.1% slowdown in javac.&nbsp;&nbsp;&nbsp;
          The second attempt was to predigest the Method* into the data
          needed for java_lang_StackTraceElement into method_name,
          class_name, file_name, and line_number, so that the Method*
          doesn't need to be saved. This had a 18% slowdown in javac. <br>
          <br>
          The 3rd attempt is to look for a way to have GC handle
          backtraces and mark methods as on_stack but adding a
          InstanceThrowableKlass and determining which closures needed
          special actions for the array of Method* pointers is a lot of
          additional special case code to the garbage collectors.&nbsp; It's
          unclear how this would work actually, but we might have to
          revisit this idea for java_lang_invoke_MemberName. <br>
          <br>
          The 4th attempt is to save the method_idnum, bci,
          version_number, and mirror.&nbsp;&nbsp; From the method_idnum, we can
          get the Method*.&nbsp;&nbsp; The version number is updated for redefine
          classes.&nbsp;&nbsp; There was an unused field that I repurposed.&nbsp;&nbsp; If
          the method's version doesn't match the backtrace, we do not
          return the source_file and line_number from the Method*
          because it's changed.&nbsp;&nbsp; You get a backtrace of the form: <br>
          <br>
          java.lang.RuntimeException: Test exception <br>
          &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; at
          RedefineMethodInBacktraceTarget.methodToRedefine(Unknown
          Source) <br>
          &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; at sun.reflect.NativeMethodAccessorImpl.invoke0(Native
          Method) <br>
          &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)<br>
          &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)<br>
          &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; at java.lang.reflect.Method.invoke(Method.java:474) <br>
          &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; at
RedefineMethodInBacktraceApp.getThrowableFromMethodToRedefine(RedefineMethodInBacktraceApp.java:60)<br>
          &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; at
RedefineMethodInBacktraceApp.doMethodInBacktraceTest(RedefineMethodInBacktraceApp.java:44)<br>
          <br>
          This fix for the Method* crash does not add to the footprint
          of the backtraces (idnum is short and bci and version number
          is packed into an int).&nbsp; It does not degrade performance of
          javac (or refworkload).&nbsp;&nbsp; The cost is to not support getting a
          source file and line number from a method that was redefined
          after being saved in a backtrace.&nbsp; Note that this change does
          not return the wrong information.&nbsp;&nbsp; A CR can be filed for
          somebody to someday lift this restriction if someday we don't
          care about performance or a new solution is found. <br>
          <br>
          This might be hard to review as a diff because I refactored
          duplicated code. <br>
          <br>
          The webrev:&nbsp; <a moz-do-not-send="true"
            class="moz-txt-link-freetext"
            href="http://cr.openjdk.java.net/%7Ecoleenp/7174978_5/">http://cr.openjdk.java.net/~coleenp/7174978_5/</a>
          <br>
          Bug link: <a moz-do-not-send="true"
            class="moz-txt-link-freetext"
            href="http://bugs.sun.com/view_bug.do?bug_id=7174978">http://bugs.sun.com/view_bug.do?bug_id=7174978</a>
          <br>
          <br>
          Tested with NSK quick testlist, runThese jck tests, JPRT,
          refworkload. <br>
          <br>
          Thanks, <br>
          Coleen <br>
          <br>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>