<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->is_valid() flag should only be enabled in debug
mode. 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&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">> There are two new tests that will
be pushed to the JDK repos using <br>
> a different bug ID (not yet filed): <br>
<br>
New bug is now filed: <br>
<br>
8007420 add test for 6805864 to com/sun/jdi, add test
for 7182152 <br>
to java/lang/instrument <br>
<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>
<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&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>
7182152 Instrumentation hot swap test incorrect
monitor count <br>
<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>
<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>
// RC_TRACE macro has an embedded ResourceMark
<br>
RC_TRACE(0x00200000, ("itable method update:
%s(%s)", <br>
new_method->name()->as_C_string(), <br>
new_method->signature()->as_C_string())); <br>
} <br>
- break; <br>
+ // cannot 'break' here; see for-loop comment
above. <br>
} <br>
ime++; <br>
} <br>
} <br>
} <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>
elusive old or obsolete methods <br>
- exposure of some printing code to the PRODUCT build so
that the new <br>
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?
Doesn't have to be long but this is distracting stuff. 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. You can add a define in that trace macros.hpp file:<br>
<br>
#define RC_PREFIX <all that stuff><br>
<br>
This code shouldn't be present in the general purpose metadata
printing. 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.<init>()<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. 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. I think it was refactored
(to cut/paste places) and the indentation wasn't fixed. 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. Oh, it's no carriage return. 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>
sdk-jdi <br>
sdk-jdi_closed <br>
sdk-jli <br>
vm-heapdump <br>
vm-hprof <br>
vm-jdb <br>
vm-jdi <br>
vm-jdwp <br>
vm-jvmti <br>
vm-sajdi <br>
<br>
The tested configs are: <br>
<br>
{Solaris-X86, WinXP} <br>
X {Client VM, Server VM} <br>
X {-Xmixed, -Xcomp} <br>
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>
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>
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>