<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">On 2/4/2013 10:15 AM, Daniel D.
      Daugherty wrote:<br>
    </div>
    <blockquote cite="mid:510FD08E.2050403@oracle.com" type="cite">
      <meta content="text/html; charset=ISO-8859-1"
        http-equiv="Content-Type">
      <tt>Adding back the missing aliases and people...<br>
      </tt></blockquote>
    <br>
    Sorry, I meant to send this re-all.<br>
    <br>
    I missed something major in my earlier review.<br>
    <br>
    The metadata-&gt;is_valid() flag should only be enabled in debug
    mode.&nbsp; It adds a word to all metadata.<br>
    <blockquote cite="mid:510FD08E.2050403@oracle.com" type="cite"><tt>
        <br>
        <br>
        Coleen,<br>
        <br>
        Thanks for the review. Replies embedded below.<br>
        <br>
        <br>
      </tt>
      <div class="moz-cite-prefix">On 2/4/13 7:26 AM, Coleen Phillimore
        wrote:<br>
      </div>
      <blockquote cite="mid:510FC519.1090808@oracle.com" type="cite">
        <meta content="text/html; charset=ISO-8859-1"
          http-equiv="Content-Type">
        <div class="moz-cite-prefix">On 2/1/2013 6:55 PM, Daniel D.
          Daugherty wrote:<br>
        </div>
        <blockquote cite="mid:510C55DF.3000002@oracle.com" type="cite">And

          here is the webrev for the new tests (relative to
          JDK8-T&amp;L): <br>
          <br>
          <a moz-do-not-send="true" class="moz-txt-link-freetext"
            href="http://cr.openjdk.java.net/%7Edcubed/8007420-webrev/0-jdk8-tl/">http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/</a>
          <br>
          <br>
          As always, comments and suggestions are welcome. <br>
          <br>
          Dan <br>
          <br>
          <br>
          On 2/1/13 4:39 PM, Daniel D. Daugherty wrote: <br>
          <blockquote type="cite">&gt; There are two new tests that will
            be pushed to the JDK repos using <br>
            &gt; a different bug ID (not yet filed): <br>
            <br>
            New bug is now filed: <br>
            <br>
            &nbsp;&nbsp;&nbsp; 8007420 add test for 6805864 to com/sun/jdi, add test
            for 7182152 <br>
            &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; to java/lang/instrument <br>
            &nbsp;&nbsp;&nbsp; <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8007420">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8007420</a>
            <br>
            &nbsp;&nbsp;&nbsp; <a moz-do-not-send="true" class="moz-txt-link-freetext"
              href="https://jbs.oracle.com/bugs/browse/JDK-8007420">https://jbs.oracle.com/bugs/browse/JDK-8007420</a>
            <br>
            <br>
            Of course, the tests cannot be pushed until the HSX changes
            have made <br>
            it into a promoted build and thus available to JDK8-T&amp;L.
            <br>
            <br>
            Dan <br>
            <br>
            <br>
            On 2/1/13 12:55 PM, Daniel D. Daugherty wrote: <br>
            <blockquote type="cite">Greetings, <br>
              <br>
              I have a fix for the following JVM/TI bug: <br>
              <br>
              &nbsp;&nbsp;&nbsp; 7182152 Instrumentation hot swap test incorrect
              monitor count <br>
              &nbsp;&nbsp;&nbsp; <a moz-do-not-send="true"
                class="moz-txt-link-freetext"
                href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7182152">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7182152</a>
              <br>
              &nbsp;&nbsp;&nbsp; <a moz-do-not-send="true"
                class="moz-txt-link-freetext"
                href="https://jbs.oracle.com/bugs/browse/JDK-7182152">https://jbs.oracle.com/bugs/browse/JDK-7182152</a>
              <br>
              <br>
              The fix for the bug in the product code is one line: <br>
              <br>
              src/share/vm/oops/klassVtable.cpp: <br>
              <br>
              @@ -992,18 +1020,50 @@ <br>
              &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; // RC_TRACE macro has an embedded ResourceMark
              <br>
              &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; RC_TRACE(0x00200000, ("itable method update:
              %s(%s)", <br>
              &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; new_method-&gt;name()-&gt;as_C_string(), <br>
              &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
              new_method-&gt;signature()-&gt;as_C_string())); <br>
              &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; } <br>
              -&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; break; <br>
              +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; // cannot 'break' here; see for-loop comment
              above. <br>
              &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; } <br>
              &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; ime++; <br>
              &nbsp;&nbsp;&nbsp;&nbsp; } <br>
              &nbsp;&nbsp; } <br>
              &nbsp;} <br>
              <br>
              and is applicable to JDK7u10/HSX-23.6 and JDK7u14/HSX-24.
              Coleen <br>
              already fixed the bug as part of the Perm Gen Removal
              (PGR) project <br>
              in HSX-25. Yes, we found a 1-line bug fix buried in the
              monster PGR <br>
              changeset. Many thanks to Coleen for her help in this bug
              hunt! <br>
              <br>
              The rest of the code in the webrevs are: <br>
              <br>
              - additional JVM/TI tracing code backported from Coleen's
              PGR changeset <br>
              - additional JVM/TI tracing code added by me and forward
              ported to HSX-25 <br>
              - a new -XX:TraceRedefineClasses=16384 flag value for
              finding these <br>
              &nbsp; elusive old or obsolete methods <br>
              - exposure of some printing code to the PRODUCT build so
              that the new <br>
              &nbsp; tracing is available in a PRODUCT build <br>
              <br>
              You might be wondering why the new tracing code is exposed
              in a PRODUCT <br>
              build. Well, it appears that more and more PRODUCT bits
              deployments are <br>
              using JVM/TI RedefineClasses() and/or RetransformClasses()
              at run-time <br>
              to instrument their systems. This bug (7182152) was only
              intermittently <br>
              reproducible in the WLS environment in which it occurred
              so I made the <br>
              tracing available in a PRODUCT build to assist in the
              hunt. <br>
              <br>
              Raj from the WLS team has also verified that the HSX-23.6
              version of <br>
              fix resolves the issue in his environment. Thanks Raj! <br>
              <br>
              Here are the URLs for the three webrevs: <br>
              <br>
              <a moz-do-not-send="true" class="moz-txt-link-freetext"
                href="http://cr.openjdk.java.net/%7Edcubed/7182152-webrev/0-hsx23.6/">http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx23.6/</a>
              <br>
              <a moz-do-not-send="true" class="moz-txt-link-freetext"
                href="http://cr.openjdk.java.net/%7Edcubed/7182152-webrev/0-hsx24/">http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx24/</a>
              <br>
              <a moz-do-not-send="true" class="moz-txt-link-freetext"
                href="http://cr.openjdk.java.net/%7Edcubed/7182152-webrev/0-hsx25/">http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx25/</a>
              <br>
            </blockquote>
          </blockquote>
        </blockquote>
        <br>
        In cpCache.cpp:<br>
        <pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><span class="new" style="color: blue; font-weight: normal;">RC_TRACE_NO_CR(0x00004000, (""));

</span><span class="new" style="color: blue; font-weight: normal;"></span>
</pre>
        Can this "searchable prefix" be defined in jvmtiTracing with the
        rest of the RC_TRACE macros as some descriptive RC_TRACE name?&nbsp;
        Doesn't have to be long but this is distracting stuff.&nbsp;&nbsp; Also,
        if you change this searchable prefix, you'd only have to change
        it once.<br>
      </blockquote>
      <tt><br>
        In the cpCache.cpp case, the RC_TRACE_NO_CR(0x00004000, (""))
        macro<br>
        calls adds a searchable prefix for each dump line when the dump
        code<br>
        is called from JVM/TI RedefineClasses tracing. Other code that
        calls<br>
        the cpCache dump code won't get the JVM/TI RedefineClasses
        tracing<br>
        prefix. The purpose for the prefix is so that all tracing output
        for<br>
        a particular tracing level, e.g., 0x00004000, is grep'able
        together.<br>
      </tt></blockquote>
    <tt>This function </tt><br>
    <pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;">void ConstantPoolCacheEntry::print(outputStream* st, int index) const {
}

</pre>
    Is general purpose code. It shouldn't have all these lines specific
    to redefine classes. If it needs to, the noise should be
    minimized.&nbsp;&nbsp; You can add a define in that trace macros.hpp file:<br>
    <br>
    #define RC_PREFIX &lt;all that stuff&gt;<br>
    <br>
    This code shouldn't be present in the general purpose metadata
    printing.&nbsp;&nbsp;&nbsp; The dump_vtable and dump_itable functions are arguable
    since they seem to only be used by redefine classes now, but I can
    see a general purpose use for these too that shouldn't include this
    noise.<br>
    <br>
    <br>
    <blockquote cite="mid:510FD08E.2050403@oracle.com" type="cite"><tt>
        <br>
        It wouldn't be appropriate to add the "searchable prefix" to the<br>
        jvmtiTraceRedefineClasses.hpp file. The HEX values in JVM/TI<br>
        RedefineClasses tracing are associated with the code being
        traced.<br>
        <br>
        <br>
      </tt>
      <blockquote cite="mid:510FC519.1090808@oracle.com" type="cite"> In
        jvmtiRedefineClasses.cpp why can't dump_methods just print the
        methods without these RC_TRACE macros? </blockquote>
      <tt><br>
        Debug output style for JVM/TI RedefineClasses()</tt><tt> is
        supposed to be done<br>
        using the jvmtiTraceRedefineClasses mechanism. It was
        inconsistent for<br>
        dump_methods() to do its output the way it was so I fixed it.<br>
        <br>
      </tt>
      <blockquote cite="mid:510FC519.1090808@oracle.com" type="cite">And
        some is printed and some is not?</blockquote>
      <tt><br>
        No, in this case, every line will have a
        jvmtiTraceRedefineClasses<br>
        prefix on it when that tracing level is turned on. Example:<br>
        <br>
        RedefineClasses-0x4000: _old_methods --<br>
        RedefineClasses-0x4000: 0 ( -2) public {old} -- virtual void
        RedefineSubclassWithTwoInterfacesTarget.&lt;init&gt;()<br>
        RedefineClasses-0x4000: 1 ( 5) public {old} {obsolete} --
        virtual jobject
        RedefineSubclassWithTwoInterfacesTarget.echo(jobject)<br>
        <br>
        Again, the prefix, "RedefineClasses-0x4000:", exists so that
        everything<br>
        at that tracing level is grep'able together.<br>
        <br>
        <br>
      </tt>
      <blockquote cite="mid:510FC519.1090808@oracle.com" type="cite">This

        is really hard to read.&nbsp;&nbsp; The indentation came out strange in
        the webrev too.<br>
      </blockquote>
      <br>
      <tt>Yes, the original dump_methods() code didn't follow hotspot<br>
        style and I reformatted it since I was changing much of the<br>
        code in the function.<br>
        <br>
        I tend to use "frames" view in webrevs and I don't see any<br>
        issues with indentation.<br>
        <br>
        <br>
      </tt></blockquote>
    <br>
    I see what happened with dump_methods.&nbsp; I think it was refactored
    (to cut/paste places) and the indentation wasn't fixed.&nbsp; You can
    leave the noise in there since it's in redefine/classes, but I
    really object to it in the general purpose code.<br>
    <br>
    Coleen<br>
    <br>
    <blockquote cite="mid:510FD08E.2050403@oracle.com" type="cite"><tt>
        <br>
      </tt>
      <blockquote cite="mid:510FC519.1090808@oracle.com" type="cite"> It
        looks like the call to dump_methods() is covered by one of these
        RC_TRACE macros.</blockquote>
      <br>
      <tt>Yes, I did that intentionally. If I didn't then I would have
        to have<br>
        redone all the print code to match jvmtiTraceRedefineClasses
        style.<br>
        It was easier to add RC_TRACE_NO_CR() calls where needed and
        protect<br>
        the initial call with an RC_TRACE_ENABLED check.<br>
        <br>
        <br>
      </tt>
      <blockquote cite="mid:510FC519.1090808@oracle.com" type="cite">Why
        isn't that enough?</blockquote>
      <tt><br>
        Because all RedefineClasses debug output is supposed to have a<br>
        prefix like this one:<br>
        <br>
        RedefineClasses-0x4000: _old_methods --<br>
        <br>
        <br>
      </tt>
      <blockquote cite="mid:510FC519.1090808@oracle.com" type="cite">This

        is really distracting because I keep wondering why it's
        RC_TRACE_NO_CR (don't file a CR??) rather than reading the
        code.&nbsp;&nbsp; Oh, it's no carriage return.&nbsp;&nbsp; Ugh.</blockquote>
      <br>
      <tt>You mean like print_cr()? :-)<br>
        <br>
        <br>
      </tt>
      <blockquote cite="mid:510FC519.1090808@oracle.com" type="cite">I
        still would like dump_methods to always dump all the methods so
        if you're debugging this you can temporarily paste this call
        various places without trying to figure out which RC_TRACE
        number to give it.<br>
      </blockquote>
      <br>
      <tt>Sorry, that's not how debugging in RedefineClasses is supposed
        to work.<br>
        dump_methods() was an outlier that needed to be fixed so that it
        matched<br>
        the rest of the jvmtiTraceRedefineClasses infrastructure.<br>
        <br>
        Of course, the Serviceability team is welcome to change this
        debugging<br>
        code to suite their own style and tastes. I think that would be
        an easier<br>
        task if it was all "the same".<br>
        <br>
        Dan<br>
        <br>
        <br>
      </tt>
      <blockquote cite="mid:510FC519.1090808@oracle.com" type="cite"> <br>
        Coleen<br>
        <br>
        <pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><span class="new" style="color: blue; font-weight: normal;"></span></pre>
        <blockquote cite="mid:510C55DF.3000002@oracle.com" type="cite">
          <blockquote type="cite">
            <blockquote type="cite"> I have run the following test
              suites from the JPDA stack on the <br>
              JDK7u10/HSX-23.6 version of the fix with
              -XX:TraceRedefineClasses=16384 <br>
              specified: <br>
              <br>
              &nbsp;&nbsp;&nbsp; sdk-jdi <br>
              &nbsp;&nbsp;&nbsp; sdk-jdi_closed <br>
              &nbsp;&nbsp;&nbsp; sdk-jli <br>
              &nbsp;&nbsp;&nbsp; vm-heapdump <br>
              &nbsp;&nbsp;&nbsp; vm-hprof <br>
              &nbsp;&nbsp;&nbsp; vm-jdb <br>
              &nbsp;&nbsp;&nbsp; vm-jdi <br>
              &nbsp;&nbsp;&nbsp; vm-jdwp <br>
              &nbsp;&nbsp;&nbsp; vm-jvmti <br>
              &nbsp;&nbsp;&nbsp; vm-sajdi <br>
              <br>
              The tested configs are: <br>
              <br>
              &nbsp;&nbsp;&nbsp; {Solaris-X86, WinXP} <br>
              &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; X {Client VM, Server VM} <br>
              &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; X {-Xmixed, -Xcomp} <br>
              &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; X {product, fastdebug} <br>
              <br>
              With the 1-liner fix in place, the new tracing code does
              not find any <br>
              instances of this failure mode in any of the above test
              suites. Without <br>
              the the 1-liner fix in place, the new tracing code finds
              one instance <br>
              of this failure mode in the above test suites: <br>
              <br>
              &nbsp;&nbsp;&nbsp; test/java/lang/instrument/IsModifiableClassAgent.java
              <br>
              <br>
              There are two new tests that will be pushed to the JDK
              repos using <br>
              a different bug ID (not yet filed): <br>
              <br>
              &nbsp;&nbsp;&nbsp; test/com/sun/jdi/RedefineAbstractClass.sh <br>
              test/java/lang/instrument/RedefineSubclassWithTwoInterfaces.sh


              <br>
              <br>
              There will be a separate review request for the new tests.
              <br>
              <br>
              I'm currently running the JPDA stack of tests on the
              JDK7u14/HSX-24 <br>
              and JDK8-B75/HSX-25 versions of the fix. That testing will
              likely <br>
              take all weekend to complete. <br>
              <br>
              Thanks, in advance, for any comments and/or suggestions. <br>
              <br>
              Dan <br>
              <br>
              <br>
              <br>
            </blockquote>
            <br>
            <br>
            <br>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>